On Wed, 8 Jan 2025 01:29:29 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> The CSS Selectors specification defines the `:root` pseudo-class that >> matches root nodes: >> https://www.w3.org/TR/selectors-4/#the-root-pseudo >> >> JavaFX uses the non-standard `.root` style class for the same purpose. We >> should also support the `:root` pseudo-class for increased compatibility of >> JavaFX CSS with the web specification. >> >> Additionally, we should also support the following child-indexed >> pseudo-classes: >> `:first-child` >> `:last-child` >> `:only-child` >> `:nth-child()` with arguments `even` and `odd` >> >> The `An+B [of S]?` microsyntax for `:nth-child()` is not supported, as this >> would require major changes in JavaFX CSS. > > Michael Strauß has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - Merge branch 'master' into feature/root-pseudo-class > - whitespace error > - add child-indexed pseudo-classes > - add pseudo-class table to Parent > - update cssref > - Set :root pseudo-class on sub-scene root node > - Set :root pseudo-class on root node modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 1530: > 1528: <p>In the following example, all background color of all buttons > uses the > 1529: looked up color "abc".</p> > 1530: <p class="example">:root { abc: #f00 }<br> I see many places where `.root` is removed in favor of `:root`, but both are supported still? At least, the code in `Scene` leads me to believe that it will also apply the `.root` style class still. Should this legacy style class be mentioned in the CSS documentation still? modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 341: > 339: > 340: @Override > 341: protected void onChanged(Change<Node> c) { I hate to bring this up, and it perhaps should be dealt with separately, but how safe is this code in `onProposedChange` / `onChanged` when it is re-entered due to the many potential listener callbacks? For example, the removal of a child on `parent` can trigger list change listeners from the user that can trigger all kinds of other changes that eventually lead to more children changes on this node, triggering a nested `onChanged`; the new changes are adding more opportunities in the form of listeners on pseudo state changes. For example, `onProposedChange` will blatantly reset `geomChanged` to `false` in all cases, even if it rejects the change later. It doesn't check if an `onChange` is in progress higher up the call stack, effectively resetting a flag that is in active use... I believe this may be quite a difficult problem to solve if nested calls remain allowed, and one of the easiest ways to fix it IMHO is to reject any changes (in `onProposedChange`) when a change is already in progress. For users this would effectively mean that listeners triggered during a child modification will not be allowed to make further modifications to the same children list until the first one completes. This is not a common situation, but can happen with very dynamic UI's that construct new nodes in response to triggers; without such protection for nested changes, the errors that crop up are often of a form that seem to be multi-threading issues (where certain invariants seem to be broken), while in reality they're just re-entrant code calls that use instance fields in an unsafe manner. modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 375: > 373: } > 374: > 375: toggleStructuralPseudoClasses(n, List.of()); This (and other `toggleStructuralPseudoClasses` and `pseudoClassStateChanged` calls later) introduces new opportunities for user listener call backs in the middle of a pretty critical code path. `n.getParent().children.remove(n);` a few lines above is also such an opportunity. modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 381: > 379: // the index of the first change to separate > unchanged from changed elements. > 380: if (firstDirtyChildIndex < 0) { > 381: firstDirtyChildIndex = from; Does this need some special casing in case the first element is modified indirectly? Let's say I have two elements, I remove the last element. Indirectly, the first element now gets the "only" and "last" pseudo classes; this may need to be communicated as a modification to the peer for example? modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 470: > 468: // Add the "last-child" pseudo-class to the last child. > 469: if (size() > 0) { > 470: > getLast().pseudoClassStateChanged(LAST_CHILD_PSEUDO_CLASS, true); These code paths seem to do double work. Would it not be better to treat each flag separately, instead of having some paths manipulate multiple flags, that then may get set/reset again individually? The first `if/else if` in this block will reset all flags, only to have some of them be set/reset again later. This may also lead to listener confusion; let's say I have 2 children. I remove the last child. The "only child" block (`size() == 1`) triggers; it removes the `odd` pseudo class; a listener may trigger on this; however, just a bit later, this `odd` tag is then re-added (at least, I hope it is as I'm not entirely sure the `firstDirtyChildIndex` will be set low enough...) modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 479: > 477: > n.pseudoClassStateChanged(NTH_EVEN_CHILD_PSEUDO_CLASS, i % 2 != 0); > 478: > n.pseudoClassStateChanged(NTH_ODD_CHILD_PSEUDO_CLASS, i % 2 == 0); > 479: } With this new code, Nodes with many children being manipulated may incur some significant new costs now that these pseudo classes need to be changed for potentially many/all children. Where before adding a new first child mostly had a cost in shifting the child array with a copy, now we also potentially have two listener call backs per child (and I'm unsure, but these may even trigger some CSS recalculations or CSS cache invalidations). Nodes with many children may suffer from this, and mostly needlessly as these pseudo classes will see very limited use. I think there's some potential for performance regression here that is perhaps sufficient to warrant the exclusion of this feature or making it optional. modules/javafx.graphics/src/test/java/test/javafx/css/StylesheetTest.java line 790: > 788: """.getBytes(StandardCharsets.UTF_8))); > 789: > 790: assertNotEquals(Background.fill(Color.RED), > root.getBackground()); This is just checking the default state of a stack pane. Would it not be fairer to first set it up in a Scene, with a different stylesheet or no stylesheet, then applying one that uses `:root` and checking the before and after states? modules/javafx.graphics/src/test/java/test/javafx/scene/Parent_pseudoClasses_Test.java line 60: > 58: assertNotPseudoClass(ONLY_CHILD, child1); > 59: assertPseudoClass(ONLY_CHILD, child2); > 60: } How about a case where the last child is removed, and seeing if the first and now only child gets the correct states? I think there currently is definitely a bug where the odd/even may not be reapplied in that case... ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907081969 PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907034950 PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907039143 PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907077704 PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907061458 PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907071898 PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907088577 PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907090798
