RFR: 8091673: Public focus traversal API for use in custom controls [v6]

[John Hendrikx]

On Tue, 29 Oct 2024 18:14:31 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

> Just to complete the picture, I also want to explain why I am strongly against making `focusVisible` writable.

I don't disagree, I'm just a bit on the fence if making it writable by proxy is a good idea then when it is a concept that should be controlled by FX's implementation.

> The (much older) `focused` property is an example of bad API design: it is a read-only property that is controlled by the focus subsystem, but in a stroke of complete hackery, also allows subclasses to change its value arbitrarily with the protected `setFocused` method. From this it necessarily follows that the documentation of `focused` is lying:
> 
> ```
>     /**
>      * Indicates whether this {@code Node} currently has the input focus.
>      * To have the input focus, a node must be the {@code Scene}'s focus
>      * owner, and the scene must be in a {@code Stage} that is visible
>      * and active. See {@link #requestFocus()} for more information.
>      *
>      * @see #requestFocus()
>      * @defaultValue false
>      */
> ```
> 
> Since `focused` can be set (and will be set) arbitrarily by subclasses, the presence of this flag does _not_ necessarily indicate that the node has the input focus, it does _not_ necessarily indicate that it is the scene's focus owner, and it does _not_ necessarily indicate that it is in an active scene.

Yeah, it's not a very useful property in its current state; I see it as more of an indication that something has the focused look (like disabled, pressed and armed for example) and not necessarily that such a Node actually has the focus.  Better to use focusOwner for that.

> You might think that this is only a small loophole in the implementation, but sadly it is not: Several controls, most notably TableView, abuse the `focused` property for an entirely different purpose. They use it to mark the cell that is currently _selected_, and then hack the `focused` property value to reflect the _selection state_. This selection persists even when the cell doesn't have the input focus (and thus, the `focused` property lies about the input focus).

I think it is sometimes desirable to have a cell look like it has focus, as well as the TableView itself, but I could be wrong, or it should have been implemented differently.  Perhaps the concepts are more like the TableView has the focus, and its cursor is on a specific cell (similar to a simple text box, where the box has the focus, and there's a cursor for navigation/editing -- we don't say that position 43 has the focus either when the cursor is located there...).

As for `focusVisible`, I think a good default would definitely be to set it to true when the programmatic methods are called.  Yes, you could call these methods in response to a mouse click, but I think that would be really silly (ie. the user clicks at location X,Y, and you programmatically change focus to something that wasn't clicked).  Regardless, indicating **where** the focus went, even if it was a mouse click, is probably a good idea.

If we can't think of a good use case now to set it to `false` after programmatic navigation, then I would strongly suggest to wait for such a use case to appear (on the bug tracker), evaluating that, and then potentially adding an overload or some other solution (like the one I suggested that checks if a key event was in the call stack).

Also something that just occurred to me, but perhaps I'm late to the party; with the current API being on `Node` you could call `requestFocusTraversal` on basically any `Node`, even one that is currently not focused.  I'm not sure if this is intentional or that the API should perhaps only work from the current focused node.

ps.  the diff for this PR still looks huge here?

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1604#issuecomment-2445369003