On Tue, 29 Sep 2026 11:15:18 GMT, Marius Hanl <[email protected]> wrote:
>> Found while implementing a fix in https://github.com/openjdk/jfx/pull/2225 >> and also marked in the review by @andy-goryachev-oracle. >> >> `CssStyleHelper` holds the `triggerStates`. Those are needed for pseudoclass >> matching. >> >> Example: `.parent:ps > .leaf { ... }` >> What happens: >> `leaf` is adding `ps` into the triggerStates of `parent`. >> This is done so that `parent` will know: If my pseudoclass `ps` changes, I >> have children that are interested in that change, so I must update the CSS. >> >> The problem with that currently: >> We might need to create an empty `CssStyleHelper` 'shell' to hold the >> `triggerStates`. >> >> In the example above, we will create a `CssStyleHelper` for `parent` only >> for the `triggerStates`. >> Because `parent` is not styled, it normally has no `CssStyleHelper`. >> >> Because of that mechanism, the code in `CssStyleHelper` has multiple >> problems: >> 1. We need to check if we have a real `CssStyleHelper` or just a 'shell' for >> the `triggerStates` >> 1.1. This is done by various checks for `cacheContainer != null` >> 2. `findFirstStyleableAncestor()` can return a `Node` with such an empty >> `CssStyleHelper` (that was only created for the `triggerStates`), which is >> wrong >> 2.1. I managed to write a test that actually fails because of this >> misbehavior. With this refactor, the test succeeds >> 3. We create more `CssStyleHelper` objects than we actually need to >> >> Solution: Move the `triggerStates` to `Node`. This is also where the other >> css flags live in. >> >> Overall, this will improve performance a bit, make the `CssStyleHelper` a >> bit cleaner, especially regarding separation of concerns. >> Now the following is always true: If a `CssStyleHelper` exists, the `Node` >> is styled and always has a `CacheContainer` that is never null. >> Will help towards finishing: https://github.com/openjdk/jfx/pull/2225. >> >> I wrote a bunch of tests that will succeed before and after. >> One test fails before and succeeds now. >> No change in behavior is expected other than being more correct and fixing >> one misbehavior. >> >> --------- >> - [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 incrementally with one additional > commit since the last revision: > > Failing test due to findFirstStyleableAncestor set to a 'shell' > CssStyleHelper Some preliminary minor comments for now - the code feels like we are going in the right direction. I'll do a more thorough review later. modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 176: > 174: updateTriggerStates(node, depth, triggerStates); > 175: > 176: final CssStyleHelper helper = new CssStyleHelper(new > CacheContainer(node, styleMap, depth)); no need for `final` keyword if the variable is effectively final modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 356: > 354: if ( parent instanceof Node) { > 355: Node parentNode = (Node)parent; > 356: final CssStyleHelper helper = parentNode.styleHelper; unneeded `final` ------------- PR Review: https://git.openjdk.org/jfx/pull/2333#pullrequestreview-5354051894 PR Review Comment: https://git.openjdk.org/jfx/pull/2333#discussion_r4134657223 PR Review Comment: https://git.openjdk.org/jfx/pull/2333#discussion_r4134673928
