openjdk
8340852: ScrollPane should not consume navigation keys when it doesn't have direct focus
This change modifies ScrollPaneBehavior to only consume keys that are targetted at it. As KeyEvents 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.
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change requires CSR request JDK-8342655 to be approved
- [ ] Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
Issues
- JDK-8340852: ScrollPane should not consume navigation keys when it doesn't have direct focus (Bug - P4)
- JDK-8342655: ScrollPane should not consume navigation keys when it doesn't have direct focus (CSR)
Reviewers
- Andy Goryachev (@andy-goryachev-oracle - Reviewer) 🔄 Re-review required (review applies to de3c94cc)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1582/head:pull/1582
$ git checkout pull/1582
Update a local copy of the PR:
$ git checkout pull/1582
$ git pull https://git.openjdk.org/jfx.git pull/1582/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1582
View PR using the GUI difftool:
$ git pr show -t 1582
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1582.diff
Webrev
:wave: Welcome back jhendrikx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@hjohn This change now passes all automated pre-integration checks.
ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.
After integration, the commit message for the final commit will be:
8340852: ScrollPane should not consume navigation keys when it doesn't have direct focus
Reviewed-by: angorya, kcr
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.
At the time when this comment was updated there had been 24 new commits pushed to the master branch:
- 076b4018de1a6fd659778b77d66d2478def315a3: 8341514: Add reducedMotion and reducedTransparency preferences
- e2a3074029d94e19332ffb1f1a49e94c5e3e5163: 8342462: TextAreaSkin: remove USE_MULTIPLE_NODES
- 6ac2dd3ee0d175053442fb5de1bd0e3f92175874: 8336031: Create implementation of NSAccessibilityStaticText protocol
- f5b18adfa4151a7760b146a95ecea08b2b407d39: 8337280: Include jdk.jsobject module with JavaFX
- f71c3906d5da83adb69bf55d1e2854b8891dbefe: 8340003: Bump minimum JDK version for JavaFX to JDK 22
- 77482debff0b6e550b451516b4d4d1466895fed8: 8341372: BackgroundPosition, BorderImage, BorderStroke, CornerRadii should be final
- c4b1e1c019c98e97c64df8b11ee2f9635c67256d: 8341686: FX: Update copyright year in docs, readme files to 2025
- 1c86d3b089bec1ade1e9e986ef71ec77cae7b533: 8340850: Wrong bug ID listed as reason for skipping SwingNodePlatformExitCrashTest
- 9c31cb0c696c9ec8bf71038f8f5f53633c457d04: 8340005: Eliminate native access calls from javafx.swing
- 0cafd8011b218162259b81872b1672a1a0649eef: 8341920: Intermittent WebKit build failure on Windows generating PDB files in 619.1
- ... and 14 more: https://git.openjdk.org/jfx/compare/ecab6b6b0eab288eff1e13173a79a2fa4f4aca80...master
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.
When this is ready for review, it will need plenty of testing. Since it is a behavioral change (albeit to fix a bug), we might consider whether it needs a CSR and/or a release note.
/reviewers 2
@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).
I'm thinking still about what would be a good test. I could probably add a JUnit test for the behavior to see if it handles the keys correctly now, seeing that it acts on keys when it has focus, and leaves keys to bubble up when it doesn't.
Webrevs
- 04: Full - Incremental (ff42e927)
- 03: Full - Incremental (4ffc34ac)
- 02: Full - Incremental (38d29e20)
- 01: Full - Incremental (de3c94cc)
- 00: Full (575d015d)
Would it be possible to provide an SCCE that clearly illustrates the problem? This can be a base for a unit test in the PR.
Another question - do you think it might make sense to wait until we get the traversal policy API #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'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));
SCCE = Self-contained code example, or "complete code sample" in https://wiki.openjdk.org/display/OpenJFX/Submitting+a+Bug+Report
I am not entirely convinced that the proposed solution addresses compatibility concerns, unless this can be demonstrated. I would imagine the idea behind SP handling these keys might have been a desire to be able to scroll the content in the view without changing the currently focused node within possibly?
Alternatively, should we use a different set of key bindings instead of simple arrow keys, like for instance alt-ctrl-LEFT/RIGHT/UP/DOWN (option-command-LEFT/RIGHT/UP/DOWN) key bindings, similarly to #1393 ?
edit: thanks for the explanations!
I do not see much behavioral difference between controls inside a regular VBox and VBox inside a ScrollPane
https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/research/FocusPolicyResearch.java
Am I doing things wrong?
I do not see much behavioral difference between controls inside a regular VBox and VBox inside a ScrollPane
https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/research/FocusPolicyResearch.java
Am I doing things wrong?
Yeah, as I explained, FX provided controls, like ToggleButton will capture navigation keys before they bubble up to ScrollPane. See here in ToggleButtonBehavior:
new KeyMapping(RIGHT, e -> traverse(e, "ToggleNext-Right")),
new KeyMapping(LEFT, e -> traverse(e, "TogglePrevious-Left")),
new KeyMapping(DOWN, e -> traverse(e, "ToggleNext-Down")),
new KeyMapping(UP, e -> traverse(e, "TogglePrevious-Up"))
So, with FX provided controls, this issue is being worked around. The controls themselves handle navigation, and so ScrollPane won't interfere. ToggleButton however is not a good example, as it has special needs for its navigation (which I hope are documented) and so is correct to override how navigation works.
Using a regular Button instead for your tests would be better, however this also won't work as FX provided controls are all providing their own navigation to avoid letting events bubble up to ScrollPane.
To make a real comparison, you need to either:
-
Remove the navigation behavior from one of the FX controls, ie. in
ButtonBehavior, remove the line:// add focus traversal mappings addDefaultMapping(buttonInputMap, FocusTraversalInputMap.getFocusTraversalMappings()); -
Set a different skin on
Button(which will also kill its standard behavior).
I'll provide an example.
edit: Example posted below.
SCCE = Self-contained code example, or "complete code sample" in https://wiki.openjdk.org/display/OpenJFX/Submitting+a+Bug+Report
Ah, thank you, I hadn't seen this before.
I am not entirely convinced that the proposed solution addresses compatibility concerns, unless this can be demonstrated. I would imagine the idea behind SP handling these keys might have been a desire to be able to scroll the content in the view without changing the currently focused node within possibly?
That would be counter to how all scrollable views in almost all UI's work. Generally, a ScrollPane only scrolls when it has the focus itself. As an example, in Eclipse, use the Package Explorer and ensure it has a horizonal scroll bar. Now use the mouse to scroll to the right. Now press the left arrow key to go "up the tree" and close nodes. You'll notice that when you are at a root item, and pressing left again will not "bubble" up to the ScrollPane and it starts scrolling left.
At the risk of repeating myself, this is not a matter of what the original programmers may have thought (it is highly likely an oversight, considering how bubbling works and how you don't need to worry about this in "normal" controls), but what people expect from UI's. The focused component deals with key presses, and nothing else does. The only exceptions to these are well understood:
- Menu Hotkeys (usually with a specific kind of menu modifier key as it would otherwise be very confusing)
- Underscored keys to change focus (also only with a modifier, and made visual to the user)
- Navigation
Alternatively, should we use a different set of key bindings instead of simple arrow keys, like for instance alt-ctrl-LEFT/RIGHT/UP/DOWN (option-command-LEFT/RIGHT/UP/DOWN) key bindings, similarly to #1393 ?
That's certainly possible, if that's considered normal for such a control on a given platform. AFAIK, FX is still doing its best to emulate how controls work on the platforms it supports (as evidenced by specific mac/linux/windows bindings in many behaviors).
On Windows however, I'm not aware of any such bindings. I've also looked into the Mac platform, and I could not find any combinations that would make the container scroll while another control within it has focus (like an edit box for example).
Edit: I see that this PR for FX already went ahead and added special keys for these, which are not normally present on the platforms it runs on...
Here is a minimal working example that demonstrates the issue when people create a custom control.
Note that the standard controls and the custom control all have directional navigation working, except in the case where the custom control is embedded inside a ScrollPane. This creates a big burden on custom control developers as to make their control work when used in a scroll pane, they now need to implement their own navigation...
package app;
import javafx.application.Application;
import javafx.scene.Node;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.control.ScrollPane;
import javafx.scene.control.Skin;
import javafx.scene.layout.GridPane;
import javafx.scene.layout.HBox;
import javafx.scene.layout.StackPane;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;
public class App extends Application {
@Override
public void start(Stage primaryStage) {
GridPane gp = new GridPane(10, 10);
gp.add(new VBox(new Label("Standard Buttons in normal container"), new HBox(5, new Button("A"), new Button("B"), new Button("C"))), 0, 0);
gp.add(new VBox(new Label("Standard Buttons in ScrollPane"), new ScrollPane(new HBox(5, new Button("A"), new Button("B"), new Button("C")))), 1, 0);
gp.add(new VBox(new Label("Custom Buttons in normal container"), new HBox(5, new CustomButton("A"), new CustomButton("B"), new CustomButton("C"))), 0, 1);
gp.add(new VBox(new Label("Custom Buttons in ScrollPane"), new ScrollPane(new HBox(5, new CustomButton("A"), new CustomButton("B"), new CustomButton("C")))), 1, 1);
Scene scene = new Scene(gp);
primaryStage.setScene(scene);
primaryStage.show();
}
static class CustomButton extends Button {
CustomButton(String title) {
super(title);
setSkin(new CustomButtonSkin(this));
}
}
static class CustomButtonSkin implements Skin<Button> {
private final Button control;
private final StackPane container;
public CustomButtonSkin(Button button) {
this.control = button;
this.container = new StackPane();
this.container.getChildren().add(new Label(button.getText()));
}
@Override
public Button getSkinnable() {
return control;
}
@Override
public Node getNode() {
return container;
}
@Override
public void dispose() {
container.getChildren().clear();
}
}
}
Here is a minimal working example
Very helpful, thank you.
I would say the use of arrow keys to traverse focus in this case looks to me to be application-specific. In other words, this may or may not be the desired behavior.
In the context of a simple dialog which only has buttons - yes, it is the desired behavior. In the case of a more complex UI where text input components are present - probably not, but it may depend on the application requirements.
In your example, the navigation with TAB / shift+TAB is there, and it works correctly in all four panes. So it's not like the functionality is severely impeded.
What I am saying is that what we currently have is probably ok and JDK-8340852 may not be needed.
I would like to get feedback from a wider audience, if possible.
A cursory search in JBS showed no complaints about ScrollPane consuming the events.
There is a bunch of issues related to focus traversal and/or arrow keys though:
JDK-8087583 JDK-8091294 JDK-8091294 JDK-8091825
Also: swing apparently does not use arrow keys in a similar situation:
https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/research/JScrollPane_Buttons.java
Here is a minimal working example
Very helpful, thank you.
I would say the use of arrow keys to traverse focus in this case looks to me to be application-specific. In other words, this may or may not be the desired behavior.
In the context of a simple dialog which only has buttons - yes, it
isthe desired behavior. In the case of a more complex UI where text input components are present - probably not, but it may depend on the application requirements.
I am sorry, you're saying that you think it is okay that setting a Skin suddenly makes your control malfunction, but only if it has a ScrollPane somewhere in its hierarchy? Where is the logic exactly? Should it not be either:
- All navigation stops working for custom controls, regardless of placement
- Navigation keeps working for custom controls, regardless of placement
... this in between stance is baffling me, and seems to be just defending the status quo in favor of a work-around solution that is not addressing core problems.
The fact that I used buttons has no bearing on this. Other controls will have the exact same problems, if they let directional keys bubble up. For text input controls this is usually not the case as they have other purposes for those keys, but that may change with read only status, or if they want to say allow scrolling from one text component to the next.
In your example, the navigation with TAB / shift+TAB is there, and it works correctly in all four panes. So it's not like the functionality is severely impeded.
Yes, I never claimed tab didn't work. I only claimed that FX is duplicating navigation everywhere in its own controls, just so users wouldn't have to deal with ScrollPane affecting navigation keys.
It sounds like you would be okay with having FX controls behave in the same way, then at least it would be consistent. So let's remove directional navigation from all FX controls, and only have that bubble up to Scene when there is no ScrollPane in between. Then custom controls and FX controls would be on the same footing.
Or, alternatively, let's remove default navigation from Scene and make navigation not a global concern (as it should really be) but the business of each and every control built..
What I am saying is that what we currently have is probably ok and JDK-8340852 may not be needed.
I'm unable to follow your reasoning. If you could explain in more detail why you think the current system should:
a) Have navigation decentralized in every control b) Should lose only half of the standard navigation keys (not all, or none) when a Skin is assigned c) Custom skinned controls have different navigation when placed in ScrollPane while FX controls don't
I tend to think John is right and that the current behavior of ScrollPane is a bug. In general, dropping a control into a ScrollPane shouldn't change aspects of the behavior that aren't directly related to the functioning of the ScrollPane. The fact that JavaFX's controls have to install their own keyboard handler to get what would otherwise be the default behavior underscores this.
I'll admit that I haven't looked at this in enough detail to be sure, but I don't see any good reason for a non-focused ScrollPane to consume keyboard events that bubble up to it.
I agree that the current ScrollPane implementation is wrong. Traversal is an accessibility issue so it’s imperative that we don’t block the traversal keys. We can’t expect custom controls to work around the ScrollPane's faulty behavior.
I don’t agree with the idea that a ScrollPane should only process key events when it has the focus largely because I can’t think of an instance where a ScrollPane receives focus. Focus either lies on the node that owns the ScrollPane (like a TableView) or one of the nodes inside the ScrollPane but not on the ScrollPane itself.
In any case there are instances in any UI toolkit where key event handling is split between a control and non-focused containers further up the tree (for example, I just verified that on macOS the NSScrollView implements PgUp and PgDn but never has focus). I haven’t dug into JavaFX controls in depth but I would be surprised if there weren’t similar instances. It’s a pretty common occurrence.
If we remove the existing PgUp, PgDn, and Home behavior folks will notice and accessibility will take a hit. I’m not entirely sure anyone will notice if we remove the arrow key behavior since the other Controls work so hard to hide them from the ScrollPane to begin with. I do agree that these bindings need to be removed OR completely re-implemented so they don’t block traversal (e.g. they could try to traverse and scroll if traversal isn't possible).
TL;DR Don’t try to filter events based on whether the ScrollPane has focus. Remove the ScrollPane bindings that block the traversal keys. Leave the rest.
I can’t think of an instance where a ScrollPane receives focus.
when there is a non-interactive content inside (such as image or a label). they you do want to handle arrow keys to scroll.
Focus either lies on the node that owns the ScrollPane (like a TableView) or one of the nodes inside the ScrollPane but not on the ScrollPane itself.
this seems to be incorrect: unless you set mouseTransparent, ScrollPane can be focused with a mouse click, even when focusTraversable is false.
Focus either lies on the node that owns the ScrollPane (like a TableView) or one of the nodes inside the ScrollPane but not on the ScrollPane itself.
this seems to be incorrect: unless you set
mouseTransparent, ScrollPane can be focused with a mouse click, even whenfocusTraversableis false.
I didn't say that a ScrollPane couldn't take focus, just that it normally doesn't. And if focus lies with a node inside the ScrollPane we can't expect a user to move focus to the ScrollPane to get PgUp and PgDn working.
I can’t think of an instance where a ScrollPane receives focus.
when there is a non-interactive content inside (such as image or a label). they you do want to handle arrow keys to scroll.
Unless that would make the ScrollPane a traversal dead-end.
It's a conundrum. There are cases where the arrows should scroll particularly for top-level content (I can use them to scroll through a web page in Safari). But from what I can tell this is not the default behavior in the macOS toolkit probably because it interferes with traversal.
Whatever behavior we want the way it's implemented in JavaFX at the moment is wrong.
BTW, I'm not optimistic that we can find some combination of modifiers + arrow keys to use. The behavior of Shift is already defined and many of the other modifier + arrow combinations are used by one OS or another.
you're saying that you think it is okay that setting a Skin suddenly makes your control malfunction
No, I am not saying that.
I played a bit more with your example. Your CustomButtonSkin is weird as it does not place the original button in the scene graph, not sure if that's important.
I've added a number of custom components to the example
static class L extends Label {
public L(String s) {
super(s);
setFocusTraversable(true);
setMouseTransparent(false);
}
}
together with monitoring the scene's focusOwnerProperty (don't want to mess with CSS).
These "L" components do traverse with the arrow keys and your fix with and without the ScrollPane. So perhaps I am wrong and we do want this change.
A couple of notes: the existing example, at least when considering its top two panels with the regular buttons, exhibits reduced accessibility when the scroll pane shows a scroll bar. One can still move focus and the scroll bar does scroll the focused component to view, but one cannot induce it to scroll (for example, if one adds a long label after the last button).
One can try making the scroll pane itself focusTraversable, but then the traversal becomes a bit weird. Perhaps it's a case for custom traversal policy.
Whatever behavior we want the way it's implemented in JavaFX at the moment is wrong.
I changed my mind and I agree with you and John that this change makes sense.
BTW, I'm not optimistic that we can find some combination of modifiers + arrow keys to use. The behavior of Shift is already defined and many of the other modifier + arrow combinations are used by one OS or another.
There was a recent change made to a number of scrollable views (ListView, TreeView, TableView, TreeTableView) to add alt-ctrl-LEFT/RIGHT/UP/DOWN (option-command-LEFT/RIGHT/UP/DOWN) key bindings just for that.
Perhaps we could apply the same idea to the ScrollPane.
just in case, my variant: https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/research/ScrollPane_ConsumeNavEvents2.java
I agree that the current ScrollPane implementation is wrong. Traversal is an accessibility issue so it’s imperative that we don’t block the traversal keys. We can’t expect custom controls to work around the ScrollPane's faulty behavior.
I don’t agree with the idea that a ScrollPane should only process key events when it has the focus largely because I can’t think of an instance where a ScrollPane receives focus. Focus either lies on the node that owns the ScrollPane (like a TableView) or one of the nodes inside the ScrollPane but not on the ScrollPane itself.
ScrollPane however has no idea how to behave correctly to process those keys. ScrollPanes are so universal that processing keys like Home/End, PgUp/PgDown cannot be given a proper meaning without knowing what they contain.
Examples:
-
A ScrollPane contains a street map, currently centered on a location that's being tracked.
- Press PgUp -- what do we do? Moving the vertical bar "one 'page' up" would leave you without seeing the location you'd want to see. Zooming out may make more sense. Doing nothing may also make more sense, yet, this key cannot be disabled...
- Press Home or End... where do we go? Home probably should center on the location being tracked. End has no purpose whatsoever. Scrolling to the top makes zero sense. Should this key scroll to just the top, or top left is another question you can ask... what if the map is infinitely large? Where do you scroll to then?
-
A ScrollPane contains a bunch of focusable control (like a variable amount of input fields and buttons). Let's say we've scrolled down a bit and a Button is focused. Now press pgup/pgdown/home or end. End result: button remains focused, but is now no longer visible in the pane.
-
A ScrollPane contains a rich text editable area, like Java code.
- Press PgUp/Down: ScrollPane has no notion of lines or units, nor does it know how large the font is that is being used. It scrolls but ends up displaying the top line only half instead of showing only discrete lines.
-
A ScrollPane contains a 3D rotatable representation of a building. What should the pgup/pgdown/home/end keys do?
-
A ScrollPane is nested within another ScrollPane. What should pressing Home do? First scroll the inner scroll pane to the top, then on a 2nd press the outer ScrollPane? Scroll both immediately to their Home position? Neither happens currently, and it seems unlikely that a ScrollPane abstraction could possibly know what the correct thing to do is here without knowing anything about its contents.
-
A ScrollPane contains actual pages (zoomed out, like in say Word).
- PgUp/PgDown will not be aware of this and scroll up/down a self determined amount. This will not end nicely at a page boundary... you could "configure" this on the ScrollPane, but it would be near impossible to get right as different units are used, nor could it handle something like larger and smaller pages (or portrait/landscape pages).
What the above examples show primarily is that what a ScrollPane should do is highly dependent on its content. Applications aiming to be accessible should therefore define their own handling for accessible keys. Standard View controls that incorporate a scrollable area should handle such keys themselves (and they do), here's an excerpt from ListViewBehavior:
new KeyMapping(new KeyBinding(HOME), e -> selectFirstRow(), isInEditableComboBox),
new KeyMapping(new KeyBinding(END), e -> selectLastRow(), isInEditableComboBox),
new KeyMapping(new KeyBinding(HOME).shift(), e -> selectAllToFirstRow(), isInComboBox),
new KeyMapping(new KeyBinding(END).shift(), e -> selectAllToLastRow(), isInComboBox),
new KeyMapping(new KeyBinding(PAGE_UP).shift(), e -> selectAllPageUp()),
new KeyMapping(new KeyBinding(PAGE_DOWN).shift(), e -> selectAllPageDown()),
I'm afraid that the current key handling 'behavior' of ScrollPane has been added with a very narrow point of view that it would only be used to display something like a list, that has sensible home and end locations and units suitable to paging. Even when used with something that may look like a list, ScrollPane has no notion of "rows" or "pages" and so will just scroll some fixed (although configurable) amount.
In any case there are instances in any UI toolkit where key event handling is split between a control and non-focused containers further up the tree (for example, I just verified that on macOS the NSScrollView implements PgUp and PgDn but never has focus). I haven’t dug into JavaFX controls in depth but I would be surprised if there weren’t similar instances. It’s a pretty common occurrence.
I'd be interested in how you verified this. AFAIK, contained controls will handle paging (like NSTextView).
If we remove the existing PgUp, PgDn, and Home behavior folks will notice and accessibility will take a hit. I’m not entirely sure anyone will notice if we remove the arrow key behavior since the other Controls work so hard to hide them from the ScrollPane to begin with. I do agree that these bindings need to be removed OR completely re-implemented so they don’t block traversal (e.g. they could try to traverse and scroll if traversal isn't possible).
Trying to traverse, then scroll if you can't would just be annoying. You'd press Up to go the previous control (perhaps expecting it to "wrap around" or stop at the first control if holding the key), but there isn't any and instead the system does something else which you didn't ask for (scrolls up a bit). Assigning such double meanings to keys without a visual clue what will happen will only confuse users. This is why we have the focus indicator, and that's why a ScrollPane should not be acting upon keys by default unless it visually has the focus.
TL;DR Don’t try to filter events based on whether the ScrollPane has focus. Remove the ScrollPane bindings that block the traversal keys. Leave the rest.
That could be a compromise for now.
However, the larger point still stands: As shown in the examples, I doubt that a ScrollPane, being a very low level component, could ever sensibly implement the keys that it currently does without knowing what it is displaying. Again, the View controls in FX don't rely on ScrollPane for Home/End/PgUp/PgDown either. Nobody should rely on these to be honest. If your situation requires these keys to do something, then you should add these yourself (where it makes sense, this could be at the ScrollPane level, or on any focusable controls within the pane).
I wish I could provide a "one size fits all" key handling for ScrollPane, but I don't think that's realistic. At least with this PR, we're making it possible to provide your own implementation that may make more sense, as you can now actually capture those keys when they arrive at a ScrollPane without the default handler consuming them first (as with this PR it will only consume them when it has focus).
Currently, your only other option to "fix" this is to create a ScrollPane subclass and completely reimplement its Skin and, as you can't copy its behavior since those are private, you then won't get the default event handler that steals your keys.
Here's an example illustrating one of the problems with a ScrollPane's default scroll behavior. A window opens with 20 buttons in a ScrollPane. Make the Window small enough. Then Scroll halfway. Select a Button. Now press keys like Home/End and see how the focused control disappears from view. More sensible actions would be: pgup/pgdown focus a button 5 down or 5 up. Home/End: focus first, last button.
package app;
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.control.ScrollPane;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;
public class App2 extends Application {
@Override
public void start(Stage primaryStage) {
VBox vbox = new VBox(new Label("Standard Buttons in ScrollPane"), new ScrollPane(
new VBox(5, createButtons())
));
Scene scene = new Scene(vbox);
primaryStage.setScene(scene);
primaryStage.show();
}
Button[] createButtons() {
Button[] buttons = new Button[20];
for(int i = 0; i < 20; i++) {
buttons[i] = new Button("Button " + (i + 1));
}
return buttons;
}
}
Focus either lies on the node that owns the ScrollPane (like a TableView) or one of the nodes inside the ScrollPane but not on the ScrollPane itself.
this seems to be incorrect: unless you set
mouseTransparent, ScrollPane can be focused with a mouse click, even whenfocusTraversableis false.I didn't say that a ScrollPane couldn't take focus, just that it normally doesn't. And if focus lies with a node inside the ScrollPane we can't expect a user to move focus to the ScrollPane to get PgUp and PgDn working.
I can’t think of an instance where a ScrollPane receives focus.
when there is a non-interactive content inside (such as image or a label). they you do want to handle arrow keys to scroll.
Unless that would make the ScrollPane a traversal dead-end.
It's a conundrum. There are cases where the arrows should scroll particularly for top-level content (I can use them to scroll through a web page in Safari). But from what I can tell this is not the default behavior in the macOS toolkit probably because it interferes with traversal.
It makes sense to not make something that doesn't apply in all situations the default behavior. A browser is mostly a read-only affair, and consuming directional keys for scrolling makes a lot of sense. They don't allow directional navigation for focusable controls, except in very specific cases (like an embedded toolbar) so they are not dealing with this conundrum at all.
The toolkit browsers are built on however do allow directional navigation (their toolbars and other native controls support it), but it was a choice to not to do this on the web page itself (the nested application). It was also a choice there to not be able to escape the nested application easily (when tabbing through things on the web page, you wouldn't want to accidentally tab to the back button or address bar, that would break the immersion of the nested application). Instead browsers provide special short cuts to "escape" the web page (F6 to focus the address bar for example).
FX is similar to most other toolkits. It provides directional navigation as that makes sense for productivity applications. It however sets up its scroll pane's default behavior to act like a web browser would. It is taking away the choice here, as you can't disable this easily without making a new skin for ScrollPane. It would have been better to not set such an opiniated default that can't be overridden easily. In the opposite situation, users can easily install handlers to make the scroll pane act the way they want (and deal with the consequences of their actions by consuming certain keys before they bubble up). So to sum up there are two options:
-
ScrollPane aggressively consumes navigational and other keys [current]
- To prevent this, must create a new skin for ScrollPane to get rid of its default behavior; possibly this can be improved with a behavior or input map proposal, but the main point stands: why have such an opiniated default that doesn't apply in all situations?
- Or alternatively: have all embedded controls handle such keys before they reach the scroll pane (what FX does with the controls it provides in order to get the expected result of navigation before scrolling).
-
ScrollPane by default remains impassive when not focused [proposed]
- To get the existing behavior, just install an event handler on ScrollPane (a choice the user makes that will block directional navigation in favor of scrolling)
- If you want to allow directional navigation first, then the handler should be installed on Scene. Any left over keys can then be converted into actions for a specific scroll pane (probably a top level one, browser style)
- If you want to have exact control over the scrolls a scroll pane should do, then consider handling those at the embedded control that is aware of how its content is best represented (like ListView, TableView) -- this is how pgup/pgdown/home/end should be handled -- a scroll pane is not smart enough to make such keys behave sensibly in all situations
BTW, I'm not optimistic that we can find some combination of modifiers + arrow keys to use. The behavior of Shift is already defined and many of the other modifier + arrow combinations are used by one OS or another.
Yeah, I think we definitely should not do this as part of this problem. A scroll pane is too generic; it makes much more sense to leave this up to the specific implementation instead (ie. TableView, ListView). There is little sense to provide something out of the box that for any realistic control probably needs to be overridden to do what the user expects.
I played a bit more with your example. Your CustomButtonSkin is weird as it does not place the original button in the scene graph, not sure if that's important.
I think there is a misunderstanding. The CustomButton is part of the scene graph (it's added by the user). It is after all the container in which the Skin is placing the controls that give this custom control its appearance. The easiest way to give it an appearance is to add a Text or Label, which I did. Adding the Node provided to the Skin to the scene graph again (instead of things like shapes, texts, labels, etc) would create a cycle (and FX wil throw an immediate exception :-) )
The structure is:
CustomButton (situated in a container somewhere in the Scene, added by the user to the scene graph)
|
\--> Label (displaying the button text, added by the Skin)
The default skins work similar.
I've added a number of custom components to the example
static class L extends Label { public L(String s) { super(s); setFocusTraversable(true); setMouseTransparent(false); } }together with monitoring the scene's focusOwnerProperty (don't want to mess with CSS).
These "L" components do traverse with the arrow keys and your fix with and without the ScrollPane. So perhaps I am wrong and we do want this change.
Yeah, without the fix these would also need to install navigational keys in their input maps just in case someone made them focus traversable. It's a good point that I didn't think of to justify this change, thanks for finding it.
A couple of notes: the existing example, at least when considering its top two panels with the regular buttons, exhibits reduced accessibility when the scroll pane shows a scroll bar. One can still move focus and the scroll bar does scroll the focused component to view, but one cannot induce it to scroll (for example, if one adds a long label after the last button).
I also discovered the directional navigation seems to bug and skip controls if you reduce the spacing in the HBox's to zero, but that's a separate problem for sure.
One can try making the scroll pane itself focusTraversable, but then the traversal becomes a bit weird. Perhaps it's a case for custom traversal policy.
I think it will be highly situational how an application would wish to handle this. So, I'd say that a custom event handler for such keys, or perhaps indeed something with traversal policies would need to be created by the user to solve this on a case by case basis. It will be hard to always do the expected thing with a control as generic as ScrollPane.
or perhaps indeed something with traversal policies would need to be created by the user to solve this on a case by case basis. It will be hard to always do the expected thing with a control as generic as ScrollPane.
I played with your examples and this PR a bit more. I am now convinced you are right and we should fix the ScrollPane as you proposed (with a minor suggestion).
On the traversal subject, the keyboard navigation in FX is sometimes weird, which further confirms the need for a public traversal APIs (#1555). For example, arrow down unexpectedly navigates away from the scroll pane content into the side panel (with or without this PR):
or perhaps indeed something with traversal policies would need to be created by the user to solve this on a case by case basis. It will be hard to always do the expected thing with a control as generic as ScrollPane.
I played with your examples and this PR a bit more. I am now convinced you are right and we should fix the ScrollPane as you proposed (with a minor suggestion).
On the traversal subject, the keyboard navigation in FX is sometimes weird, which further confirms the need for a public traversal APIs (#1555). For example, arrow down unexpectedly navigates away from the scroll pane content into the side panel (with or without this PR):
The directional navigation does a best fit approach here, it looks for the closest target in a 90 degree arc from the origin to find the nearest matching target -- I think it just clips the HBar policy dropdown, which is nearest, so it goes there. Otherwise it would have gone most likely to the HMax drop down. It is not aware of things like rows or layout :)