On Tue, 21 Jul 2026 22:25:35 GMT, Andy Goryachev <[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 b... > > 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 That's an odd amount of detail that is rather implementation specific. I would suggest we remove it, as I don't think there are good reasons for this other than "that's how it works currently", and it actually is an active source of problems. A property being unsettable does not mean you can't evaluate the stylesheet to find what value it would have gotten (and that's what we do now). The only reason this method exists is that is faster than catching "RuntimeException: A bound value cannot be set.". There are two good reasons to still evaluate the CSS value (even if we won't set it): - The property may not be bound forever; if the cache entry was created without the calculation of the value for a "locked" property, then we later won't know what CSS based value to put in there (note: unbinding a property is not detected, so binding/unbinding already plays very badly with the CSS system). - The cache entry may be shared with siblings in the same state; if the cache entry was initialized by a Node with bound properties, and we then just skip those, then the other siblings would not have that value either -- this causes incorrect value resets and properties to not be styled, which this PR specifically wants to address If there are good reasons to keep it, then the only other option we have is to NOT create a cache entry at all when a Node has any CSS property bound (a sibling that is in a better state may create it still though). This could be a performance hazard though, because if it is the only node of its kind, it would always need to go through the slow `lookup` path until there are no more bound properties. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2218#discussion_r3626440691
