Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v2]
On Fri, 22 Jan 2021 18:54:35 GMT, Kevin Rushforth wrote: >> 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. > > 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. Thanks for clarifying. I have adapted the documentation accordingly. - PR: https://git.openjdk.java.net/jfx/pull/383
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v2]
On Fri, 22 Jan 2021 19:01:13 GMT, Kevin Rushforth wrote: >> 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. > > 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. Agreed. Additional lines removed. - PR: https://git.openjdk.java.net/jfx/pull/383
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v2]
On Fri, 22 Jan 2021 12:02:13 GMT, Robert Lichtenberger 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
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v2]
On Fri, 22 Jan 2021 12:01:55 GMT, Robert Lichtenberger wrote: >> Oh wow. Further experimentation has shown, that if >> NodeOrientation.RIGHT_TO_LEFT is used on the anchor, then Side.LEFT used to >> make the menu appear on the **right** hand side and Side.RIGHT used to make >> the menu appear on the **left** hand side of the button, which really >> surprises me, since I didn't expect the side parameter to be relative. > > Ok, I just pushed all the changes necessary to keep the old behaviour for > right-to-left node orientation. > If you would rather have the behaviour changed (it doesn't seem very > intuitive to me...) just tell me and I can change the implementation / tests. Good catch. The current behavior is consistent with how RTL works for most things, so we do want to preserve that behavior. - PR: https://git.openjdk.java.net/jfx/pull/383
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v2]
> 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. - Changes: - all: https://git.openjdk.java.net/jfx/pull/383/files - new: https://git.openjdk.java.net/jfx/pull/383/files/48d6453a..67a7da14 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=383=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=383=00-01 Stats: 123 lines in 3 files changed: 87 ins; 29 del; 7 mod Patch: https://git.openjdk.java.net/jfx/pull/383.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/383/head:pull/383 PR: https://git.openjdk.java.net/jfx/pull/383
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v2]
On Fri, 22 Jan 2021 11:02:29 GMT, Robert Lichtenberger
wrote:
>> While trying to come up with a good documentation I've detected a real
>> change in behaviour in connection with the NodeOrientation of the anchor
>> node.
>> Although this has never been documented, when NodeOrientation.RIGHT_TO_LEFT
>> is set, the context menu was right-aligned for Side=TOP and Side=BOTTOM.
>> Since I assume we don't want to change this behaviour I would now document
>> it and adapt my patch accordingly. I've already written a test case like
>> this:
>> @Test public void test_position_withOrientation() throws
>> InterruptedException {
>> ContextMenu cm = createContextMenu(false);
>> anchorBtn.setNodeOrientation(NodeOrientation.RIGHT_TO_LEFT);
>> cm.show(anchorBtn, Side.TOP, 0, 0);
>>
>> Bounds anchorBounds =
>> anchorBtn.localToScreen(anchorBtn.getLayoutBounds());
>> Node cmNode = cm.getScene().getRoot();
>> Bounds cmBounds =
>> cm.getScene().getRoot().localToScreen(cmNode.getLayoutBounds());
>>
>> assertEquals(anchorBounds.getMaxX(), cmBounds.getMaxX(), 0.0);
>> assertEquals(anchorBounds.getMinY(), cmBounds.getMaxY(), 0.0);
>> anchorBtn.setNodeOrientation(NodeOrientation.LEFT_TO_RIGHT);
>> }
>> which passes with the old implementation but (currently) fails with the new
>> implementation.
>
> Oh wow. Further experimentation has shown, that if
> NodeOrientation.RIGHT_TO_LEFT is used on the anchor, then Side.LEFT used to
> make the menu appear on the **right** hand side and Side.RIGHT used to make
> the menu appear on the **left** hand side of the button, which really
> surprises me, since I didn't expect the side parameter to be relative.
Ok, I just pushed all the changes necessary to keep the old behaviour for
right-to-left node orientation.
If you would rather have the behaviour changed (it doesn't seem very intuitive
to me...) just tell me and I can change the implementation / tests.
-
PR: https://git.openjdk.java.net/jfx/pull/383
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v2]
> 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 refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. - Changes: - all: https://git.openjdk.java.net/jfx/pull/381/files - new: https://git.openjdk.java.net/jfx/pull/381/files/48d6453a..d8bb41d1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=381=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=381=00-01 Stats: 144 lines in 2 files changed: 14 ins; 115 del; 15 mod Patch: https://git.openjdk.java.net/jfx/pull/381.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/381/head:pull/381 PR: https://git.openjdk.java.net/jfx/pull/381
