On Fri, 22 Jan 2021 12:02:13 GMT, Robert Lichtenberger <rlich...@openjdk.org> wrote:
>> 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. > > Robert Lichtenberger has updated the pull request incrementally with one > additional commit since the last revision: > > 8228363: ContextMenu.show with side=TOP does not work the first time 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. The fix and the tests look good to me. I have one comment on the docs (and I left a minor formatting comment). modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java line 237: > 235: * NodeOrientation.RIGHT_TO_LEFT is set. > 236: * Using NodeOrientation.RIGHT_TO_LEFT will also "mirror" the > meaning of Side.LEFT and > 237: * Side.RIGHT respectively. We don't document the effect of node orientation in other controls or in charts, so I wouldn't want to mention it here. Instead you can document the behavior assuming the default effective orientation of `LEFT_TO_RIGHT` (without mentioning it). You could make the case that we should document more precisely the effect or NodeOrientation, but that would be a large task, and not something I would want to do for an isolated control in the course of a bug fix (and it would require a CSR). The rest of the doc changes look good and don't need a CSR. modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java line 302: > 300: > 301: > 302: Minor: there should only be one blank line here. ------------- PR: https://git.openjdk.java.net/jfx/pull/383
