On Tue, 21 Jul 2026 22:08:51 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/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? You are right that `41.0` would make more sense here, but that's assuming we could get that value at all. The source of the exception used here is just to test the `SKIP` adding code path (as it is crucial now that we add `SKIP` or a value for every value in the CSS metadata). The path I was testing however is *normally* only triggered when `lookup` fails, and when that fails there simply isn't a value to put in the cache at all (ie. wrong type in the CSS or you used a reference that doesn't exist). `applyStyle` isn't allowed to throw exception, so results are "best effort" (and how this worked is pre-existing, I didn't change it). ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2218#discussion_r3626388282
