On Wed, 16 Sep 2026 22:55:11 GMT, Marius Hanl <[email protected]> wrote:
>> Another much better try to fix the issue. >> I recommend to read: https://github.com/openjdk/jfx/pull/2201 first. All >> tests from there are included. >> I added some new ones that succeed before and after, a first step for more >> CSS tests as discussed in: >> https://github.com/openjdk/jfx/pull/2218#issuecomment-5094495306 >> >> My new idea is now the following constraint, which I think is also a much >> better approach: >> - A `CssStyleHelper` always has a correct `firstStyleableAncestor`. We can >> at any time trust and rely on it. >> - We will also reuse the existing loop for the `isUserSetFont` check to >> improve the performance a bit >> >> Implementation: >> - We now save a flag to exactly know in which CSS state the `Node` is. >> - We will collect all `Node`s in the scene tree once and then reuse the list >> when we need to process a stale `styleHelper` from a parent. >> - This will make sure we do no process the parent again and again when we >> have some stale `styleHelper`s in the chain >> - Not every `Node` has a `styleHelper` - it is only created when needed, so >> we can not attach the flag in there. >> >> This fixes the issue while a deep (optionally unstyled) scene graph has no >> performance penality. >> The approach is similar than my previous PR, but more smart. And with the >> set constraint mentioned above. >> >> --- >> >> I do think we can improve the `CssStyleHelper` more. But for another day. >> Maybe at one point, with more tests and when all requirements are clear, we >> can find a way without `CssHelperState` and without creating an empty >> `CssStyleHelper` just to hold trigger states (because of that, we need to >> check `styleHelper.cacheContainer != null` a lot of times). >> >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Marius Hanl has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge branch 'master' of https://github.com/openjdk/jfx into > 8388277-REDO]-Looked-up-color-fails-for--fx-background-color-in-JavaFX-CSS-file > - fix > - test > - idea how to fix that issue > - failing test > - Fix another broken case > - change the way we process a stale CssStyleHelper > - Merge branch 'master' of https://github.com/openjdk/jfx into > 8388277-REDO]-Looked-up-color-fails-for--fx-background-color-in-JavaFX-CSS-file > - Merge branch 'master' of https://github.com/openjdk/jfx into > 8388277-REDO]-Looked-up-color-fails-for--fx-background-color-in-JavaFX-CSS-file > > # Conflicts: > # > modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java > - 8388277: [REDO] Looked-up color fails for -fx-background-color in JavaFX > CSS file modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 111: > 109: // Listeners running while the old helper reset > properties may have changed the hierarchy. > 110: // The remaining stale ancestors must not be rebuilt for > an outdated path, so start over. > 111: if (propertiesReset && !isPathValid(path)) { a listener can make the newly installed helper stale again (for example, by changing the ancestor style). we probably need to restart when the updated node becomes stale again (in addition to checking the path). modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 157: > 155: > 156: final StyleMap styleMap = > 157: StyleManager.getInstance().findMatchingStyles(node, > node.getSubScene(), triggerStates); I think we still have a problem: `findMatchingStyles` can re-enter CSS and see the ancestors as stale, so it will try to re-build them, possibly in a loop. a solution might be to introduce `RESOLVING` state which prevents that (similar to `Parent.performingLayout` flag) modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 417: > 415: int startIndex) { > 416: int ctr = 0; > 417: int[] smapIds = new int[path.size() - startIndex]; we still seem to have quadratic allocation: each stale ancestor allocates a `triggerStates` array in L154, and each changed helper allocates and scans another suffix in its cache key. the existing test tracks neither allocations nor scans. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2225#discussion_r4049567146 PR Review Comment: https://git.openjdk.org/jfx/pull/2225#discussion_r4049357912 PR Review Comment: https://git.openjdk.org/jfx/pull/2225#discussion_r4049770047
