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... I like the removal of all this conditionals around the caching, the new code is much more straightforward. Found a couple of possible issues (inline). One persistent thought I had is whether or not it is possible to have a more formalized approach to testing the CSS subsystem. Like, exhaustively enumerate all possible transitions and develop a test for each. modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 919: > 917: final String property = cssMetaData.getProperty(); > 918: > 919: CalculatedValue calculatedValue = cacheEntry.get(property); it looks like the value is stored by property name, which means if the SKIP value was stored and then the metadata was replaced (by setting a skin, for example, which uses the same property names), the old SKIP would prevent re-evaluation. modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 946: > 944: */ > 945: > 946: if (!cssMetaData.isSettable(node)) continue; the contract for `CssMetaData.isSettable()` says "_This method is called before any styles are looked up for the given property._" but here it's called after the lookup() in L929 modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 955: > 953: * JDK-8127435: If there is no style for the property > (SKIP), then check if it must be reset > 954: * to its initial value. Otherwise, continue with CSS > application. > 955: */ minor: I would have used `//` style comments modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java line 879: > 877: /* > 878: * The other node is now transitioned to the same state. Since > the value of -fx-boom > 879: * could not be evaluated, the value is reset to its default > value (1.0). shouldn't it be the value specified by the stylesheet? 41.0, since the styling of the `other` has not encountered an error? ------------- PR Review: https://git.openjdk.org/jfx/pull/2218#pullrequestreview-4748982700 PR Review Comment: https://git.openjdk.org/jfx/pull/2218#discussion_r3625969179 PR Review Comment: https://git.openjdk.org/jfx/pull/2218#discussion_r3626119670 PR Review Comment: https://git.openjdk.org/jfx/pull/2218#discussion_r3625674650 PR Review Comment: https://git.openjdk.org/jfx/pull/2218#discussion_r3626038526
