8388304: Scene.setNodeOrientation triggers instant CSS but also doesn't update properly. by FlorianKirmaier · Pull Request #2213 · openjdk/jfx

FlorianKirmaier · 2026-07-15T09:54:42Z

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

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Error

⚠️ Pull request body is missing required line: - [x] I confirm that I make this contribution in accordance with the [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai).

Warning

⚠️ Found trailing period in issue title for 8388304: Scene.setNodeOrientation triggers instant CSS but also doesn't update properly.

Issue

JDK-8388304: Scene.setNodeOrientation triggers instant CSS but also doesn't update properly. (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2213/head:pull/2213
$ git checkout pull/2213

Update a local copy of the PR:
$ git checkout pull/2213
$ git pull https://git.openjdk.org/jfx.git pull/2213/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2213

View PR using the GUI difftool:
$ git pr show -t 2213

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2213.diff

Replace applyCss() with reapplyCSS() in the nodeOrientation invalidation: applyCss() reapplied styles eagerly without rebuilding style maps, leaving :dir() matches stale. reapplyCSS() re-matches and defers to the next pulse.

bridgekeeper · 2026-07-15T10:00:46Z

👋 Welcome back fkirmaier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-07-15T10:02:02Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

kevinrushforth · 2026-07-15T13:53:32Z

This seems like a relatively safe fix on the surface, but we will not have time to adequately review it before the jfx27 RDP1 fork. Since it touches CSS and since you have a somewhat related PR #2215 also out for review, it seems better to target this for jfx28 anyway.

FlorianKirmaier · 2026-07-15T15:11:52Z

I could imagine, that some parts of the code-base rely on the forced CSS computation.
But otherwise i have a good feeling that its correct.

Maran23

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:

jfx/modules/javafx.graphics/src/main/java/javafx/scene/Node.java

Lines 9961 to 9974 in 1718958

    
           // 
        
           // One idiom employed by developers is to, during the layout pass, 
        
           // add or remove nodes from the scene. For example, a ScrollPane 
        
           // might add scroll bars to itself if it determines during layout 
        
           // that it needs them, or a ListView might add cells to itself if 
        
           // it determines that it needs to. In such situations we must 
        
           // apply the CSS immediately and not add it to the scene's queue 
        
           // for deferred action. 
        
           // 
        
           if (getParent() != null && getParent().isPerformingLayout()) { 
        
               NodeHelper.processCSS(this); 
        
           } else { 
        
               notifyParentsOfInvalidatedCSS(); 
        
           }

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.

Maran23 · 2026-07-17T11:48:11Z


+    @Disabled("JDK-8234152")
    @Test
    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

Fixed the 2 Compun - couldn't find another spelling error.

Maran23 · 2026-07-17T11:49:13Z

    }

+    @Test
+    public void testCssUpdates() {


Maybe we should name it similar as what you wrote in your description - maybe testChangeNodeOrientationWillReapplyCss or testChangeNodeOrientationWillBatchReapplyCss

It's now named testChangeNodeOrientationWillReapplyCss.

Maran23 · 2026-07-23T09:58:05Z

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 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?

hjohn · 2026-07-23T23:29:47Z

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 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).

I don't know, specificity is encoded a bit weird (only 4 bits, and doesn't guard overflows) in Match, and is lacking specificity for types (but perhaps FX handles this elsewhere). However, specifically for node orientation, there is some exception being made, so perhaps that has something to do with what you're seeing?

    Match(final Selector selector, Set<PseudoClass> pseudoClasses, int idCount, int styleClassCount) {
        Objects.requireNonNull(selector);
        Objects.requireNonNull(pseudoClasses);

        this.selector = selector;
        this.idCount = idCount;
        this.styleClassCount = styleClassCount;
        this.pseudoClasses = ImmutablePseudoClassSetsCache.of(pseudoClasses);
        int nPseudoClasses = pseudoClasses.size();
        if (selector instanceof SimpleSelector simple) {
            if (simple.getNodeOrientation() != INHERIT) {
                nPseudoClasses += 1;
            }
        }
        specificity = (idCount << 8) | (styleClassCount << 4) | nPseudoClasses;
    }

Maran23 · 2026-07-24T08:10:24Z

    if (selector instanceof SimpleSelector simple) {
        if (simple.getNodeOrientation() != INHERIT) {
            nPseudoClasses += 1;
        }
    }
    specificity = (idCount << 8) | (styleClassCount << 4) | nPseudoClasses;

Good catch! Thats an odd check. Together with:

jfx/modules/javafx.graphics/src/main/java/com/sun/javafx/css/SimpleSelector.java

Lines 243 to 246 in 1e7e7d0

    
           // handle functional pseudo-class :dir() 
        
           // INHERIT applies to both :dir(rtl) and :dir(ltr) 
        
           if (nodeOrientation != INHERIT && styleable instanceof Node) { 
        
               final Node node = (Node)styleable;

this gives us the full picture. Unrelated to this PR, but very weird.

FlorianKirmaier mentioned this pull request Jul 15, 2026

8388313: ContextMenu.show(node, side,x, y) evaluates CSS multiple times. #2215

Open

4 tasks

Maran23 reviewed Jul 17, 2026

View reviewed changes

8388304: fixed spelling and test-name

02eaa57

Navigation Menu

Uh oh!

8388304: Scene.setNodeOrientation triggers instant CSS but also doesn't update properly. - #2213

8388304: Scene.setNodeOrientation triggers instant CSS but also doesn't update properly.#2213
FlorianKirmaier wants to merge 2 commits into
openjdk:masterfrom
FlorianKirmaier:JDK-8388304-fix-scene-nodeorientation

FlorianKirmaier commented Jul 15, 2026 •

edited by openjdk Bot

Loading

Uh oh!

bridgekeeper Bot commented Jul 15, 2026

Uh oh!

openjdk Bot commented Jul 15, 2026

Uh oh!

kevinrushforth commented Jul 15, 2026

Uh oh!

FlorianKirmaier commented Jul 15, 2026

Uh oh!

Maran23 left a comment

Uh oh!

Maran23 Jul 17, 2026

Uh oh!

FlorianKirmaier Jul 23, 2026

Uh oh!

Maran23 Jul 17, 2026

Uh oh!

FlorianKirmaier Jul 23, 2026

Uh oh!

Maran23 commented Jul 23, 2026

Uh oh!

hjohn commented Jul 23, 2026

Uh oh!

Maran23 commented Jul 24, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

	//
	// One idiom employed by developers is to, during the layout pass,
	// add or remove nodes from the scene. For example, a ScrollPane
	// might add scroll bars to itself if it determines during layout
	// that it needs them, or a ListView might add cells to itself if
	// it determines that it needs to. In such situations we must
	// apply the CSS immediately and not add it to the scene's queue
	// for deferred action.
	//
	if (getParent() != null && getParent().isPerformingLayout()) {
	NodeHelper.processCSS(this);
	} else {
	notifyParentsOfInvalidatedCSS();
	}

Uh oh!

Conversation

FlorianKirmaier commented Jul 15, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix

Test improvements

Progress

Error

Warning

Issue

Reviewing

Uh oh!

bridgekeeper Bot commented Jul 15, 2026

Uh oh!

openjdk Bot commented Jul 15, 2026

Uh oh!

kevinrushforth commented Jul 15, 2026

Uh oh!

FlorianKirmaier commented Jul 15, 2026

Uh oh!

Maran23 left a comment

Choose a reason for hiding this comment

Uh oh!

Maran23 Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

FlorianKirmaier Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Maran23 Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

FlorianKirmaier Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Maran23 commented Jul 23, 2026

Uh oh!

hjohn commented Jul 23, 2026

Uh oh!

Maran23 commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

FlorianKirmaier commented Jul 15, 2026 •

edited by openjdk Bot

Loading

Maran23 commented Jul 24, 2026 •

edited

Loading