On Thu, 30 Jul 2026 22:49:30 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: >> - A new private `CssHelperState` is introduced on `Node`. We need to know >> whether we can trust the `styleHelper`. >> - 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 two 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 > > # 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 104: > 102: Styleable parent = node; > 103: int depth = 0; > 104: while (parent != null) { there might be potential quadratic execution time here. imagine a long chain of nodes root + node1 + node2 ... + nodeN - start by adding a child to nodeN, this makes the entire chain dirty. - change style class in all the nodes in the chain - add a second child to nodeN adding the second child recursively rebuilds every stale ancestor, walking all the way to the root. modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 119: > 117: > 118: if (node.styleHelper != null) { > 119: setFirstStyleableAncestor(node.styleHelper, > styleableAncestor); this changes the existing helper's ancestor unconditionally. the helper might contain stale styles (especially during style or hierarchy updates from within the listeners) modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 285: > 283: private static boolean isStyleableAncestor(Node parentNode) { > 284: if (parentNode.cssHelperState == Node.CssHelperState.STALE) { > 285: parentNode.cssHelperState = > Node.CssHelperState.RESOLVED_EARLY; there might be a problem here: this code sets `RESOLVED_EARLY` before creating the `styleHelper`. createStyleHelper() can invoke application listeners that might, for example, add new nodes and force `applyCss()` immediately. The re-entrance descendants see `RESOLVED_EARLY` and trust the old, stale, `styleHelper`. would it make sense to add another state, or perhaps move the state into `styleHelper` ? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2225#discussion_r3713907177 PR Review Comment: https://git.openjdk.org/jfx/pull/2225#discussion_r3714127349 PR Review Comment: https://git.openjdk.org/jfx/pull/2225#discussion_r3713717010
