On Thu, 23 Jul 2026 10:01:06 GMT, Marius Hanl <[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... > > Tested on multiple applications yesterday and today, all good. Also no > performance issues or anything noticeable. > Agree with @Maran23 , this is an improvement and looks good. I would like to > know your take on these two comments: > > 1. is it possible to have a more formalized approach to testing the CSS > subsystem. Like, exhaustively enumerate all possible transitions and develop > a test for each? It might be possible, but it is hard to say how far that could go (CSS is quite complex). If you're looking to make things more testable, then I would start by splitting up `CssStyleHelper` in a few smaller classes that focus on specific aspects that can then be independently tested. For testing transitions, I'd probably not use JUnit for something like this but a more dedicated state transition test system (I've developed an in-house one) where one simply specifies actions (with preconditions), and assertions that must hold; the framework then figures out what combinations to test. Otherwise you either will make your own "loops" in JUnit, or find that using `@Nested` quickly becomes a terrible way of testing potentially 10.000's state transitions... > 2. could relaxing the constraint on `isSettable()` API backfire somehow? I doubt it, it is an API not that much used, and the opposite approach is actually what was doing the backfiring. ------------- PR Comment: https://git.openjdk.org/jfx/pull/2218#issuecomment-5093145085
