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 #383

Closed
wants to merge 14 commits into from

Conversation

effad
Copy link
Collaborator

@effad effad commented Jan 21, 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

Issue

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

Reviewers

Download

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

Robert Lichtenberger and others added 11 commits Oct 18, 2019
By using the collection itself as synchronization lock we achieve behaviour that matches java.util.Collections classes.
Pull in changes from main repo
…in the presence of CSS

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.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 21, 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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 21, 2021

Webrevs

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 21, 2021

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Jan 21, 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 21, 2021

I saw your comment in #381 regarding the removed section of API docs. We can discuss it as part of the review of this PR. I'll take a look and see whether simply removing it is fine or if I can suggest a replacement. It still might need a CSR (I'll let you know), but it would be a simple one to record the doc changes.

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Jan 21, 2021

This approach looks good to me, and it makes it simpler.
I agree that some info about the Side parameter would be appropriate in the JavaDoc (although it is close to trivial and the source is self-explaining)

@johanvos johanvos self-requested a review Jan 21, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

I still need to test this, but the approach looks good. I left a few inline comments.

…in the presence of CSS

Corrections as per Kevin Rushforth's comments.
Also added two more test cases that test right-to-left node orientation.
Fixed the implementation and the API documentation.
Copy link
Member

@kevinrushforth kevinrushforth left a comment

The fix and the tests look good to me. I have one comment on the docs (and I left a minor formatting comment).

…in the presence of CSS

Removed node orientation from API doc.
Removed extra blank lines.
@@ -280,6 +309,7 @@ public void show(Node anchor, double screenX, double screenY) {
if (anchor == null) return;
if (getItems().size() == 0) return;
getScene().setNodeOrientation(anchor.getEffectiveNodeOrientation());
setAnchorLocation(AnchorLocation.CONTENT_TOP_LEFT);
Copy link
Collaborator

@aghaisas aghaisas Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to handle RTL case here?

Copy link
Collaborator Author

@effad effad Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so.
At least we didn't handle the RTL case here until now. The call to setAnchorLocation(AnchorLocation.CONTENT_TOP_LEFT); is only neccessary to "reset" the anchor location in case we called show with a Side before.

…in the presence of CSS

Removed unnecessary imports.
@openjdk
Copy link

@openjdk openjdk bot commented Jan 27, 2021

@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:

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

Reviewed-by: kcr, aghaisas

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 2 new commits pushed to the master branch:

  • c1b14de: 8259718: Remove the Marlin rasterizer (single-precision)
  • 9d06bd3: 8259864: Cleanup unused Prism files

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.

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 (@kevinrushforth, @aghaisas) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jan 27, 2021
@effad
Copy link
Collaborator Author

@effad effad commented Jan 27, 2021

/integrate

@openjdk openjdk bot added the sponsor label Jan 27, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 27, 2021

@effad
Your change (at version ba803ab) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 27, 2021

/sponsor

@openjdk openjdk bot closed this Jan 27, 2021
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels Jan 27, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 27, 2021

@kevinrushforth @effad Since your change was applied there have been 2 commits pushed to the master branch:

  • c1b14de: 8259718: Remove the Marlin rasterizer (single-precision)
  • 9d06bd3: 8259864: Cleanup unused Prism files

Your commit was automatically rebased without conflicts.

Pushed as commit 6c1a0ca.

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

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