RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v2]
Kevin Rushforth
kcr at openjdk.java.net
Fri Jan 22 20:01:11 UTC 2021
On Fri, 22 Jan 2021 12:02:13 GMT, Robert Lichtenberger <rlichten at 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
More information about the openjfx-dev
mailing list