On Fri, 17 Jul 2026 11:46:41 GMT, Marius Hanl <[email protected]> wrote:
>> 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: https://github.com/openjdk/jfx/pull/2213 >> But it is fixed in a way that works independently of it. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java > line 257: > >> 255: >> PopupWindowHelper.ownerWindow(this).set(anchor.getScene().getWindow()); >> 256: PopupWindowHelper.ownerNode(this).set(anchor); >> 257: 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. > modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java > line 700: > >> 698: } >> 699: >> 700: @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) > modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java > line 702: > >> 700: @Test public void test_css_skin_counter() { >> 701: anchorBtn.getScene().getStylesheets().add( >> 702: >> getClass().getResource("test_css_skin_counter.css").toExternalForm() > > you could create a data url CSS, then you don't need a separate CSS file. > > https://github.com/openjdk/jfx/blob/17189589cb82a2cec6397e1e14024593f5ce30c3/modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java#L943 > > https://github.com/openjdk/jfx/blob/17189589cb82a2cec6397e1e14024593f5ce30c3/modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java#L968-L970 Used it, by making the method public. Tell me, if another way to use it would be prefered. Removed the css-file. > modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java > line 709: > >> 707: button.skinProperty().subscribe(skin -> { >> 708: System.out.println("new Skin: " + skin); >> 709: 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. > modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java > line 725: > >> 723: assertEquals(2, skinCounter.get()); >> 724: } >> 725: > > unnecessary newline removed ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2215#discussion_r3636381370 PR Review Comment: https://git.openjdk.org/jfx/pull/2215#discussion_r3636585579 PR Review Comment: https://git.openjdk.org/jfx/pull/2215#discussion_r3636580241 PR Review Comment: https://git.openjdk.org/jfx/pull/2215#discussion_r3636594405 PR Review Comment: https://git.openjdk.org/jfx/pull/2215#discussion_r3636580897
