On Thu, 3 Oct 2024 15:55:55 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> This change modifies `ScrollPaneBehavior` to only consume keys that are
>> targetted at it. As `KeyEvent`s are in almost all cases only intended for
>> the targetted node (as visually that's where the user expects the keyboard
>> input to go, as per normal UI rules) consuming key events that bubble up is
>> simply incorrect. When the `ScrollPane` is focused directly (it has the
>> focused border) then it is fine for it to respond to all kinds of keys.
>>
>> In FX controls normally there is no need to check if a `Control` is focused
>> (although they probably should **all** do this) as they do not have children
>> that could have received the Key Events involved, and Key Events are always
>> sent to the focused Node. When `ScrollPane` was developed this was not
>> taken into account, leading to it consuming keys not intended for it.
>>
>> This fixes several unexpected problems for custom control builders. A
>> custom control normally benefits from standard navigation out of the box
>> (TAB/shift+TAB) and directional keys. However, this breaks down as soon as
>> this custom control is positioned within a `ScrollPane`, which is very
>> surprising behavior and not at all expected. This makes it harder than
>> needed for custom control developers to get the standard navigation for the
>> directional keys, as they would have to specifically capture those keys
>> before they reach the `ScrollPane` and trigger the correct navigation action
>> themselves (for which as of this writing there is no public API).
>>
>> The same goes for all the other keys captured by `ScrollPane` when it does
>> not have focus, although not as critical as the UP/DOWN/LEFT/RIGHT keys.
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ScrollPaneBehavior.java
> line 88:
>
>> 86:
>> 87: new InputMap.KeyMapping(new KeyBinding(HOME), e ->
>> verticalHome(), this::isNotFocused),
>> 88: new InputMap.KeyMapping(new KeyBinding(END), e ->
>> verticalEnd(), this::isNotFocused),
>
> minor: this change creates a bunch of lambdas
> suggestion: declare
>
> Predicate<KeyEvent> isNotFocused = (ev) -> {
> return !getNode().isFocused();
> };
>
> and pass that to each key mapping instead
It's an interesting suggestion, but it is not needed. `javac` will already
deduplicate these.
You can even verify that this is the case. Use `javap` to decompile the class
file with `javap -c <classname>`. In there, `invokedynamic` is used to
represent the lambda's. It looks like this for example:
120: invokedynamic #54, 0 // InvokeDynamic
#1:test:(Lcom/sun/javafx/scene/control/behavior/ScrollPaneBehavior;)Ljava/util/function/Predicate;
Later on, you'll see another:
152: invokedynamic #54, 0 // InvokeDynamic
#1:test:(Lcom/sun/javafx/scene/control/behavior/ScrollPaneBehavior;)Ljava/util/function/Predicate;
What you can see here is that the same constant (# 54) is used to reference the
method. So, there's no need to help the compiler here.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1582#discussion_r1786759278
