On Thu, 21 Jan 2021 06:42:19 GMT, Robert Lichtenberger <rlich...@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
