RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v2]

[Kevin Rushforth]

On Wed, 9 Jun 2021 14:18:41 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> This PR adds the `Node.focusVisible` and `Node.focusWithin` properties, as well as the corresponding `:focus-visible` and `:focus-within` CSS pseudo-classes.
>
> Michael Strauß has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - wording
>  - Re-ordered methods

The API looks reasonable. I left one question inline.

The implementation will take some time to go through. I doubt that this will make JavaFX 17 (which I alluded to earlier). I'm especially curious about some of the patterns that I see, mostly surrounding the `FocusPropertyBase` implementation class.
1. `FocusPropertyBase` overrides `set` (which is unusual). The current implementation (of `FocusedProperty `) has some custom logic as well, but it didn't try to use the set method of a writable property. Also, you use a writable property and don't override `invalidated`. Can you say why you made the changes you made?
2. I'm curious about the approach for notifying listeners. It seems similar to what is currently done for `focusedProperty`, but there are differences. Also, I'm not sure that the two newly added properties need to handle notifications in the same way as focused. I can think of at least one potential issue. If focus shifts from one node to another in the same parent, you might end up notifying a parent's listeners even though its `focusWithin` is unchanged.
3. I think the way you are propagating `focusWithin` might not work if nodes are added or removed from the scene graph.
4. Normally a read-only property that needs to be settable by the implementation would use `ReadOnlyBooleanWrapper`. And then the public property method would call `getReadOnlyProperty` to return the read-only property to the user. The implementation would access the wrapper and you don't need to cast. You could consider having `FocusPropertyBase` extend `ReadOnlyBooleanWrapper`.

One thing to consider would be to keep the existing (internal) class for `focused` and use a more standard `ReadOnlyBooleanWrapper` class for the two new ones. But maybe there are some implications I haven't thought of yet.

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

> 8240:      * Indicates whether this {@code Node} should visibly indicate focus.
> 8241:      * This flag is set when a node acquires input focus via keyboard navigation,
> 8242:      * and it is cleared when {@link #requestFocus()} is called.

It's also cleared if the node loses focus, right?

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

PR: https://git.openjdk.java.net/jfx/pull/475