On Fri, 13 Jan 2023 04:04:54 GMT, Michael Strauß <[email protected]> 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
