On Fri, 4 Nov 2022 21:14:20 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Dean Wookey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Added changing scene tests for accelerator change listeners
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java
> line 154:
>
>> 152: sceneChangeListener = (scene, oldValue, newValue) -> {
>> 153: if (oldValue != null) {
>> 154:
>> ControlAcceleratorSupport.removeAcceleratorsFromScene(getSkinnable().getItems(),
>> oldValue);
>
> will it handle a case where the menu button gets attached to a different
> scene?
> could you add a second test for this scenario please?
>
> And i wonder if the problem is in ControlAcceleratorSupport rather than here.
> We do have a similar code in Control:380, do we have a problem there?
The code in ControlAcceleratorSupport adds its own scene listeners for each
node added. MenuButtonSkinBase does the same, unlike Control which only calls
it once.
I think we could fix it in ControlAcceleratorSupport alternatively or in
addition to this fix. The code in ControlAcceleratorSupport appears to do
everything this would've done.
I've added the test for changing scenes.
-------------
PR: https://git.openjdk.org/jfx/pull/937
