8389092: PopupWindow.show() unconditionally overwrites user-set scene stylesheets with the owner's stylesheets by Maran23 · Pull Request #2236 · openjdk/jfx

Maran23 · 2026-07-31T19:59:46Z

PopupWindow will always overwrite its Scene (user agent) stylesheets when an owner was set and show is called.
Code like this:

        final Popup popup = new Popup();
        popup.getScene().setUserAgentStylesheet(popupUserAgentStylesheet);
        popup.getScene().getStylesheets().add(popupStylesheet);

will do nothing, because your added stylesheets will be later overwritten when show is called.
Andy and I were already wondering about this behavior two years ago: #1394 (comment)

I can't see any reason why we should do that. Instead, this PR will only add the stylesheets of the owner if they do not exist already.
Additionally, we will not overwrite the user agent stylesheet if it was already set.

Added tests for all combinations I can think of. This PR also fixes NPEs that can happen when the owner window has no 'root window'.

There is already a test for the Cursor behavior, but there was none that verifies that the Cursor is not overwritten, so added one as well.

jfx/modules/javafx.graphics/src/main/java/javafx/stage/PopupWindow.java

Lines 479 to 481 in 05a7b6d

    
           if (sceneValue.getCursor() == null) { 
        
               sceneValue.setCursor(ownerScene.getCursor()); 
        
           }

Now, we will never overwrite anything that the developer set (before showing). And as we can see above, this was already done this way with the Cursor.

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

Progress

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

Issue

JDK-8389092: PopupWindow.show() unconditionally overwrites user-set scene stylesheets with the owner's stylesheets (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2236

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

Using diff file

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

Using Webrev

Link to Webrev Comment

… stylesheets with the owner's stylesheets

…wice

bridgekeeper · 2026-07-31T20:00:38Z

👋 Welcome back mhanl! 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-31T20:02:29Z

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

openjdk · 2026-07-31T20:04:46Z

The total number of required reviews for this PR has been set to 2 based on the presence of this label: rfr. This can be overridden with the /reviewers command.

mlbridge · 2026-07-31T20:08:22Z

Webrevs

00: Full (c1ef0803)

andy-goryachev-oracle · 2026-07-31T20:29:49Z


    private void showImpl(final Window owner) {
+        Window rootWindow = getRootWindow(owner);
+        if (rootWindow == null) {


not equivalent change: the old code set this.ownerWindow (to null in this case, see L467)

You have a point but I don't see how this could be a problem. I can't imagine a usecase where you want to call show just to reset the owner

I was pointing out that the change is not equivalent - in the old code, a null owner causes this.ownerWindow be set to null, in the new code it does not.

andy-goryachev-oracle · 2026-07-31T20:36:41Z

    private void showImpl(final Window owner) {
+        Window rootWindow = getRootWindow(owner);
+        if (rootWindow == null) {
+            return;


I am just a bit confused here. Referring to old version' line numbers, it looks like we should not have a null value returned from getRootWindow(owner): it's being dereferenced in L477 and also L485.

andy-goryachev-oracle · 2026-07-31T20:37:59Z

    void applyStylesheetFromOwner(Window owner) {
+        // JDK-8116444
+        Window rootWindow = getRootWindow(owner);
+        if (rootWindow == null) {


same thing - is it even possible for the rootWindow to be null?

Yes, as you can see in the tests I wrote.

This is not a question about the tests you wrote.
Sorry, I need to spell my question out. Looking at (old) L502:

final Scene ownerScene = getRootWindow(owner).getScene();

you see that getScene() dereferences the root window, so it can't be null there, I guess, otherwise we had seen an NPE.

andy-goryachev-oracle · 2026-08-04T19:09:55Z

+        List<String> newStylesheets = new ArrayList<>();
+        for (String stylesheet : ownerScene.getStylesheets()) {
+            if (!scene.getStylesheets().contains(stylesheet)) {
+                newStylesheets.add(stylesheet);


can a PopupWindow be reused? if so, the stylesheets might accumulate from the earlier cycles. is this ok? should we keep track of the stylesheets added here and remove them on hide()?

Does not happen as we will check with contains. There is also a test for this scenario.

I suppose I need to spell this out. L527 adds new stylesheets, but there is not removing operation. So if you keep adding stylesheets and not removing the old stylesheets, they just get accumulated.

Think of a scenario when a stylesheet is not sourced from a file/resource, but generated programmatically via a data url.

andy-goryachev-oracle · 2026-08-04T21:17:55Z

-            if (ownerScene.getUserAgentStylesheet() != null) {
-                scene.setUserAgentStylesheet(ownerScene.getUserAgentStylesheet());
+        if (scene.getUserAgentStylesheet() == null && ownerScene.getUserAgentStylesheet() != null) {
+            scene.setUserAgentStylesheet(ownerScene.getUserAgentStylesheet());


there might be another issue:

owner A has the user agent stylesheet A.css

a popup with no stylesheet is shown with the owner A. A.css is shown

the popup gets hidden

either owner A changes the stylesheet to B.css, or the popup is reusing with a different owner

the popup gets shown

since scene.getUserAgentStylesheet() is already A.css, this code does not set the new stylesheet, leaving the popup with a wrong style.

I know reusing the popup (context menu, etc.) is a bad idea, but surprisingly, I saw this happen many times in the past.

AFAIK, there is no way that you can actually change the user agent stylesheet. It is read once and then cached.
See e.g. #525, where a user proposed to change that.

So I think we are actually good here

Here is the case for the PopupTest that works in master and fails with this fix:

@Test public void updateStylesheetFromOwnerStage() { String cssA = toBase64(".root { -fx-fill: green; }"); String cssB = toBase64(".root { -fx-fill: red; }"); scene.setUserAgentStylesheet(cssA); Popup p = new Popup(); p.show(stage); assertEquals(cssA, p.getScene().getUserAgentStylesheet()); p.hide(); scene.setUserAgentStylesheet(cssB); p.show(stage); assertEquals(cssB, p.getScene().getUserAgentStylesheet()); }

andy-goryachev-oracle · 2026-08-10T17:31:54Z

+        assertEquals(List.of(sharedStylesheet), popup.getScene().getStylesheets());
+    }
+
+    private String toBase64(String stylesheet) {


should probably be static...
and since we are using it more than once, maybe we can move it to a new graphics-specific Utils class?

Maran23 added 2 commits July 31, 2026 21:43

8389092: PopupWindow.show() unconditionally overwrites user-set scene…

42ffab8

… stylesheets with the owner's stylesheets

2 more tests to check that we indeed do not add the same stylesheet t…

c1ef080

…wice

openjdk Bot added the rfr Ready for review label Jul 31, 2026

andy-goryachev-oracle reviewed Jul 31, 2026

View reviewed changes

andy-goryachev-oracle reviewed Aug 4, 2026

View reviewed changes

andy-goryachev-oracle reviewed Aug 10, 2026

View reviewed changes

andy-goryachev-oracle mentioned this pull request Aug 10, 2026

8388277: [REDO] Looked-up color fails for -fx-background-color in JavaFX CSS file #2225

Open

4 tasks

	if (sceneValue.getCursor() == null) {
	sceneValue.setCursor(ownerScene.getCursor());
	}

Uh oh!

Conversation

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

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper Bot commented Jul 31, 2026

Uh oh!

openjdk Bot commented Jul 31, 2026

Uh oh!

openjdk Bot commented Jul 31, 2026

Uh oh!

mlbridge Bot commented Jul 31, 2026

Webrevs

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!

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!

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Maran23 commented Jul 31, 2026 •

edited by openjdk Bot

Loading