Re: RFR: 8228363 Fix shifts for ContextMenu shown on TOP/LEFT side
Hi,
I am trying to add test-cases in ContextMenuTest.java to test ContextMenu
positioning,
but the test cases fails since it ignores the css padding applied on the
ContextMenu's menu/menu-item via css.
Test-cases:
@Test public void test_position_showOnTop() { // fails
ContextMenu cm = createContextMenu(false);
cm.show(anchorBtn, Side.TOP, 0, 0);
final Bounds anchorBounds =
anchorBtn.localToScreen(anchorBtn.getLayoutBounds());
assertEquals(cm.getAnchorX(), anchorBounds.getMinX(), 0.0);
assertEquals(cm.getAnchorY() + cm.getHeight(), anchorBounds.getMinY(), 0.0);
}
@Test public void test_position_showOnRight() { // passes
ContextMenu cm = createContextMenu(false);
cm.show(anchorBtn, Side.RIGHT, 0, 0);
final Bounds anchorBounds =
anchorBtn.localToScreen(anchorBtn.getLayoutBounds());
assertEquals(cm.getAnchorX(), anchorBounds.getMaxX(), 0.0);
assertEquals(cm.getAnchorY(), anchorBounds.getMinY(), 0.0);
}
I can do the following to make the tests pass, but it doesn't feel write:
@Test public void test_position_showOnTop() {
...
cm.setOnShown(e -> {
final Bounds anchorBounds =
anchorBtn.localToScreen(anchorBtn.getLayoutBounds());
assertEquals(cm.getAnchorX(), anchorBounds.getMinX(), 0.0);
assertEquals(cm.getAnchorY() + cm.getHeight(), anchorBounds.getMinY(),
0.0);
});
}
What would be the correct way to fetch "css applied" height/width of
ContextMenu?
-- Abhinay
From: openjfx-dev on behalf of Abhinay
Agarwal
Sent: Saturday, February 8, 2020 1:15 PM
To: Kevin Rushforth ; openjfx-dev@openjdk.java.net
Subject: Re: RFR: 8228363 Fix shifts for ContextMenu shown on TOP/LEFT side
Hi Kevin,
Yes, this is actually a pre-review mail to know if I am on the right path.
Exposing these properties from ContextMenu can confuse users because the
control has overloaded show() method.
The second show() method doesn't use any of these properties and instead use
screenX and screenY to place the ContextMenu.
Given the circumstances, using Helper class seems the best solution to me. If
someone has a better alternate solution, I am all ears.
-- Abhinay
From: Kevin Rushforth
Sent: Friday, February 7, 2020 10:14 PM
To: Abhinay Agarwal ; openjfx-dev@openjdk.java.net
Subject: Re: RFR: 8228363 Fix shifts for ContextMenu shown on TOP/LEFT side
I presume this is a "pre" review before sending out an actual review
(which the Skara tooling will do automatically when you generate a pull
request)...
My concern with this approach is that it leaks what should be an
implementation detail into a visible property of the control (and
really, using the properties map in this manner is a bit of a
workaround). An alternative would be to use package scope methods and a
Helper class, which is done in many other places. Worth noting is that
if this is information that a custom skin would also need, then some
sort of way to make this visible to the Skin will eventually be needed.
-- Kevin
On 2/7/2020 8:33 AM, Abhinay Agarwal wrote:
> The issue is caused because of a fix which was pushed as a part of
> https://bugs.openjdk.java.net/browse/JDK-8159044
>
> The root cause of the issue is that ContextMenuSkin#performPopupShifts
> doesn't consider the cases where shiftX / shiftY can be negative, just as in
> case of the example provided by Robert in 8228363. Moreover, these shifts are
> only required for Side.TOP and Side.LEFT.
>
> My fix tries to find a solution to this problem using properties map as side,
> dx, dy passed to ContextMenu#show(Node anchor, Side side, double dx, double
> dy) are not exposed by the control and cannot be accessed by Skin. I am not
> sure if it is the best way to do it.
>
> Changes:
> https://github.com/abhinayagarwal/jfx/commit/148b1ba2b0d3c6bc748df24b1b635e964a7b8001
>
> Any feedback on the changes are appreciated.
>
> Best regards,
> Abhinay
Re: RFR: 8228363 Fix shifts for ContextMenu shown on TOP/LEFT side
Hi Kevin, Yes, this is actually a pre-review mail to know if I am on the right path. Exposing these properties from ContextMenu can confuse users because the control has overloaded show() method. The second show() method doesn't use any of these properties and instead use screenX and screenY to place the ContextMenu. Given the circumstances, using Helper class seems the best solution to me. If someone has a better alternate solution, I am all ears. -- Abhinay From: Kevin Rushforth Sent: Friday, February 7, 2020 10:14 PM To: Abhinay Agarwal ; openjfx-dev@openjdk.java.net Subject: Re: RFR: 8228363 Fix shifts for ContextMenu shown on TOP/LEFT side I presume this is a "pre" review before sending out an actual review (which the Skara tooling will do automatically when you generate a pull request)... My concern with this approach is that it leaks what should be an implementation detail into a visible property of the control (and really, using the properties map in this manner is a bit of a workaround). An alternative would be to use package scope methods and a Helper class, which is done in many other places. Worth noting is that if this is information that a custom skin would also need, then some sort of way to make this visible to the Skin will eventually be needed. -- Kevin On 2/7/2020 8:33 AM, Abhinay Agarwal wrote: > The issue is caused because of a fix which was pushed as a part of > https://bugs.openjdk.java.net/browse/JDK-8159044 > > The root cause of the issue is that ContextMenuSkin#performPopupShifts > doesn't consider the cases where shiftX / shiftY can be negative, just as in > case of the example provided by Robert in 8228363. Moreover, these shifts are > only required for Side.TOP and Side.LEFT. > > My fix tries to find a solution to this problem using properties map as side, > dx, dy passed to ContextMenu#show(Node anchor, Side side, double dx, double > dy) are not exposed by the control and cannot be accessed by Skin. I am not > sure if it is the best way to do it. > > Changes: > https://github.com/abhinayagarwal/jfx/commit/148b1ba2b0d3c6bc748df24b1b635e964a7b8001 > > Any feedback on the changes are appreciated. > > Best regards, > Abhinay
Re: RFR: 8228363 Fix shifts for ContextMenu shown on TOP/LEFT side
I presume this is a "pre" review before sending out an actual review (which the Skara tooling will do automatically when you generate a pull request)... My concern with this approach is that it leaks what should be an implementation detail into a visible property of the control (and really, using the properties map in this manner is a bit of a workaround). An alternative would be to use package scope methods and a Helper class, which is done in many other places. Worth noting is that if this is information that a custom skin would also need, then some sort of way to make this visible to the Skin will eventually be needed. -- Kevin On 2/7/2020 8:33 AM, Abhinay Agarwal wrote: The issue is caused because of a fix which was pushed as a part of https://bugs.openjdk.java.net/browse/JDK-8159044 The root cause of the issue is that ContextMenuSkin#performPopupShifts doesn't consider the cases where shiftX / shiftY can be negative, just as in case of the example provided by Robert in 8228363. Moreover, these shifts are only required for Side.TOP and Side.LEFT. My fix tries to find a solution to this problem using properties map as side, dx, dy passed to ContextMenu#show(Node anchor, Side side, double dx, double dy) are not exposed by the control and cannot be accessed by Skin. I am not sure if it is the best way to do it. Changes: https://github.com/abhinayagarwal/jfx/commit/148b1ba2b0d3c6bc748df24b1b635e964a7b8001 Any feedback on the changes are appreciated. Best regards, Abhinay
RFR: 8228363 Fix shifts for ContextMenu shown on TOP/LEFT side
The issue is caused because of a fix which was pushed as a part of https://bugs.openjdk.java.net/browse/JDK-8159044 The root cause of the issue is that ContextMenuSkin#performPopupShifts doesn't consider the cases where shiftX / shiftY can be negative, just as in case of the example provided by Robert in 8228363. Moreover, these shifts are only required for Side.TOP and Side.LEFT. My fix tries to find a solution to this problem using properties map as side, dx, dy passed to ContextMenu#show(Node anchor, Side side, double dx, double dy) are not exposed by the control and cannot be accessed by Skin. I am not sure if it is the best way to do it. Changes: https://github.com/abhinayagarwal/jfx/commit/148b1ba2b0d3c6bc748df24b1b635e964a7b8001 Any feedback on the changes are appreciated. Best regards, Abhinay
