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. I'm sorry, what is an SCCE? As for the problem, I think I explained this several times now, but I'll attempt it again: When the user uses the keyboard to control an application, there generally is a border or some other indication indicating the control that will be accepting those keyboard commands. In FX, KeyEvents therefore are always send to the control that has the current focus, as that's where the user expects them to go. For any controls that don't have children, you can assume that a KeyEvent that is received is intended for you -- your control is a leaf in the hierarchy, and so if you received the KeyEvent, you must have had the focus. ScrollPane is different. It is also a Control, but is rarely the target of the focus and even more rarely a leaf in a scene. As such, KeyEvents that were not consumed by the actual target Control can bubble up to it. If a Control wishes to use standard navigation provided by Scene, it must let KeyEvents bubble up. If there is no ScrollPane in between anywhere, this works perfectly. For example, take five Buttons, put them in a Scene in an HBox, remove the standard navigation behavior from them, and observe that even without these keys being part of the InputMap that the directional navigation works for these controls. Now, do this same experiment but put a ScrollPane in between (ie. Scene -> ScrollPane -> HBox -> 5 buttons). Observe that now directional navigation no longer works, despite one of the Buttons having the focus. This is because the ScrollPane, even though it is not focused (no :focused pseudo state, no focused border) is responding to bubbled up KeyEvents intended for one of the Buttons. Custom Control authors outside FX are facing this exact problem -- their Control works flawlessly with standard navigation out of the box when put in standard containers like HBox/VBox etc. However, as soon as a ScrollPane is part of the hierarchy, the directional navigation will no longer function. Again, this is because ScrollPane is consuming KeyEvents not targetted at it. There is no good work-around for this problem, as custom control authors don't have access to input maps nor the traversal engine. Their only option is to recreate the same navigation offered by FX by responding themselves to directional keys which is cumbersome. But why are we working around this problem at all? ScrollPane has no business consuming those keys when it is not focused itself. Controls in FX are basically installing navigation keys in their input maps **because** they otherwise stop functioning correctly when a ScrollPane is in the hierarchy. Navigation is not the business of individual controls, unless their very nature requires a different form of navigation (like ToggleButtons). So in short: create a custom control, put two of them in a HBox and observe that you get directional navigation for free. Now put a ScrollPane in that mix, observe that directional navigation no longer works. > Another question - do you think it might make sense to wait until we get the > traversal policy API https://github.com/openjdk/jfx/pull/1555 and instead > install a policy by default which is identical to the current behavior, but > at the same time it can be disabled or changed to a different policy? I really don't think the current ScrollPane behavior has any place in FX, so I don't see a need to offer both the old and the new behavior. I really feel we're going overboard here to avoid potentially breaking something when it is clearly not correct behavior from a UI point of view. You can't make any changes at all anywhere to anything if the criteria is to not cause regressions in undocumented behavior. As for installing custom policies, I'm still not convinced that's the right way to go. I haven't seen any convincing use cases that aren't incredibly niche or are primarily there to work around other more fundamental problems (like this PR that attempts to address one of them). The use cases I did see could be solved by offering navigation methods on `Scene`, and wouldn't be solved by custom policies (ie. if I receive an ActionEvent, and in response to it I want to navigate to the logical next control, how would policies help here?) My take on all of this: if the navigation offered by default in Scene is insufficient for your control, then it is easy enough to look at the scene graph and pick your own next/previous control in response to a KeyEvent. Can FX help here? Sure, offer a few methods that given a Node find a logical/directional next/previous Node, or `null` if not found. There is no need to wrap this all up in a policy: In `Node`: Optional<Node> findFocusable(direction); Or if you want to get really fancy offer a few standard traversal policies that can be accessed statically: public interface TraversalPolicy { Optional<Node> findFocusable(Node from); } public enum DirectionalNavigation implements TraversalPolicy { UP, DOWN, LEFT, RIGHT; @Override public Optional<Node> findFocusable(Node from) { Node next = null; // Calculate next return Optional.of(next); } } public enum LogicalNavigation implements TraversalPolicy { NEXT, PREVIOUS; @Override public Optional<Node> findFocusable(Node from) { Node next = null; // Calculate next return Optional.of(next); } } public enum InsaneCustomNavigation implements TraversalPolicy { RANDOM, DIAGONAL_THREE_DOWN; @Override public Optional<Node> findFocusable(Node from) { Node next = null; // Calculate next return Optional.of(next); } } You can then super easily use these like this: Button button = new Button(); button.setOnAction(e -> LogicalNavigation.NEXT.findFocusable(button).ifPresent(Node::requestFocus)); You wouldn't even need the interface `TraversalPolicy` unless you want to make them configurable for the above code. With the interface you could do this: button.setOnAction(e -> configuredPolicy.findFocusable(button).ifPresent(Node::requestFocus)); ------------- PR Comment: https://git.openjdk.org/jfx/pull/1582#issuecomment-2380363546
