[jfx20] RFR: 8300013: Node.focusWithin doesn't account for nested focused nodes [v2]

[Kevin Rushforth]

On Fri, 13 Jan 2023 04:04:54 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> When a scene graph contains multiple nested focused nodes (this can happen with `TableView` and other controls), the `focusWithin` bits that are cleared when a focused node is de-focused must only be cleared when there is no other nested node in the scene graph that would also cause `focusWithin` to be set.
>> 
>> For example, consider a scene graph of nested nodes:
>> A -> B -> C -> D
>> 
>> When B and D are both focused, the scene graph looks like this:
>> A(`focusWithin`)
>> -> B(`focused`, `focusWithin`)
>> -> C(`focusWithin`)
>> -> D(`focused`, `focusWithin`)
>> 
>> When B is de-focused, the `focusWithin` flags must still be preserved because D remains focused.
>> 
>> This PR fixes the issue by counting the number of times `focusWithin` has been "set", and only clears it when it has been "un-set" an equal number of times.
>
> Michael Strauß has updated the pull request incrementally with one additional commit since the last revision:
> 
>   refactoring

Looks good. Approved for `jfx20`.

I left a couple questions, one of which might warrant a follow-up issue depending on your answer.

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

> 8319:             count += change;
> 8320: 
> 8321:             if (count == 1) {

This presumes that you never call this with `change > 1`. You don't, so it seems fine.

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

> 8323:             } else if (count == 0) {
> 8324:                 set(false);
> 8325:             }

Is it worth adding a check for `count < 0` and logging a warning (possibly treating it as if it were 0)? In theory, it shouldn't happen, but if it ever did, `focusWithin` would be wrong after that. This could be done as a P4 follow-up for 21, unless you are certain that it cannot ever happen.

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

Marked as reviewed by kcr (Lead).

PR: https://git.openjdk.org/jfx/pull/993