On Mon, 20 Jul 2026 10:07:38 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...
>
> 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
some people write two spaces after the period (I do). but a) it's an internal
class and b) even if it were a public javadoc, two spaces will not render in
html.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2218#discussion_r3625661558
