Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 0 commits into from

Conversation

effad
Copy link
Collaborator

@effad effad commented Jan 20, 2021

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.


Progress

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

Integration blocker

 ⚠️ The change requires a CSR request to be approved.

Issue

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

Download

$ git fetch https://git.openjdk.java.net/jfx pull/381/head:pull/381
$ git checkout pull/381

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 20, 2021

👋 Welcome back rlichten! 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.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 20, 2021

⚠️ @effad a branch with the same name as the source branch for this pull request (master) is present in the target repository. If you eventually integrate this pull request then the branch master in your personal fork will diverge once you sync your personal fork with the upstream repository.

To avoid this situation, create a new branch for your changes and reset the master branch. You can do this by running the following commands in a local repository for your personal fork. Note: you do not have to name the new branch NEW-BRANCH-NAME.

$ git checkout -b NEW-BRANCH-NAME
$ git branch -f master d8bb41d11f432625c6305cbfc8925e613e8e30a0
$ git push -f origin master

Then proceed to create a new pull request with NEW-BRANCH-NAME as the source branch and close this one.

@openjdk openjdk bot added the rfr label Jan 20, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 20, 2021

Webrevs

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 20, 2021

/reviewers 2
/csr

@openjdk
Copy link

@openjdk openjdk bot commented Jan 20, 2021

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

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 20, 2021

This changes the specification in a way that will require prior discussion,. It also will need a CSR.

@openjdk openjdk bot added the csr label Jan 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 20, 2021

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@effad please create a CSR request and add link to it in JDK-8228363. This pull request cannot be integrated until the CSR request is approved.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 20, 2021

I recommend that you follow the instructions in the earlier comment about pushing these changes to a new branch, resetting your master branch, and creating a new PR from your new branch.

@effad
Copy link
Collaborator Author

@effad effad commented Jan 20, 2021

This changes the specification in a way that will require prior discussion,. It also will need a CSR.
My hope is that it really doesn't change the specification in any way. All it should do is fix the bug.
What part of the spec do you think this would change?

@effad
Copy link
Collaborator Author

@effad effad commented Jan 20, 2021

I recommend that you follow the instructions in the earlier comment about pushing these changes to a new branch, resetting your master branch, and creating a new PR from your new branch.

Ah yes, will try to do so tomorrow.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 20, 2021

Regarding the spec change, I was thinking of this section, which you removed:

     * To clarify the purpose of the {@code hpos} and {@code vpos} parameters,
     * consider that they are relative to the anchor node. As such, a {@code hpos}
     * and {@code vpos} of {@code CENTER} would mean that the ContextMenu appears
     * on top of the anchor, with the (0,0) position of the {@code ContextMenu}
     * positioned at (0,0) of the anchor. A {@code hpos} of right would then shift
     * the {@code ContextMenu} such that its top-left (0,0) position would be attached
     * to the top-right position of the anchor.

As you pointed out, the reference to hpos and vpos is incorrect, but unless I'm missing something, it still seems like the concept still needs to be described.

@effad
Copy link
Collaborator Author

@effad effad commented Jan 21, 2021

Regarding the spec change, I was thinking of this section, which you removed:

     * To clarify the purpose of the {@code hpos} and {@code vpos} parameters,
     * consider that they are relative to the anchor node. As such, a {@code hpos}
     * and {@code vpos} of {@code CENTER} would mean that the ContextMenu appears
     * on top of the anchor, with the (0,0) position of the {@code ContextMenu}
     * positioned at (0,0) of the anchor. A {@code hpos} of right would then shift
     * the {@code ContextMenu} such that its top-left (0,0) position would be attached
     * to the top-right position of the anchor.

As you pointed out, the reference to hpos and vpos is incorrect, but unless I'm missing something, it still seems like the concept still needs to be described.

I assume that a very long time ago, there were hpos and vpos parameters for this method. And that really required an explanation, because then you could e.g. specify hpos=RIGHT and vpos=BOTTOM, which would give a very strange position for the context menu (the top left corner of the menu would be in the bottom right corner of the button).
It also was very counter-intuitive that giving CENTER/CENTER for hpos/vpos would put the top left corner of the menu at the top left corner of the button.
However all this confusion was ended when Side was used instead of HPos/VPos. At that very moment all the confusing cases went away. No more BOTTOM/RIGHT or CENTER/CENTER placement.
So I think we can really just remove the (obsolete) explanation of hPos/vPos.

@effad
Copy link
Collaborator Author

@effad effad commented Jan 21, 2021

I've just "recreated" this PR as #383 which is based on a separate branch as suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
csr rfr
2 participants