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

[Nir Lisker]

On Mon, 28 Oct 2024 16:13:37 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Public focus traversal API for use in custom controls
>> 
>> https://github.com/andy-goryachev-oracle/Test/blob/main/doc/FocusTraversal/FocusTraversal-v3.md
>> 
>> This work is loosely based on the patch
>> https://cr.openjdk.org/~jgiles/8061673/
>> 
>> And is a scaled down version (with the public traversal policy API removed) of
>> #1555
>
> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review comments

The changes look good, left minor comments. I only reviewed the API.

modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 10451:

> 10449:      */
> 10450:     public final boolean requestFocusTraversal(TraversalDirection direction) {
> 10451:         TraversalDirectionInternal d = switch(direction) {

Minor: space after `switch`.

modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 10458:

> 10456:         case RIGHT -> TraversalDirectionInternal.RIGHT;
> 10457:         case UP -> TraversalDirectionInternal.UP;
> 10458:         default -> null;

Do we indent labels inside a switch? I find it more readable since `switch` opens a new context. This is also what I remember seeing in OpenJFX. Don't know if it's enforced.

modules/javafx.graphics/src/main/java/javafx/scene/TraversalDirection.java line 38:

> 36:     /** Indicates a focus change to the node to the left of the currently focused node. */
> 37:     LEFT,
> 38:     /** Indicates a focus change to the next focusable Node, possibly traversing into the children of the current parent. */

Minor: "Node" -> "node" like the other places it's used.

modules/javafx.graphics/src/main/java/javafx/scene/TraversalDirection.java line 40:

> 38:     /** Indicates a focus change to the next focusable Node, possibly traversing into the children of the current parent. */
> 39:     NEXT,
> 40:     /** Indicates a focus change to the previous focusable node. */

`NEXT` talks about traversing children, does `PREVIOUS` need to mention it too? Could it traverse out of the children of the current parent, or is it not symmetric?

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

PR Review: https://git.openjdk.org/jfx/pull/1604#pullrequestreview-2399576178
PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1819364915
PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1819366367
PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1819362996
PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1819364334