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 technically, 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` handles the absence of a cached property. Basically, absence can mean two things: - 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 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 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 removed) All of those tests should fail with the old implementation. The test that most closely resembles the problem with the regression reported in [JDK-8388378](https://bugs.openjdk.org/browse/JDK-8388378) is the skin replacement test: - VirtualFlow creates new TreeTableRows **during** layout (it really shouldn't, but whatever) - Such new rows are added to the scene graph, and have CSS applied by some logic in Node when its parent/scene is assigned -- however, these are still **unskinned** as the skin logic in `Control` didn't have a chance to run yet, meaning their CSS metadata does not have the property `-fx-indent` in them -- the CSS engine therefore doesn't know of that property and created a cache entry lacking it - When later another node shares the same cache entry, the absence of the `-fx-indent` property was treated as "unstyled" and it got its value reset to whatever the default indent was... ## The Change I adjusted `CssStyleHelper` to only ever use explicitly set values in the cache; this can be a non-null `CalculatedValue` or the special value `SKIP`. If nothing was in the cache at all, it is interpreted "never evaluated" and so must be looked up. It no longer conflates absence of a value with being unstyled. This however does mean that we must store more values in the cache. Any value that was evaluated, but did not result in a value that came from CSS is now set to `SKIP`, as otherwise it would be absent and the new logic would then re-evaluate it each time. So what does that effectively mean? It means that where before a cached entry only contained entries that were explicitly assigned a CSS value, it now always contains **all** properties found in the CSS metadata -- most of those will be set to the sentinel `SKIP`, so the cost for this is low. As the entry is also shared (and is now much safer to share) the total cost for these extra `SKIP` values in the cached entry is probably neglible, but it is certainly more than it was before. I think correctness here is more important than slightly more overhead, and since it was the only solution I could think of that would solve these problems, I went ahead with this fix. If later it proves the overhead is not as neglible as I thought, we can adjust it knowing that we have several tests that catch problems with incorrect use of caches. One possible direction to go here is to use a more efficient `HashMap` implementation, possibly even just a simple list of values instead of a map, but that's definitely out of scope IMHO. --------- - [x] I confirm that I make this contribution in accordance with the [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). ------------- Commit messages: - Fix CssStyleHelper treating absent cache values as "no style applies" Changes: https://git.openjdk.org/jfx/pull/2218/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=2218&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8388378 Stats: 505 lines in 3 files changed: 454 ins; 37 del; 14 mod Patch: https://git.openjdk.org/jfx/pull/2218.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/2218/head:pull/2218 PR: https://git.openjdk.org/jfx/pull/2218
