On Fri, 3 Jul 2026 16:36:45 GMT, Marius Hanl <[email protected]> wrote:
> This one took me several days to fix. Below I will try to explain every > information I found out while debugging and fixing this. > > Originally I found this weird CSS issue already years ago, hence my comment > in the issue. > It happened because `ControlsFX` secretly changes your `Scene` root to its > `DecorationPane` (and attaches your old root under it). This seems to trigger > CSS errors sometimes that do not make sense and the UI seems to be fine as > well. > > I tried to write several tests and scenarios that should work or not work. So > some tests succeed without the fix and also after. > The fix is only in `CssStyleHelper` and consists of two changes. Both for the > `firstStyleableAncestor`, which has some issues. > > The `firstStyleableAncestor` was introcuded in > https://bugs.openjdk.org/browse/JDK-8090462, so this is technically somewhat > a regression from there. > > ### The fix > > A child can change the scene structure (e.g. change the root) as a result > from processing its CSS. I wrote multiple tests to show that. Because of this > scenario, we need to change how the `firstStyleableAncestor` is found. > > 1. The `firstStyleableAncestor` is now found by eagerly creating its > `styleHelper` (before, we just checked if it has one). If this worked (-> > that node is styleable), we have our ancestor. > - 1.1 In `createStyleHelper`, we already traverse the parents to get the > `depth`, so we can optimize our `firstStyleableAncestor` search by hanging > into the loop and check if this is our styleable ancestor > > 2. In very rare cases `firstStyleableAncestor` can be null for a non-root > `Node`. This, again can happen if something changes while we are processing > the CSS. Normally CSS processing is top-down. But when a child changes a > parent (or root), the structure changes and in this case we could no longer > have a `firstStyleableAncestor`. So both CSS processing methods `resolveRef` > and `getInheritedStyle` will search for a styleable ancestor if there is none > set. > - 2.1. This usually never happens and for `Scene` roots this will be a noop. > > #### Performance > > Performance wise, I could not see any problem or regression. We still cache > our `firstStyleableAncestor` (and even reuse a preexisting loop). > We might create a parent `styleHelper` more early in rare scenarios, but > those will be created anyway the next pulse and usually are reused then. > > I also avoid to create any null `WeakReference` objects, as `resolveRef` > could be hot path if the user hovers e.g. over table entries. So no objects > are c... This looks **very** good, thanks for the set of tests (which fail as expected in master). modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java line 975: > 973: @Test > 974: void testLookupResolvesWithPseudoClassAndIntermediatePane() { > 975: var errors = StyleManager.errorsProperty(); should we use `CssParser.errorsProperty()` instead here? (they are the same, but the `CssParser` is a public API while this one is not) ------------- Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/2201#pullrequestreview-4640053476 PR Review Comment: https://git.openjdk.org/jfx/pull/2201#discussion_r3532309756
