On Wed, 9 Jun 2021 14:18:41 GMT, Michael Strauß <mstra...@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
