On Thu, 26 Sep 2024 21:17:55 GMT, John Hendrikx <jhendr...@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
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1582#discussion_r1786476288
