Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
FlorianKirmaier wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0dba9a8
8388313: Fix redundant evaluation of css when ContextMenu.show(Node,S…
sandec-bot Jul 15, 2026
f5055e4
8388313: readded accidentally removed line.
sandec-bot Jul 15, 2026
3302b33
8388313: worked in some comments and mixed improvements.
sandec-bot Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

package javafx.scene.control;

import javafx.geometry.Bounds;
import javafx.geometry.NodeOrientation;
import javafx.beans.property.ObjectProperty;
import javafx.beans.property.ObjectPropertyBase;
import javafx.collections.ListChangeListener.Change;
Expand All @@ -44,6 +46,7 @@
import com.sun.javafx.collections.TrackableObservableList;
import com.sun.javafx.tk.Toolkit;
import com.sun.javafx.util.Utils;
import com.sun.javafx.stage.PopupWindowHelper;

/**
* <p>
Expand Down Expand Up @@ -249,10 +252,10 @@ public void show(Node anchor, Side side, double dx, double dy) {
if (anchor == null) return;
if (getItems().size() == 0) return;

PopupWindowHelper.ownerWindow(this).set(anchor.getScene().getWindow());
PopupWindowHelper.ownerNode(this).set(anchor);
PopupWindowHelper.applyStylesheetFromOwner(this, anchor.getScene().getWindow());
Comment on lines +255 to +257

@Maran23 Maran23 Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@FlorianKirmaier FlorianKirmaier Jul 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

getScene().setNodeOrientation(anchor.getEffectiveNodeOrientation());
if (getScene().getStylesheets().isEmpty()) {
getScene().getStylesheets().setAll(anchor.getScene().getStylesheets());
}

HPos hpos = side == Side.LEFT ? HPos.LEFT : side == Side.RIGHT ? HPos.RIGHT : HPos.CENTER;
VPos vpos = side == Side.TOP ? VPos.TOP : side == Side.BOTTOM ? VPos.BOTTOM : VPos.CENTER;
Expand Down
35 changes: 35 additions & 0 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@
import test.com.sun.javafx.scene.control.infrastructure.KeyEventFirer;
import test.com.sun.javafx.scene.control.infrastructure.MouseEventFirer;
import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
import javafx.scene.control.skin.ButtonSkin;
import java.util.concurrent.atomic.AtomicInteger;

public class ContextMenuTest {

Expand Down Expand Up @@ -695,6 +697,32 @@ private ContextMenu createContextMenuAndShowSubMenu() {
assertEquals(anchorBounds.getMinY(), cmBounds.getMinY(), 0.0);
}

@Test public void test_css_skin_counter() {

@Maran23 Maran23 Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testCssNewSkinAppliedByStyleClassChange

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

@FlorianKirmaier FlorianKirmaier Jul 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

anchorBtn.getScene().getStylesheets().add(
getClass().getResource("test_css_skin_counter.css").toExternalForm()

@Maran23 Maran23 Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
}

@FlorianKirmaier FlorianKirmaier Jul 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
anchorBtn.getStyleClass().add("anchor");
AtomicInteger skinCounter = new AtomicInteger(0);
Button button = new Button();
button.skinProperty().subscribe(skin -> {
System.out.println("new Skin: " + skin);
new Exception().printStackTrace();

@Maran23 Maran23 Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like debugging code?

@FlorianKirmaier FlorianKirmaier Jul 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

skinCounter.incrementAndGet();
});
menuItem.setGraphic(button);
ContextMenu cm = createContextMenu(false);
cm.show(anchorBtn, Side.TOP, 0, 0);

Bounds anchorBounds = anchorBtn.localToScreen(anchorBtn.getLayoutBounds());
Node cmNode = cm.getScene().getRoot();
Bounds cmBounds = cm.getScene().getRoot().localToScreen(cmNode.getLayoutBounds());

assertEquals(anchorBounds.getMinX(), cmBounds.getMinX(), 0.0);
assertEquals(anchorBounds.getMinY(), cmBounds.getMaxY(), 0.0);

assertEquals(2, skinCounter.get());
}

Comment thread
FlorianKirmaier marked this conversation as resolved.

@Test public void test_position_withCSS() {
anchorBtn.getScene().getStylesheets().add(
Expand Down Expand Up @@ -775,4 +803,11 @@ private ContextMenu createContextMenuAndShowSubMenu() {
assertEquals(0, padding.getLeft(), 0.0);
anchorBtn.setGraphic(null);
}

public static class ButtonSkin1 extends ButtonSkin {
public ButtonSkin1(Button button) { super(button); }
}
public static class ButtonSkin2 extends ButtonSkin {
public ButtonSkin2(Button button) { super(button); }
}
}
2 changes: 2 additions & 0 deletions ...es/javafx.controls/src/test/resources/test/javafx/scene/control/test_css_skin_counter.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
.button { -fx-skin: "test.javafx.scene.control.ContextMenuTest$ButtonSkin1"; }
.anchor .button { -fx-skin: "test.javafx.scene.control.ContextMenuTest$ButtonSkin2"; }
11 changes: 11 additions & 0 deletions modules/javafx.graphics/src/main/java/com/sun/javafx/stage/PopupWindowHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import com.sun.javafx.util.Utils;
import javafx.collections.ObservableList;
import javafx.beans.property.ReadOnlyObjectWrapper;
import javafx.scene.Node;
import javafx.stage.PopupWindow;
import javafx.stage.Window;
Expand Down Expand Up @@ -67,6 +68,14 @@ public static void applyStylesheetFromOwner(PopupWindow popupWindow, Window owne
popupWindowAccessor.applyStylesheetFromOwner(popupWindow, owner);
}

public static ReadOnlyObjectWrapper<Window> ownerWindow(PopupWindow popupWindow) {
return popupWindowAccessor.ownerWindow(popupWindow);
}

public static ReadOnlyObjectWrapper<Node> ownerNode(PopupWindow popupWindow) {
return popupWindowAccessor.ownerNode(popupWindow);
}

public static ObservableList<Node> getContent(PopupWindow popupWindow) {
return popupWindowAccessor.getContent(popupWindow);
}
Expand All @@ -84,5 +93,7 @@ public interface PopupWindowAccessor {
void doVisibleChanging(Window window, boolean visible);
void doVisibleChanged(Window window, boolean visible);
void applyStylesheetFromOwner(PopupWindow popupWindow, Window owner);
ReadOnlyObjectWrapper<Window> ownerWindow(PopupWindow popupWindow);
ReadOnlyObjectWrapper<Node> ownerNode(PopupWindow popupWindow);
}
}
10 changes: 10 additions & 0 deletions modules/javafx.graphics/src/main/java/javafx/stage/PopupWindow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,16 @@ public ObservableList<Node> getContent(PopupWindow popupWindow) {
public void applyStylesheetFromOwner(PopupWindow popupWindow, Window owner) {
popupWindow.applyStylesheetFromOwner(owner);
}

@Override
public ReadOnlyObjectWrapper<Window> ownerWindow(PopupWindow popupWindow) {
return popupWindow.ownerWindow;
}

@Override
public ReadOnlyObjectWrapper<Node> ownerNode(PopupWindow popupWindow) {
return popupWindow.ownerNode;
}
});
}

Expand Down