8261840: Submenus close to screen borders are no longer repositioned #410
Conversation
Reverting to the old way of showing the context menu but with application of CSS prior to calling prefHeight(-1) / prefWidth(-1) to ensure correct size measurement of the menu.
|
👋 Welcome back rlichten! A progress list of the required criteria for merging this PR into |
|
/reviewers 2 |
|
@kevinrushforth |
modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java
Show resolved
Hide resolved
|
This fixes the bug in question, although I see a slight regression in behavior on Windows with 125% pixel scaling (it doesn't reproduce with any other scaling value that I tried). With the original test case for JDK-8228363, and Given that this new bug only happens with 125% scaling, it seems likely that it is a preexisting bug, and this fix merely exposes it. Can you take a look at this? If it is preexisting, then we should file a follow-on bug for this.
|
|
Yes I can try to look into this.
…On 3/12/21 2:59 PM, Kevin Rushforth wrote:
This fixes the bug in question, although I see a slight regression in
behavior on Windows with 125% pixel scaling (it doesn't reproduce with
any other scaling value that I tried). With the original test case for
JDK-8228363 <https://bugs.openjdk.java.net/browse/JDK-8228363>, and
|TOP| as the value of |side|, the initial menu is positioned slightly
lower (by a few pixels) than it should be. See the attached image. It
is correct the second and subsequent times the context menu is opened.
Given that this new bug only happens with 125% scaling, it seems
likely that it is a preexisting bug, and this fix merely exposes it.
Can you take a look at this? If it is preexisting, then we should file
a follow-on bug for this.
ContextMenu-125
<https://user-images.githubusercontent.com/34689748/110947924-9991ce00-82f5-11eb-82d2-6959ef24293f.png>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#410 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABCKAQ667DN5I4VPLF37ACTTDIF23ANCNFSM4YCWEUJQ>.
|
|
@effad have you had a chance to look at the rendering issues with 125% HiDPI scaling? |
Unfortunately no, not yet. I had some troubles setting up the Windows build and a lot of other stuff to do. |
|
I've finally managed to build JavaFX under Windows and tried out MenuShowBug.java from JDK-8228363 under different setups: JavaFX 16: Does not contain any fix for JDK-8228363 and thus will always show the menu at the wrong position when opened the first time. PR-383: This was the version that used AnchorLocation. When using 125% DPI, it works well, but not perfect, i.e. when only one of my two screens is set to 125% DPI, gaps still appear sometimes. However this solution is unacceptable anyway, due to the issue described in JDK-8261840 (for which this PR is the fix). PR-410 (i.e.: this PR): I can reproduce the problem of @kevinrushforth (i.e. gap on first click). Since up to JavaFX 16 the first opening of the menu never worked under any DPI setting, this is not a regression but rather another issue that we need to look into. To be on the safe side, I also checked MenuShowBug without any stylesheets:
|
|
Thank you for your detailed explanation. I filed https://bugs.openjdk.java.net/browse/JDK-8265526 to track the issue of misaligned positioning with 125% scaling. |
|
@effad 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 41 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 (@aghaisas, @kevinrushforth) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
/integrate |
|
/sponsor |
openjdk
bot
added
integrated
|
@aghaisas @effad Since your change was applied there have been 41 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 67828ae. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Reverting to the old way of showing the context menu but with application
of CSS prior to calling prefHeight(-1) / prefWidth(-1) to ensure correct
size measurement of the menu.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/410/head:pull/410$ git checkout pull/410Update a local copy of the PR:
$ git checkout pull/410$ git pull https://git.openjdk.java.net/jfx pull/410/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 410View PR using the GUI difftool:
$ git pr show -t 410Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/410.diff