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

[Michael Strauß]

On Fri, 25 Jun 2021 23:17:10 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

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

Technically, it doesn't _override_ `set`.
`FocusPropertyBase` extends `ReadOnlyBooleanPropertyBase`, and also defines a _new_ `set` method. As implemented, setting the value is completely decoupled from notifying listeners, which leads me directly to your next question...

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

`FocusPropertyBase.set` just records the new value, and the decision of whether to notify listeners is deferred until after all focus-related properties have been set on the current node and any of its parents. In the scenario you describe, the recorded value of `focusWithin` is changed twice, but ends up being the same value as before. In this case, `FocusPropertyBase.notifyListeners` will _not_ fire a change notification.

There's a test for this scenario: `FocusTest.testFocusWithinListenerIsNotInvokedIfPropertyDidNotEffectivelyChange`.

> 3. I think the way you are propagating `focusWithin` might not work if nodes are added or removed from the scene graph.

Good point, I'll try to come up with some test cases to see if there are any issues.

> 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`.

That could certainly be an option, although it would increase the memory footprint of `Node`.
I'm thinking instead to eagerly instantiate all three focus-related properties. In this case, the property field could be referenced directly, which would also remove the need for most casts.

I think that having the same deferred-notification property implementation for all three focus-related properties is the best approach, since the implementation guarantees that listeners are only notified after all changes are committed. At least, I see no upside to _not_ guarantee this. Maybe the atomicity guarantee for `focused`, `focusVisible` and `focusWithin` should be included in the documentation.

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

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