8388313: ContextMenu.show(node, side,x, y) evaluates CSS multiple times. by FlorianKirmaier · Pull Request #2215 · openjdk/jfx

FlorianKirmaier · 2026-07-15T13:10:13Z

When ContextMenu.show(node, side, x, y) is executed, the CSS is evaluated multiple times.
Of these 3 times, 2 process a non-final CSS state.
This leads to temporary, unnecessary changes.

This is fixed by properly defining the stylesheets and the ownerWindow/ownerNode properties of the context menu
before applyCss is called. The previous solution was incomplete and resulted in unnecessary CSS calls.

Slightly related to my other PR: #2213
But it is fixed in a way that works independently of it.

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

Integration blocker

⚠️ Title mismatch between PR and JBS for issue JDK-8388313

Warning

⚠️ Found trailing period in issue title for 8388313: ContextMenu.show(node, side,x, y) evaluates CSS multiple times.

Issue

JDK-8388313: ContextMenu.show(node, side,x, y) evaluates CSS multiple times (Bug - P4) ⚠️ Title mismatch between PR and JBS.

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2215

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

Using diff file

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

…ide,x.y) is called

bridgekeeper · 2026-07-15T13:11:34Z

👋 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-15T13:13:18Z

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

kevinrushforth · 2026-07-15T13:49:24Z

This looks like a risky fix that will need very careful review, which can happen some time after tomorrow's RDP1 fork. CSS performance fixes have a fairly high risk of regression, so this is the sort of change that is better earlier in the release cycle.

FlorianKirmaier · 2026-07-15T14:58:40Z

Just noticed that i accidentally removed a line. Readded it.

Conceptional the fix is quite simple.
Do all the changes which influence CSS before requesting the CSS state.

Basically fixes a half implementation which was done before.

Targeting the Release after makes of course sense!

Maran23

some questions

Maran23 · 2026-07-17T11:34:52Z

+            System.out.println("new Skin: " + skin);
+            new Exception().printStackTrace();


looks like debugging code?

Removed them.
But honestly, tests should have debugging code, and be loud about everything thats happening.
Makes it easier to go back into the topic. And the junit-test-report separates the outputs by test anyways.
But thats just my opinion. And it's no where done in the project.

Maran23 · 2026-07-17T11:36:26Z


+    @Test public void test_css_skin_counter() {
+        anchorBtn.getScene().getStylesheets().add(
+                getClass().getResource("test_css_skin_counter.css").toExternalForm()


you could create a data url CSS, then you don't need a separate CSS file.

jfx/modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java

Line 943 in 1718958

scene.getStylesheets().add(toDataURL(css));

jfx/modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java

Lines 968 to 970 in 1718958

private static String toDataURL(String stylesheet) {

return "data:text/plain;base64," + Base64.getEncoder().encodeToString(stylesheet.getBytes(StandardCharsets.UTF_8));

}

Used it, by making the method public.
Tell me, if another way to use it would be prefered.
Removed the css-file.

Maran23 · 2026-07-17T11:38:10Z

        assertEquals(anchorBounds.getMinY(), cmBounds.getMinY(), 0.0);
    }

+    @Test public void test_css_skin_counter() {


testCssNewSkinAppliedByStyleClassChange

Or something like that would be suggestion to better express what we test here

It's now named testCssProcessedOnlyOnce,
And i also updated the test, to check for 1 instead of 2 (subscribe triggered once before the change, not its a real change-listener)

Maran23 · 2026-07-17T11:46:41Z

+        PopupWindowHelper.ownerWindow(this).set(anchor.getScene().getWindow());
+        PopupWindowHelper.ownerNode(this).set(anchor);
+        PopupWindowHelper.applyStylesheetFromOwner(this, anchor.getScene().getWindow());


Those seem to be set already later in doShow -> show -> showImpl.

And also the stylesheet is applied there as well. So I wonder if can instead just remove the stylesheet stuff below and this as well, and let be done later.

No, this is basically the heart of the PR.
We have to set all this before the layout is computed at:

Point2D point = Utils.pointRelativeTo(anchor, prefWidth(-1), prefHeight(-1), hpos, vpos, dx, dy, true);

Which happens before the show.
The double-assignment is a bit odd.
But the other solution would be to split the show() into a "preShow()" which prepares CSS and owners.
Which would be a bigger change.

8388313: Fix redundant evaluation of css when ContextMenu.show(Node,S…

0dba9a8

…ide,x.y) is called

kevinrushforth mentioned this pull request Jul 15, 2026

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

Open

4 tasks

8388313: readded accidentally removed line.

f5055e4

Maran23 reviewed Jul 17, 2026

View reviewed changes

8388313: worked in some comments and mixed improvements.

3302b33

Navigation Menu

Uh oh!

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

8388313: ContextMenu.show(node, side,x, y) evaluates CSS multiple times.#2215
FlorianKirmaier wants to merge 3 commits into
openjdk:masterfrom
FlorianKirmaier:JDK-8388313-context-menu-redundant-css-evaluation

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!

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

		System.out.println("new Skin: " + skin);
		new Exception().printStackTrace();

	private static String toDataURL(String stylesheet) {
	return "data:text/plain;base64," + Base64.getEncoder().encodeToString(stylesheet.getBytes(StandardCharsets.UTF_8));
	}

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!

Progress

Error

Integration blocker

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

FlorianKirmaier commented Jul 15, 2026 •

edited by openjdk Bot

Loading