RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS
Kevin Rushforth
kcr at openjdk.java.net
Thu Jan 21 23:28:40 UTC 2021
On Thu, 21 Jan 2021 06:42:19 GMT, Robert Lichtenberger <rlichten at openjdk.org> wrote:
> By using the anchor location facility of PopupWindows we can avoid miscalculation of the
> menu's height entirely.
> This fix also cleans up some documentation issues.
> This fix introduces tests that check the correct positioning (test_position_)
> test_position_withCSS reproduces the problem that is fixed with this patch.
> The other test_position_ cases serve as "proof" that no regressions are introduces.
> They work before and after the fix is introduced.
I still need to test this, but the approach looks good. I left a few inline comments.
modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java line 241:
> 239: * the {@code ContextMenu} such that its top-left (0,0) position would be attached
> 240: * to the top-right position of the anchor.
> 241: * <p>
I think it is worth adding a short replacement for this, explaining how the side parameter and delta values affect the location of the popup.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java line 646:
> 644: @Test public void test_position_showOnScreen() {
> 645: ContextMenu cm = createContextMenu(false);
> 646: cm.show(anchorBtn,100, 100);
Minor: missing space after the `,`
modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java line 686:
> 684: private String createStylesheet() {
> 685: try {
> 686: File f = File.createTempFile("test_position_showOnTopWithCSS", ".css");
There is no need to create a temporary file for this css resource, since its contents are fixed. Instead, you should add `test_position_showOnTopWithCSS.css` to the repo under [`modules/javafx.controls/src/test/resources/test/javafx/scene/control/`](https://github.com/openjdk/jfx/tree/master/modules/javafx.controls/src/test/resources/test/javafx/scene/control). Then you can just do this:
return ContextMenuTest.class.getResource("test_position_showOnTopWithCSS.css").toExternalForm();
modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java line 649:
> 647:
> 648: assertEquals(cm.getAnchorX(), 100, 0.0);
> 649: assertEquals(cm.getAnchorY(), 100, 0.0);
The expected and actual values are backwards (expected should be first).
-------------
PR: https://git.openjdk.java.net/jfx/pull/383
More information about the openjfx-dev
mailing list