6415065: Submenu is shown on wrong screen in multiple monitor environment #15185
Conversation
|
Hi @stachenov, welcome to this OpenJDK project and thanks for contributing! We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user stachenov" as summary for the issue. If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing |
|
@stachenov The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
|
/covered |
|
Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated! |
| if (position.y + y < screenBounds.y) { // Above the current screen? | ||
| y = screenBounds.y - position.y; // Fit into the screen, relative to our origin. | ||
| } | ||
| if (position.y + y >= screenBounds.y + screenBounds.height) { // Below the current screen? |
There was a problem hiding this comment.
why only the y coordinate is checked? Can we implement it for x as well for symmetry?
There was a problem hiding this comment.
A good idea. I was actually thinking about it after I had submitted this. Even though I've never encountered any issues with the X coordinate, I think it's still possible in theory in some unusual circumstances. Anyway, an extra check definitely won't hurt. Will do.
There was a problem hiding this comment.
Added the X coordinate checks as well. While I was testing it, I noticed that sometimes the screen bounds are already wrong here because the graphics configuration is returned for the wrong display above. It happens, for example, in Ubuntu, when a window spans across two monitors. This seems to be a separate issue, though, and I can't do anything about it here, it's something deeper inside the internals, and likely is platform-dependent.
…ment What happens here is that when the position of a popup menu is calculated, it can expand above or below, depending on the position of the parent menu, the item being expanded and the size of the submenu and screen resolution. If the menu decides to expand above, the position calculation in JMenu.getPopupMenuOrigin may yield a coordinate above the current screen. Later, JPopupMenu.adjustPopupLocationToFitScreen tries to fit the entire menu into the screen. However, it has no idea which screen is correct, as all it has is an (x, y) location. If that location is invalid, it may correct it by fitting it into the screen. However, if it is valid, but located on an incorrect screen, then the whole logic goes awry and the menu is fitted into the wrong screen. In theory, a similar issue is possible if the menu decides to expand below, though it's not easily reproducible as it would require a rather unique combination of factors, such as unusually large Menu.submenuPopupOffsetY. The same thing could also happen to the X location, although it's less likely because the popup menu has to be unusually wide for that. Fix by pre-adjusting the location to fit into the correct screen in JMenu.getPopupMenuOrigin, where the correct screen is still known. The resulting location may still not be final, as the menu's height needs to be taken into account as well, but that's exactly what JPopupMenu.adjustPopupLocationToFitScreen does. Since the coordinate is on the correct screen now, it fits the menu into the same screen, which guarantees it'll be the correct one.
stachenov
force-pushed
the
JDK-6415065-Popup-menu-multiple-monitor-bug
branch
from
878c01b to
3fb94a6
Compare
|
@stachenov Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
OK, sorry about force-pushing, just followed the usual GitHub procedure that I'm used to. Didn't know local specifics. |
bridgekeeper
bot
removed
oca
|
@stachenov This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
Any news on this? The bot warns me that it'll be closed if there's no activity, but I don't understand what I can do here. |
prrace
left a comment
prrace
left a comment
There was a problem hiding this comment.
Looks reasonable to me. Clearly what's seen in the JB tracker is odd ..
|
@stachenov 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: 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 549 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prrace) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
/integrate |
|
@stachenov |
|
/sponsor |
|
Going to push as commit 23ed890.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@avu @stachenov Pushed as commit 23ed890. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
4 participants
Hello!
I'm a member of the UI team in JetBrains IntelliJ department, and we have this bug with popup menus being shown on the wrong monitor in multi-monitor environments:
https://youtrack.jetbrains.com/issue/JBR-5824/Dual-monitor-bug-on-the-context-menu
I managed to track it down to this JDK bug:
https://bugs.openjdk.org/browse/JDK-6415065
I've described the cause and the fix in the commit message, but in short, what happens here is that
JMenu.getPopupMenuOriginsometimes returns coordinates outside (usually above) of the current screen, and laterJPopupMenu.adjustPopupLocationToFitScreenuses those coordinates to fit the entire popup menu into the screen, which goes wrong because at that point it's no longer known which screen the menu was initially invoked on.I've fixed this by making sure the Y coordinate is still within the correct screen when it's returned from
JMenu.getPopupMenuOrigin.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15185/head:pull/15185$ git checkout pull/15185Update a local copy of the PR:
$ git checkout pull/15185$ git pull https://git.openjdk.org/jdk.git pull/15185/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15185View PR using the GUI difftool:
$ git pr show -t 15185Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15185.diff
Webrev
Link to Webrev Comment