8269374: Menu inoperable after setting stage to second monitor

[jperedadnr]

On Windows, with two monitors with different DPI settings, if a JavaFX application changes screens (either by dragging or programmatically) there is a resize event to adjust the view to its new platform scale.

Note that there is already a call to View::updateLocation in WinWindow, right after notifyMoveToAnotherScreen and notifyScaleChanged . But this is done too soon, before the resize event. There are other MOVE events processed too (in a long complex chain of recursive calls to WinWindow::setBounds).

As a consequence, as commented in the issue JDK-8269374, the view x/y coordinates are wrongly set using the old X,Y values (from the previous screen) with the new platform scale (from the new screen).

This PR adds an extra call to View::updateLocation` in WinWindow, forcing the view relocation on Windows after every resize event to fix the issue. With the correct location of the scene, the Menus are now perfectly aligned, and the mouse events are correctly processed. It doesn't have any side effect on other platforms.

There's a very small penalty, as this new relocation will be called whenever there is a resize event on Windows, but it simplifies the overhead of detecting when the change of screens is effectively done (including the chain of move/resize) events.

No tests are provided, since these would require a setup of two monitors with different DPI settings. However, the test case in the JBS issue can be used to validate the fix.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8269374: Menu inoperable after setting stage to second monitor

Reviewers

• Kevin Rushforth (@kevinrushforth - Reviewer)
• Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/572/head:pull/572
$ git checkout pull/572

Update a local copy of the PR:
$ git checkout pull/572
$ git pull https://git.openjdk.java.net/jfx pull/572/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 572

View PR using the GUI difftool:
$ git pr show -t 572

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/572.diff

[bridgekeeper]

👋 Welcome back jpereda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 00: Full (80a3895c)

[kevinrushforth]

This seems like a correct fix. I can test it along with a formal review. Even though it is a small change in terms of lines of code, I'd like a second reviewer.

/reviewers 2

[openjdk]

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[kevinrushforth]

Looks good.

[arapte]

I tested the fix with different scaling combinations, the issue reported in JBS does not occur with fix. But noticed a similar issue that occurs only if the primary screen is in landscape mode and external monitor is in portrait mode. This behavior is observed both with and without this change. The behavior may be unrelated to fix, in that it can be addressed separately.
@kevinrushforth request you to take look and call on this.

Steps:

1. Primary screen, 125%, Landscape mode
2. Secondary screen, 100%, Portrait mode
3. Run the program attached to JBS
4. Move the Stage to secondary screen
5. Maximize Stage
6. Keep focus on the Stage and press Windows + Shift + Left key
7. Stage will be moved to primary screen
8. Click on menu, observe that location is not correct. Below is screenshot.

---

A slight different behavior can be observed with a change in step 6
Follow same steps as above except at step 6: Click 'Change screen' button. Below is the screenshot

[kevinrushforth]

I'd be OK handling this as a follow-on bug.

@jperedadnr Can you take at least a quick look and see if you can spot why this case fails? Maybe there is another place where we need to call updateLocation?

[jperedadnr]

I can reproduce the issues mentioned by @arapte.

Even with two landscape screens, if you maximise (or just make it big enough) the app on the 100% monitor, and click the button to move it to the other, the app doesn't move correctly, stays more or less at the middle of the DPI screen (with wrong scale), but menus location and scale are correct:

This issues probably require their own follow-up JBS?

[kevinrushforth]

This issues probably require their own follow-up JBS?

Agreed. Can you file a new bug in JBS?

@arapte Unless you have any concerns, can you review / approve this fix now?

[jperedadnr]

I've filed JDK-8270868, and also investigated the possible root cause of the issue. It turns out there is a reason for the issues @arapte notified, as those happened with a full screen.

[openjdk]

@jperedadnr This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8269374: Menu inoperable after setting stage to second monitor

Reviewed-by: kcr, arapte

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 8 new commits pushed to the master branch:

• 43900e3: 8188027: List/TableCell: must not fire event in startEdit if already editing
• 1814991: 8269639: [macos] Calling stage.setY(0) twice causes wrong popups location
• 8b8cea2: 8269638: Property methods, setters, and getters in printing API should be final
• 1d97808: Merge
• 8501416: 8270246: Deprecate for removal implementation methods in Scene
• 948df32: 8268849: Update to 612.1 version of WebKit
• a34928f: 8269968: [REDO] Bump minimum version of macOS for x64 to 10.12
• 00b353e: 8269967: JavaFX should fail fast on macOS below minimum version

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[jperedadnr]

/integrate

[openjdk]

Going to push as commit c490ddf.
Since your change was applied there have been 25 commits pushed to the master branch:

• 1992bd5: Merge
• 63c3bba: Merge
• 5a2d640: Merge
• 43bae4d: Merge
• 88a8a7c: Merge
• 83155dd: Merge
• e8e647d: Merge
• 489fff4: Merge
• 7cc1d2c: 8263111: Enhance String Conclusions
• bfc8b87: Merge
• ... and 15 more: https://git.openjdk.java.net/jfx/compare/0c98d9608611aa2378b38604e2528935c94e84ae...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jperedadnr Pushed as commit c490ddf.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.