On Wed, 15 Jul 2026 09:54:42 GMT, Florian Kirmaier <[email protected]> wrote:
> ### Fix > The PR replaces applyCss() with reapplyCSS in the nodeOrientation code of > Scene. > applyCss() reapplied styles eagerly - and didn't rebuild the "style maps" > resulting in wrong css. > reapplyCSS() rematches correctly - and also defers it to the next pulse. > > I've added a unit test to: Node_effectiveOrientation_Css_Test. > > ### Test improvements > Because the whole test class was disabled, I've also investigated which tests > are working - and reenabled the working tests. > This PR also fixes 2 of the previously failing tests in > Node_effectiveOrientation_Css_Test - which are now enabled. > > Which are the following tests: > > Node_effectiveOrientation_Css_Test.test_dir_pseudoClass_functions_on_scene_effective_orientation_not_node > Node_effectiveOrientation_Css_Test.test_SimpleSelector_dir_pseudoClass_with_scene_effective_orientation_rtl > > > > > > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Fix looks good to me and makes sense. This is in-line with other operations. `applyCss` is usually only ever called for newly added children so that they are computed correctly immediately. Although this is very fragile, as there is also: https://github.com/openjdk/jfx/blob/17189589cb82a2cec6397e1e14024593f5ce30c3/modules/javafx.graphics/src/main/java/javafx/scene/Node.java#L9961-L9974 Which sometimes seems to not work correctly, probably because of nested parent. Me and John were wondered about this at one point as well. But that is another story altogether. So the `nodeOrientation` really is the odd one here. I don't see a reason we should call `applyCss` instead of the `reapplyCSS`. Looks good, tested on several applications and all good. Note that you need to fix the warnings / errors that are mentioned in the PR body. This is unrelated to this PR (so should not block it): I just had a look why the other tests fail. It is always the same reason which I think might not even a bug. When I change: Stylesheet stylesheet = new CssParser().parse( ".root:dir(rtl) .rect { -fx-fill: #ff0000; }" + ".root:dir(ltr) .rect { -fx-fill: #00ff00; }" + ".root .rect { -fx-fill: #0000ff; }" ); to: Stylesheet stylesheet = new CssParser().parse( ".root .rect { -fx-fill: #0000ff; }" + ".root .rect:dir(rtl) { -fx-fill: #ff0000; }" + ".root .rect:dir(ltr) { -fx-fill: #00ff00; }" ); all tests are green. In CSS, the order matters. Although the CSS specification documents that the [specifity](https://www.w3.org/TR/selectors-4/#specificity-rules) should be higher with a pseudoclass (so in this case, the order should NOT matter). So JavaFX is not following the CSS specification here, but might be intended (although weird - I would say this is a bug). cc @mstr2, @hjohn Do you have some thoughts on this? modules/javafx.graphics/src/test/java/test/javafx/scene/Node_effectiveOrientation_Css_Test.java line 167: > 165: @Disabled("JDK-8234152") > 166: @Test > 167: public void > test_CompounSelector_dir_pseudoClass_on_child_with_scene_effective_orientation_ltr() > { While here, can we fix the typo in `test_Compoun` -> `test_Compound` here and some other tests aswell modules/javafx.graphics/src/test/java/test/javafx/scene/Node_effectiveOrientation_Css_Test.java line 244: > 242: > 243: @Test > 244: public void testCssUpdates() { Maybe we should name it similar as what you wrote in your description - maybe `testChangeNodeOrientationWillReapplyCss` or `testChangeNodeOrientationWillBatchReapplyCss` ------------- PR Review: https://git.openjdk.org/jfx/pull/2213#pullrequestreview-4722356126 PR Comment: https://git.openjdk.org/jfx/pull/2213#issuecomment-5057059574 PR Review Comment: https://git.openjdk.org/jfx/pull/2213#discussion_r3602882426 PR Review Comment: https://git.openjdk.org/jfx/pull/2213#discussion_r3602888875
