RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS

[Kevin Rushforth]

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