RFR: 8090456: Focus Management

Tue Sep 10 17:09:15 UTC 2024

On Tue, 3 Sep 2024 19:09:15 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

> 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/

I did a first pass of the public API and left a few comments. I'll need a second pass and a more careful look at the docs.

modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 850:

> 848:     /**
> 849:      * The {@link TraversalPolicy} to be used by this Parent to provide assistance to the
> 850:      * JavaFX focus traversal subsystem.

What assistance does this policy provide? When is it used?

modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 853:

> 851:      *
> 852:      * @defaultValue null
> 853:      * @since 999 TODO

Don't forget to update this (probably to jfx24, since there is plenty of time to get this in).

modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 31:

> 29: import java.io.File;
> 30: import java.lang.ref.WeakReference;
> 31: import java.security.AccessControlContext;

Meta-comment: This is the sort of bulk reordering that _really_ makes me want a prescribed ordering that we all use.

modules/javafx.graphics/src/main/java/javafx/scene/traversal/FocusTraversal.java line 34:

> 32: 
> 33: /**
> 34:  * Provides the mechanism for focus traversal in the JavaFX application.

I'd like to see additional documentation here describing focus traversal.

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

> 38: public final class FocusTraversal {
> 39:     /**
> 40:      * Traverses focus to the adjacent node as specified by the direction.

What does it do if the `node` to traverse from is not the currently focused node?

How does traversal differ if the method is `KEY` vs `DEFAULT`?

modules/javafx.graphics/src/main/java/javafx/scene/traversal/FocusTraversal.java line 63:

> 61: 
> 62:     /**
> 63:      * Traverse focus downward as a response to pressing a key.

Is this a convenience method like the ones that follow? If so, add docs (and if not, why not?).

modules/javafx.graphics/src/main/java/javafx/scene/traversal/FocusTraversal.java line 65:

> 63:      * Traverse focus downward as a response to pressing a key.
> 64:      *
> 65:      * @param node the origin node

What is the "origin" node? Is it is the same as the "node to traverse focus from" in the `traverse` method? If so, use the same language. If not, clearly define the difference.

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

> 33:  */
> 34: public enum TraversalDirection {
> 35:     /** Moving focus downward. */

Suggestion: `Moving` --> `Moves`

modules/javafx.graphics/src/main/java/javafx/scene/traversal/TraversalMethod.java line 26:

> 24:  */
> 25: 
> 26: package javafx.scene.traversal;

You need an `@since` javadoc tag on this class.

modules/javafx.graphics/src/main/java/javafx/scene/traversal/TraversalPolicy.java line 51:

> 49:  * @since 999 TODO
> 50:  */
> 51: public abstract class TraversalPolicy {

Who subclasses this? And for what purpose? This should be documented in the class docs.

modules/javafx.graphics/src/main/java/javafx/scene/traversal/TraversalPolicy.java line 53:

> 51: public abstract class TraversalPolicy {
> 52:     /**
> 53:      * Traverse from owner, in direction dir.

Is the "owner" the "focus owner"? What is its relationship to the "node" parameter?

modules/javafx.graphics/src/main/java/javafx/scene/traversal/TraversalPolicy.java line 63:

> 61:      * <ol>
> 62:      * <li>Find the nearest parent of the "owner" that is handled by this TraversalPolicy (i.e. it's a direct child of the root).
> 63:      * <li>select the next node within this direct child using the context.selectInSubtree() and return it

I'm going to need a second pass to understand what this does, but one thing that jumps out at me: what is "context" ? and what is the "selectInSubtree" method? I see no such method in the public API, so this description is not comprehensible (nor suitable for public documentation).

modules/javafx.graphics/src/main/java/javafx/scene/traversal/TraversalPolicy.java line 96:

> 94:      * The constructor.
> 95:      */
> 96:     public TraversalPolicy() {

Abstract classes should have a `protected` not public constructor. And the docs should say "Constructor for subclasses to call."

modules/javafx.graphics/src/main/java/javafx/scene/traversal/package-info.java line 28:

> 26: /**
> 27:  * <p>Provides the set of classes for focus traversal.</p>
> 28:  */

This should have an `@since ` tag.

Minor: I don't think the `<p>` HTML tag is needed.

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

PR Review: https://git.openjdk.org/jfx/pull/1555#pullrequestreview-2293014279
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752279424
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752281034
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752283448
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752285689
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752288335
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752292570
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752292924
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752297334
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752302045
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752306901
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752308905
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752320230
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752305206
PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752337828