jfx
jfx copied to clipboard
openjdk
8090456: Focus Management
Public APIs for focus traversal and the focus traversal policy:
https://github.com/andy-goryachev-oracle/Test/blob/main/doc/FocusTraversal/FocusTraversal.md
This work is loosely based on the patch https://cr.openjdk.org/~jgiles/8061673/
/csr /reviewers 3
Progress
- [x] Change must not contain extraneous whitespace
- [ ] Change requires a CSR request matching fixVersion jfx24 to be approved (needs to be created)
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed (3 reviews required, with at least 1 Reviewer, 2 Authors)
Issue
- JDK-8090456: Focus Management (Enhancement - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1555/head:pull/1555
$ git checkout pull/1555
Update a local copy of the PR:
$ git checkout pull/1555
$ git pull https://git.openjdk.org/jfx.git pull/1555/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1555
View PR using the GUI difftool:
$ git pr show -t 1555
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1555.diff
Webrev
:wave: Welcome back angorya! 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}
❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.
@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@andy-goryachev-oracle please create a CSR request for issue JDK-8090456 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.
@andy-goryachev-oracle The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 1 Reviewer, 2 Authors).
Webrevs
- 11: Full (9caeaa18)
- 10: Full - Incremental (da2cef2a)
- 09: Full (29770631)
- 08: Full (5b31a64f)
- 07: Full (9e1fa796)
- 06: Full - Incremental (d1f3152a)
- 05: Full - Incremental (98af7a6e)
- 04: Full - Incremental (de70ad46)
- 03: Full - Incremental (02a8d261)
- 02: Full (12a54fee)
- 01: Full - Incremental (ddd3b281)
- 00: Full (4f168952)
By its own admission, this proposal basically promotes an internal implementation to public API (albeit with some refactoring). This may or may not be a good idea, so I'd like to take a step back and look at the problem we're trying to solve.
Looking at several of the linked JBS issues, a missing feature of JavaFX is the ability to customize the focus traversal logic of a custom control (or a container of nodes in the widest sense of the word). Any proposed solution should support complex scenarios like toolbars with overflow, or radio groups.
I think there are two distinct modes of focus traversal: logical ("tabbing") and directional (arrow keys). Both modes are independent axes of the problem at hand, and should be customizable independently from each other.
The current implementation provides a single Algorithm (or TraversalPolicy after the refactoring) for both traversal modes. I've played around with the code for a while, and while in theory I can provide my own TraversalPolicy, it can be quite a lot of code and it's not as easy as it could be. Additionally, it is hard to customize the traversal policies of the substructure of skinnable controls.
I question whether we need custom traversal policies at all, as the ways in which focus can move from one node to the next are quite limited:
- Permeable edges: The next node in a container is selected until an edge is reached, then the input focus leaves the container.
- Confined to container: The next node in a container is selected until the end of the container is reached; then the input focus doesn't move on.
- Cyclic in container: Like confined, but at the end of the container, the input focus wraps around to the first element.
- Single focused node in container: When the input focus enters a container, it moves to the node that was most recently selected (or to the first node in the container if none of the nodes was selected before); after that the input focus leaves the container.
Having two traversal toggles (logical and directional), each with four possible modes, already solves most of the problem. The advantage of this over a custom policy implementation is that these properties can be modelled with an enumeration and easily be set by control and application developers, and they can also be set via CSS. This allows developers to customize the focus traversal behavior of existing skinnable controls, as the substructure of a skin is accessible via CSS.
The "single focused node in container" mode is required for scenarios like radio groups, when the developers wants to return to the point of previous focus when tabbing into the group a second time. This requires another API that allows a focus container to indicate its entry point, which is the initial element that is selected when the input focus moves into the container.
Thank you @mstr2 for a detailed writeup!
I fully agree with you that the majority of use cases can be handled with the existing policies.
The default one seems to be geometry-based ("directional") where supported, and the other being a "logical" one which is not explicitly accessible. The question, therefore, is do we need a static getter for that kind of a policy? (I'd say no since the logical order depends on a particular control).
A couple of points:
- setting via CSS is not a goal (I probably should mention this in the JEP)
- the 4-item enumeration is not complete. For example, application may specify some other, or even dynamic, order of traversal, which would require a custom policy.
Another question is whether there exists a policy that cannot be implemented by the new APIs provided by this PR.
What do you think?
The default one seems to be geometry-based ("directional") where supported, and the other being a "logical" one which is not explicitly accessible. The question, therefore, is do we need a static getter for that kind of a policy? (I'd say no since the logical order depends on a particular control).
I don't quite understand what you mean by "not explicitly accessible". Both categories are exposed with the TraversalDirection enum: UP, DOWN, LEFT, and RIGHT are the directional modes, whereas NEXT, NEXT_IN_LINE, and PREVIOUS are the logical modes.
I think that disentangling those two categories, and having a set of predefined traversal policies that can be set independently for both of those, will be good enough for 99% of cases. The reason I'm bringing this up is that Algorithm (now TraversalPolicy) is too low-level to be useful for most application developers. We should be implementing those algorithms, and then allow developers to compose them as they see fit.
When I look at the existing code, I see quite a lot of bespoke Algorithm implementations. For example, ToolBarSkin has almost 100 lines of code dedicated to a custom implementation. However, when I just remove this implementation entirely, I get almost the exact same focus traversal behavior, even in complicated scenarios like when a toolbar item is itself a focus-traversable container.
This suggests to me that we shouldn't encourage the proliferation of even more bespoke implementations, but make it easier to compose well-tested existing algorithms.
A couple of points:
- setting via CSS is not a goal (I probably should mention this in the JEP)
- the 4-item enumeration is not complete. For example, application may specify some other, or even dynamic, order of traversal, which would require a custom policy.
We can add scenarios to this list if it is not complete, and then see if we still need the option to create a custom traversal algorithm. My point is that the API should be tailored to be simple and useful in 99% of cases, and then maybe have an additional extension point for the rest. Right now, you're proposing to expose the lowest-level API which requires developers to reason about the scene graph and find individual nodes, which I think should be reconsidered.
When I look at various JBS issues and browse StackOverflow, I notice that most developers are only looking for pretty simple things like cyclic traversal in a container. It's probably prudent to tailor the API for those use cases.
Another question is whether there exists a policy that cannot be implemented by the new APIs provided by this PR.
What do you think?
Since the API works directly on the scene graph, you can probably create all kinds of algorithms with it. The downside of that is, however, that all implementations are ad-hoc solutions that don't generalize. For example, the radio group I mentioned earlier suggests that we should have an API to define an "entry point" into a container of nodes. You can do that with a custom implementation, but I think having this be elevated to a proper API would be better.
Thank you @mstr2 for your comments! Since we also have a parallel discussion in the mailing list, I'll respond there.
I think we both agree that the main goal of this proposal (or any proposal) is to make the things easier in the most common scenarios, while making all other scenarios possible. Right now it is simply not possible to do focus traversal on the application side.
This proposal makes focus traversal and custom policies possible. You are right that in most cases the default policy would be sufficient, and we also might provide additional standard policies (like one based on an array of Nodes within a Parent), and that should cover 99% of the use cases.
One thought I'd like to vocalize: the idea of setting traversal properties or policies from CSS does not feel right to me. The CSS define presentation aspects (styles) rather than behavioral ones. I know it is possible to set custom skins and the corresponding behavior via CSS, and we know why (skins define the appearance), but we should not go beyond that, in my opinion.
@hjohn following the discussion in https://github.com/openjdk/jfx/pull/1582 , are there any objections from your side?
I'm missing the explanation in the proposal for why the API needs to be static methods called on nodes, and not have these methods on the nodes themselves. Instead of public static boolean traverseDown(Node node), why not public boolean traverseDown() in Node?
I'm missing the explanation in the proposal for why the API needs to be static methods called on nodes, and not have these methods on the nodes themselves.
I would say
FocusTraversalacceptsnullnodes, making the code more resilient- the static nature of
FocusTraversalindicates that it is not something related to particular node, but happens in the context of the scene graph
do you want this mentioned in the JEP or in the FocusTraversal class?
FocusTraversal accepts null nodes, making the code more resilient
In what way is it more resilient? traverse does nothing if the node is null, which means that passing null is a bug more often than not. null nodes can't be displayed in the scenegraph anyway, so what is the use case of passing in null? Resilient code is usually more restrictive, guiding the user away from misuse, not one that accepts everything.
the static nature of FocusTraversal indicates that it is not something related to particular node, but happens in the context of the scene graph
But methods like next of left are related to a particular node - the one that indicates to which node focus will be moved. Node::requestFocus means "give focus to me", Node::focuseNext means "give focus to the next node from me".
maybe the word to use is "convenient" instead of "resilient".
But ultimately, I wanted to separate already large Node class from the focus traversal and its convenience methods. At some point, we are going to hit one of the class file limits... Ultimately, there is no difference, as it all eventually goes through the same logic.
It might be a matter of personal preference. After all, we do have SwingUtilities.invokeLater() and EventQueue.invokeLater() in swing, and Event.fireEvent() in fx.
Is there a deep architectural reason you don't want FocusTraversal with its static methods?
FWIW I have no issue with these as static methods. You're asking the traversal engine to move the focus and supplying a starting node. The traversal engine is the only actor here; the node is just a bystander. It might be clearer if the name was FocusTraversal.traverseLeftFrom(Node node) though that's a matter of style (I'm not asking for any changes.
Utility classes (static methods) decouple the behavior from the data and operate via side-effects. Node already defines focusedProperty (and protected setFocused), which means it has taken responsibility for focus management. This also means you can't ask for focus on null nodes.
Adding an external class makes it more difficult to do focus management. It also makes the code less clean because of the added utility class, which the user doesn't need because they already reference the object:
TextField tf = ...
tf.focusNext();
TextField tf = ...
ExternlUtility.focusNext(tf); // plus import
In general, this is anti-OO. While there are very valid cases for this sort of data-oriented design, it usually applies to processing of immutable data. Otherwise an object is usually responsible for mutating its own data and not let an external class do it.
I can just as well ask, why do we need all the setOn__ methods on Node? We can have a utility class for EventManager.setOn(Node node, Event event). Or any other property.
If Node is too big, create an internal FocusManager class (or with whatever suitable name) in its own file and have Node hold a reference to it while only delegating to its methods.
I have also been wondering (before this discussion) if it ever made sense for all nodes to have focus management. Many nodes are panes - can they actually be focused? If not, I would have used a Focusable interface only for those subclasses that can have focus.
I would say instance methods operate on instances, that is, the only change typically expected from them is the change of internal state of that instance.
It is true for the focusable property: it's the property of the Node. It's a read-only property, meaning something else is setting it (the focus subsystem).
The reason for FocusTraversal class is to indicate the fact that traverseXXX methods impact more than one node, so I would disagree with the statement that this is anti-OO. It is not about that particular node any more, so it should not be an instance method, in my opinion.
I would have used a
Focusableinterface only for those subclasses that can have focus.
a good point! I can add one more to that: why are we creating copies of Events during the dispatch? Doing so breaks isConsumed() contract and causes issues that are impossible to fix without some kind of hacking around.
Adding an external class makes it more difficult to do focus management.
How so?
There are two parts to the focus management: focus traversal which effects the focusOwner change, and traversal policy that allows for custom logic.
The third part (the "focus manager") is built-in and not a subject to customization.
What exactly do you mean by the focus management? What specific task is impossible using the proposed API?
(I am trying to understand the reasoning behind your objection)
I would be in favor of sending this proposal back to the drawing board for the following reasons:
- It feels like an undercooked implementation detail was promoted to API. What developers need is a way to easily customize the focus traversal behavior, instead of coding new behaviors from scratch. The correct way to do this is to realize that there are two distinct modes of focus traversal (directional and logical), and that these two modes should be separately configurable.
- Instead of providing two separate toggles with a set of values to choose from, this API encourages developers to write custom traversal policies like the one in
ToolBarSkin, which contains 100 lines of code that do... something, and if you just remove all of it, the behavior is almost unchanged. That's not a good "code to observable effects" ratio. It will also lead to a proliferation of poorly-made custom policies (who has the time to cover all edge cases). - It fails to provide an easy out-of-the-box solution for the most common developer requests, like container-cyclic focus traversal.
My suggestion for a better API is: have two separate knobs to configure directional and logical focus traversal, and provide a very small set of curated policies. We should only consider opening up the API to allow more customization when there is a demonstrable need that goes beyond the curated policies.
This PR does not provide any new logic.
It opens up two APIs which are independent of the implementation: the focus traversal methods and the traversal policy, for customization.
We can take upon creating additional traversal policies as a separate effort, including any optimization of the policies used by ToolBarSkin or any other skin, and maybe making them public if they are general enough.
We can also take upon any changes in the current traversal logic, including maybe a way to customize the global traversal algorithm, also as a separate effort.
This PR provides the absolute minimal APIs to support custom components and custom traversal policies.
Are there any objections to the new public APIs (and not the implementation)? Is there something the application developer or custom component developer cannot do with the proposed API?
Are there any objections to the new public APIs (and not the implementation)? Is there something the application developer or custom component developer cannot do with the proposed API?
If we add the traversalPolicy API now, this effectively closes the door on directionalTraversalPolicy and logicalTraversalPolicy as independently configurable APIs. So as far as your question is concerned, my objection is that the proposed API fails to recognize that this is the most important distinction that should be modeled by the API.
My suggestion for a better API is: have two separate knobs to configure directional and logical focus traversal, and provide a very small set of curated policies. We should only consider opening up the API to allow more customization when there is a demonstrable need that goes beyond the curated policies.
As I was reviewing the existing JavaFX control set and the w3c guidelines I found at least three different cyclical traversal patterns (menu bars and radio button groups are the two that spring to mind). In all cases the directional and logical traversals behaviors were intertwined. Among the three different ways of cycling (left/right, up/down, and with the tab key) you have to ensure at least one of them allows the user to escape the cycle and each model opted for a different escape route. Cyclical policies are more diverse than one might think.
If we close up the traversal API we will be on the hook for identifying and implementing all the traversal policies. If a developer needs something more they'll have to petition for it to be included in JavaFX or provide their own PR and try to get it through the review process. That's not a good use of our time or theirs.
In any case I don't believe that we need to protect developers from themselves. Presumably they're a smart bunch. If you're concerned they won't get it right make sure there's ample documentation and sample code.
My suggestion for a better API is: have two separate knobs to configure directional and logical focus traversal, and provide a very small set of curated policies. We should only consider opening up the API to allow more customization when there is a demonstrable need that goes beyond the curated policies.
As I was reviewing the existing JavaFX control set and the w3c guidelines I found at least three different cyclical traversal patterns (menu bars and radio button groups are the two that spring to mind). In all cases the directional and logical traversals behaviors were intertwined. Among the three different ways of cycling (left/right, up/down, and with the tab key) you have to ensure at least one of them allows the user to escape the cycle and each model opted for a different escape route. Cyclical policies are more diverse than one might think.
I don't think they are intertwined. For example, a radio button group will have a directionalTraversalPolicy="cyclic in container" and a logicalTraversalPolicy="single focused node in container" (see here). As I said before, this requires the addition of another API, namely the "entry point" into a container-like structure.
If we close up the traversal API we will be on the hook for identifying and implementing all the traversal policies. If a developer needs something more they'll have to petition for it to be included in JavaFX or provide their own PR and try to get it through the review process. That's not a good use of our time or theirs.
This is not a sufficient reason to skip API engineering and just expose an implementation detail, like this PR does. For example, Martin says this about NEXT_IN_LINE back in 2014:
I also had to introduce special direction NEXT_IN_LINE, which does traverse to the next Node and ignores children of the current Node. This is needed when a Parent's inner traversal for NEXT returns null, we need to traverse from that Parent, but not inside it (which is normally done with NEXT). I don't like NEXT_IN_LINE much, but since it's not a public API (yet), I think we can live with it.
"Living with it" is not good enough for public API, and so far, no one has demonstrated why the API as proposed in this PR is a sensible API in the first place, its raison d'être simply being "it already exists in the internals".
In any case I don't believe that we need to protect developers from themselves. Presumably they're a smart bunch. If you're concerned they won't get it right make sure there's ample documentation and sample code.
I'm not concerned about the proficiency of JavaFX developers, I'm concerned about convincing ourselves that since we don't have the time to do it right, we should just rush out clearly undercooked APIs.
@mstr2 My apologies, I made the cardinal sin of not re-reading earlier portions of the thread and took your latest comments out of context. I need to take the time to get back up to speed. In particular I should have noticed your earlier comments about an API to control entry into a container; I agree that does seem to be missing from this proposal.
this requires the addition of another API, namely the "entry point" into a container-like structure.
API to control entry into a container; I agree that does seem to be missing from this proposal.
public abstract Node selectFirst(Parent root)
I struggle to see what problem this will be solving that isn't already solved with the above.
Let me try to explain this (again).
At high level, there might be application-level requirements for conditional traversal, which should be implemented via application-level traversal policy.
To give an example: "if you answered [yes] on line 23, go directly to line 35".
I struggle to see what problem this will be solving that isn't already solved with the above.
Let me try to explain this (again).
...
At high level, there might be application-level requirements for conditional traversal, which should be implemented via application-level traversal policy.
To give an example: "if you answered [yes] on line 23, go directly to line 35".
IMHO this sounds like a solution looking for a problem. This is form based type logic, which is IMHO far too high level for FX to cater to. Specifically for this case, I'd expect "lines" 24-34 to simply be disabled, and so navigation automatically proceeds to the next logical location.
I remain thoroughly unconvinced that this is sufficient to require a new traversal policy API. I would move that we provide methods that can change focus in the way FX currently allows with directional and logical navigation keys so users/developers don't need to simulate key presses to get at this functionality. Then we can see what use cases are still uncovered.
(some time later)
I did my best to actually find some use cases... so I checked 50 bugs that match JavaFX + focus that are not closed or resolved.
I only found two that had some relevancy to recent discussions:
| Link | Title | Resolution |
|---|---|---|
| https://bugs.openjdk.org/browse/JDK-8090584 | Support standard for UP / DOWN remapping | Seems like it would be solved by fixing ScrollPane and removing the early focus traversal mappings embedded directly in controls... |
| https://bugs.openjdk.org/browse/JDK-8224744 | Add a feature similar to Swing's FocusEvent.getOppositeComponent | Seems invalid, you can do this with a ChangeListener on Scene#focusOwner |
Plus there was a request on the mailinglist to provide a way to just focus the next logical control. So, where are the use cases then? What are we exactly trying to solve for which group of users? That's really something one needs to know when designing a new API.
I did my best to actually find some use cases...
Allow me to refer to some of the linked tickets in RFE https://bugs.openjdk.org/browse/JDK-8090456:
JDK-8091524 Introduce a "focus container" concept in class Parent
JDK-8091670 Add API for better control over keyboard focus
I did my best to actually find some use cases...
Allow me to refer to some of the linked tickets in RFE https://bugs.openjdk.org/browse/JDK-8090456:
JDK-8091524 Introduce a "focus container" concept in class Parent
JDK-8091670 Add API for better control over keyboard focus
JDK-8091524 - problem: User wants to create a panel that has its own focus cycle, and focus should not escape to other controls in the same Scene. I saw this issue and feel that it is a "WON'T FIX" as there are standard solutions for this already.
It is clearly an X/Y problem. A common thing that trips up beginning FX developers is that they're unwilling to create a 2nd Scene or a Dialog and prefer to just "add a child somewhere to show some extra controls". This is not how you do this, and we shouldn't need to cater to something that is basically building a Dialog type infrastructure from scratch. How would this handle mouse clicks outside the "blessed" panel? Should that be restricted as well then?
Possible ways to address this problem: use dialogs, use nested event loops, disable components that should not be part of the focus cycle temporarily, overlay a transparent pane blocking clicks, etc... but best option, just use a dialog...
JDK-8091670 - describes the same problem... Ensemble is not using the right tools to restrict focus. Use dialogs for this purpose. There is no need to restrict focus on container level. Again, what happens when focus is restricted to a container, and the user clicks outside it? What should then happen when I navigate? Should the restricted container be part of the primary focus cycle, but then capture it with no way to exit it once you get there?
Working example
Just to show you can easily do this already, below is an example that will show a new pane, with navigation restricted to it, allowing you to select a new name for a button. Note that you can't click outside it with the mouse (although I'm sure I can add that as well and then "hide" the dialog). Also note that the Dialog doesn't appear as a dialog (they don't have to) and that no new Window is created on the task bar or whatever.
package app;
import javafx.application.Application;
import javafx.event.ActionEvent;
import javafx.geometry.Point2D;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Dialog;
import javafx.scene.control.Label;
import javafx.scene.control.ScrollPane;
import javafx.scene.layout.Border;
import javafx.scene.layout.GridPane;
import javafx.scene.layout.HBox;
import javafx.scene.layout.VBox;
import javafx.scene.paint.Color;
import javafx.stage.Stage;
import javafx.stage.StageStyle;
public class App extends Application {
@Override
public void start(Stage primaryStage) {
GridPane gp = new GridPane(10, 10);
gp.addEventHandler(ActionEvent.ACTION, e -> {
// Create a Dialog:
Dialog<String> d = new Dialog<>();
// Get Button that triggered it:
Button node = (Button)e.getTarget();
// Find location of Button where we want to display a new Pane:
Point2D pos = node.localToScreen(new Point2D(0, node.getLayoutBounds().getHeight()));
// Set pane position and make it look like it is integrated:
d.initStyle(StageStyle.TRANSPARENT);
d.initOwner(node.getScene().getWindow());
d.setX(pos.getX());
d.setY(pos.getY());
// Some content for our pane:
HBox content = new HBox(5, new Label("Change button name to: "), new Button("Foo"), new Button("Bar"), new Button("Baz"));
// Add a border to make it more clearly delineated, and a handler when any button was clicked:
content.setBorder(Border.stroke(Color.RED));
content.addEventHandler(ActionEvent.ACTION, ae -> {
d.setResult(((Button)ae.getTarget()).getText());
d.close();
});
// Set the content, show the pane, and if there was a result, set that the new text on the button that triggered it:
d.getDialogPane().setContent(content);
d.showAndWait().ifPresent(node::setText);
});
gp.add(new VBox(new Label("Standard Buttons in normal container"), new VBox(5, new Button("Click me"), 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);
Scene scene = new Scene(gp);
primaryStage.setScene(scene);
primaryStage.show();
}
}