Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v4]
On Wed, 27 Jan 2021 12:14:54 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 > > Removed unnecessary imports. Marked as reviewed by aghaisas (Reviewer). - 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 [v4]
On Wed, 27 Jan 2021 12:14:54 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 > > Removed unnecessary imports. Marked as reviewed by kcr (Lead). - 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 [v4]
> 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 Removed unnecessary imports. - Changes: - all: https://git.openjdk.java.net/jfx/pull/383/files - new: https://git.openjdk.java.net/jfx/pull/383/files/b02dcb02..ba803abf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=383=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=383=02-03 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 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 [v3]
On Wed, 27 Jan 2021 11:21:19 GMT, Ajit Ghaisas 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 >> >> Removed node orientation from API doc. >> Removed extra blank lines. > > modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java > line 312: > >> 310: if (getItems().size() == 0) return; >> 311: >> getScene().setNodeOrientation(anchor.getEffectiveNodeOrientation()); >> 312: setAnchorLocation(AnchorLocation.CONTENT_TOP_LEFT); > > Do we need to handle RTL case here? 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. - 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 [v3]
On Wed, 27 Jan 2021 10:41:25 GMT, Ajit Ghaisas 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 >> >> Removed node orientation from API doc. >> Removed extra blank lines. > > modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java > line 49: > >> 47: import static com.sun.javafx.scene.control.ContextMenuContentShim.*; >> 48: >> 49: import java.io.File; > > These 3 imports are unused. Remove them. Good catch. I removed the (now unused) imports. - 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 [v3]
On Mon, 25 Jan 2021 11:40:08 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 > > Removed node orientation from API doc. > Removed extra blank lines. modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java line 49: > 47: import static com.sun.javafx.scene.control.ContextMenuContentShim.*; > 48: > 49: import java.io.File; These 3 imports are unused. Remove them. modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java line 312: > 310: if (getItems().size() == 0) return; > 311: > getScene().setNodeOrientation(anchor.getEffectiveNodeOrientation()); > 312: setAnchorLocation(AnchorLocation.CONTENT_TOP_LEFT); Do we need to handle RTL case 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 [v3]
On Mon, 25 Jan 2021 11:40:08 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 > > Removed node orientation from API doc. > Removed extra blank lines. Marked as reviewed by kcr (Lead). - 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 [v3]
> 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 Removed node orientation from API doc. Removed extra blank lines. - Changes: - all: https://git.openjdk.java.net/jfx/pull/383/files - new: https://git.openjdk.java.net/jfx/pull/383/files/67a7da14..b02dcb02 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=383=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=383=01-02 Stats: 8 lines in 1 file changed: 0 ins; 6 del; 2 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 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
On Fri, 22 Jan 2021 10:53:00 GMT, Robert Lichtenberger
wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java
>> line 241:
>>
>>> 239: * the {@code ContextMenu} such that its top-left (0,0) position
>>> would be attached
>>> 240: * to the top-right position of the anchor.
>>> 241: *
>>
>> I think it is worth adding a short replacement for this, explaining how the
>> side parameter and delta values affect the location of the popup.
>
> 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 suprises me,
since I didn't expect the side parameter to be relative.
-
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
On Thu, 21 Jan 2021 23:07:23 GMT, Kevin Rushforth 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.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java
> line 241:
>
>> 239: * the {@code ContextMenu} such that its top-left (0,0) position
>> would be attached
>> 240: * to the top-right position of the anchor.
>> 241: *
>
> I think it is worth adding a short replacement for this, explaining how the
> side parameter and delta values affect the location of the popup.
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.
-
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
On Thu, 21 Jan 2021 23:15:09 GMT, Kevin Rushforth 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.
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java
> line 686:
>
>> 684: private String createStylesheet() {
>> 685: try {
>> 686: File f =
>> File.createTempFile("test_position_showOnTopWithCSS", ".css");
>
> There is no need to create a temporary file for this css resource, since its
> contents are fixed. Instead, you should add
> `test_position_showOnTopWithCSS.css` to the repo under
> [`modules/javafx.controls/src/test/resources/test/javafx/scene/control/`](https://github.com/openjdk/jfx/tree/master/modules/javafx.controls/src/test/resources/test/javafx/scene/control).
> Then you can just do this:
>
> return
> ContextMenuTest.class.getResource("test_position_showOnTopWithCSS.css").toExternalForm();
Thanks for pointing this out. I took the example from the bug entry (where it
was nice to have everything in one file). But in the test it is of course much
more elegant to use a .css file from the resources.
Adapted the test 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
On Thu, 21 Jan 2021 23:11:44 GMT, Kevin Rushforth 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.
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java
> line 646:
>
>> 644: @Test public void test_position_showOnScreen() {
>> 645: ContextMenu cm = createContextMenu(false);
>> 646: cm.show(anchorBtn,100, 100);
>
> Minor: missing space after the `,`
Space inserted.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java
> line 649:
>
>> 647:
>> 648: assertEquals(cm.getAnchorX(), 100, 0.0);
>> 649: assertEquals(cm.getAnchorY(), 100, 0.0);
>
> The expected and actual values are backwards (expected should be first).
Swapped positions.
-
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
On Thu, 21 Jan 2021 06:42:19 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.
I still need to test this, but the approach looks good. I left a few inline
comments.
modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java
line 241:
> 239: * the {@code ContextMenu} such that its top-left (0,0) position
> would be attached
> 240: * to the top-right position of the anchor.
> 241: *
I think it is worth adding a short replacement for this, explaining how the
side parameter and delta values affect the location of the popup.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java
line 646:
> 644: @Test public void test_position_showOnScreen() {
> 645: ContextMenu cm = createContextMenu(false);
> 646: cm.show(anchorBtn,100, 100);
Minor: missing space after the `,`
modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java
line 686:
> 684: private String createStylesheet() {
> 685: try {
> 686: File f =
> File.createTempFile("test_position_showOnTopWithCSS", ".css");
There is no need to create a temporary file for this css resource, since its
contents are fixed. Instead, you should add
`test_position_showOnTopWithCSS.css` to the repo under
[`modules/javafx.controls/src/test/resources/test/javafx/scene/control/`](https://github.com/openjdk/jfx/tree/master/modules/javafx.controls/src/test/resources/test/javafx/scene/control).
Then you can just do this:
return
ContextMenuTest.class.getResource("test_position_showOnTopWithCSS.css").toExternalForm();
modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java
line 649:
> 647:
> 648: assertEquals(cm.getAnchorX(), 100, 0.0);
> 649: assertEquals(cm.getAnchorY(), 100, 0.0);
The expected and actual values are backwards (expected should be first).
-
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
On Thu, 21 Jan 2021 12:47:01 GMT, Kevin Rushforth 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. > > 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. 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) - 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
On Thu, 21 Jan 2021 06:42:19 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. 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. - 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
On Thu, 21 Jan 2021 06:37:59 GMT, Robert Lichtenberger
wrote:
>> 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.
>
>> 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.
I've just "recreated" this PR as https://github.com/openjdk/jfx/pull/383 which
is based on a separate branch as suggested.
-
PR: https://git.openjdk.java.net/jfx/pull/381
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS
On Wed, 20 Jan 2021 16:49:35 GMT, Kevin Rushforth wrote:
> 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.
-
PR: https://git.openjdk.java.net/jfx/pull/381
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
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS
On Wed, 20 Jan 2021 16:44:14 GMT, Robert Lichtenberger
wrote:
>> 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.
>
>> 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.
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.
-
PR: https://git.openjdk.java.net/jfx/pull/381
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS
On Wed, 20 Jan 2021 16:37:04 GMT, Kevin Rushforth wrote: > 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? - PR: https://git.openjdk.java.net/jfx/pull/381
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS
On Wed, 20 Jan 2021 16:39:45 GMT, Kevin Rushforth wrote: > 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. - PR: https://git.openjdk.java.net/jfx/pull/381
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS
On Wed, 20 Jan 2021 16:37:04 GMT, Kevin Rushforth 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. > > This changes the specification in a way that will require prior discussion,. > It also will need a CSR. 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. - PR: https://git.openjdk.java.net/jfx/pull/381
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS
On Wed, 20 Jan 2021 11:57:04 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. This changes the specification in a way that will require prior discussion,. It also will need a CSR. - PR: https://git.openjdk.java.net/jfx/pull/381
