On Sun, 19 Jul 2026 22:52:26 GMT, John Hendrikx <[email protected]> wrote:
> I uncovered a problem that has been in the CSS engine for a long time, even
> before #1076 was applied. However, #1076 made this problem more obvious
> because of a side fix that was done there:
>
> - The `BitSet` `equals` implementation was updated to NOT take the length of
> the allocated array (to store the bits in) into account for equality. As this
> array is just allocated on demand depending on what bits were set and reset,
> it should not be taken into account for equality
>
> The above bug hid problems when nodes were **supposed to** share a CSS cache
> entry, but didn't because their `BitSet`s were considered different (even
> though semantically, they were the same).
>
> With the fix in #1076, a lot more cases were sharing CSS cache entries (as
> they should) but this now exposed a bug in how `CssStyleHelper` handled the
> absence of a cached property. Basically, absence could mean two things before
> this change:
>
> - The property was not computed at all: it either didn't exist at the time
> (due to `CssMetaData` changing!) or because the property was not settable
> (because it was bound)
> - The property was computed but no styles applied to it, so no need to cache
> anything...
>
> The CSS engine always assumed the latter, which means that if for whatever
> reason the cache entry was created by a Node that had outdated `CssMetaData`
> (a Control that is yet to be skinned, or one where a CSS property was
> unsettable), the engine would assume that such a missing property was
> unstyled and can safely be reset. As entries are shared, this doesn't hold
> true for all nodes that share the same entry (if another node that shares the
> same entry has different `CssMetaData` or did not have the same property
> bound, then it may have been styled, and should not be reset!).
>
> ## Tests that confirm the problem
>
> I added 4 new test cases, for four paths that could potentially result in the
> wrong things being in the cache:
>
> - A cache entry being created for a Node that has bound properties (and is
> later shared with a different node)
> - A cache entry being created for a Node that isn't skinned yet (and so it's
> CssMetaData may change); the CSS engine simply can never assume that
> CssMetaData is stable (skins can also be changed at any time)
> - Ensuring that an exception during `applyStyle` still results in a `SKIP`
> entry in the cache
> - Ensuring that we still put a value in the cache, even if the Node can't use
> it itself directly as its value is bound (this test overlaps a bit with the
> first, perhaps one can be remo...
Overall, this looks good. Nice to see some documentation for the
`CssStyleHelper` and related classes! Tests look nice as well and you explained
all scenarios very well.
And it is great to see the removal of `forceSlowpath`, which during my
debugging was hard for me to understand why it even exists.
Will do more testing later this week!
modules/javafx.graphics/src/main/java/com/sun/javafx/css/StyleCacheEntry.java
line 37:
> 35: /**
> 36: * Caches the calculated value of each styleable property a specific
> combination of
> 37: * pseudo-class states and font size (see {@link Key}). An entry is
> shared by every
Minor: Two spaces.
Suggestion:
* pseudo-class states and font size (see {@link Key}). An entry is shared by
every
modules/javafx.graphics/src/main/java/com/sun/javafx/css/StyleCacheEntry.java
line 41:
> 39: * <p>
> 40: * {@link #get(String)} returning {@code null} means only that no one has
> evaluated that
> 41: * property for this entry yet; it should never be treated as "no style
> applies"(!). This
Minor: Grammar
Suggestion:
* property for this entry yet; it should never be treated as "no style
applies"(!). This is
modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 946:
> 944: */
> 945:
> 946: if (!cssMetaData.isSettable(node)) continue;
I would prefer this style, but again, minor and is all of the place in this
file anyway
Suggestion:
if (!cssMetaData.isSettable(node)) {
continue;
}
modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java
line 780:
> 778:
> 779: /*
> 780: * The "other" dummy control has the extra property attached to
> it immediately
Those inline doc comments are very helpful!
One idea: I wonder if we should put (most of) them at the test method as
javadoc.
This is up to you, just a potential suggestion.
-------------
PR Review: https://git.openjdk.org/jfx/pull/2218#pullrequestreview-4733959682
PR Review Comment: https://git.openjdk.org/jfx/pull/2218#discussion_r3613474893
PR Review Comment: https://git.openjdk.org/jfx/pull/2218#discussion_r3613477512
PR Review Comment: https://git.openjdk.org/jfx/pull/2218#discussion_r3613484558
PR Review Comment: https://git.openjdk.org/jfx/pull/2218#discussion_r3613504977
