8300013: Node.focusWithin doesn't account for nested focused nodes #993

mstr2 · 2023-01-12T03:08:30Z

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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8300013: Node.focusWithin doesn't account for nested focused nodes

Reviewers

Ajit Ghaisas (@aghaisas - Reviewer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/993/head:pull/993
$ git checkout pull/993

Update a local copy of the PR:
$ git checkout pull/993
$ git pull https://git.openjdk.org/jfx pull/993/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 993

View PR using the GUI difftool:
$ git pr show -t 993

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/993.diff

bridgekeeper · 2023-01-12T03:09:58Z

👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mstr2 · 2023-01-12T03:15:14Z

I think this should go into JavaFX 20.

mlbridge · 2023-01-12T03:16:01Z

Webrevs

01: Full - Incremental (1508cdbf)
00: Full (96661623)

kevinrushforth · 2023-01-12T12:38:51Z

I think this should go into JavaFX 20.

I agree. In the (very likely) case it doesn't get reviewed before the fork, I'll ask you to retarget it to the jfx20 branch.

kevinrushforth · 2023-01-12T12:39:00Z

/reviewers 2

openjdk · 2023-01-12T12:40:12Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

tcfurrer · 2023-01-16T17:35:07Z

Hi @tcfurrer, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user tcfurrer for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

aghaisas

The fix looks fine to me!

mstr2 · 2023-01-26T18:37:37Z

@kevinrushforth @arapte With RDP1 ending soon, would you be willing to review this fix?

kevinrushforth · 2023-01-27T15:02:31Z

@kevinrushforth @arapte With RDP1 ending soon, would you be willing to review this fix?

One of us will review it next week.

kevinrushforth

Looks good. Approved for jfx20.

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

kevinrushforth · 2023-01-31T16:41:03Z

modules/javafx.graphics/src/main/java/javafx/scene/Node.java

+        void adjust(int change) {
+            count += change;
+
+            if (count == 1) {


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

kevinrushforth · 2023-01-31T16:42:32Z

modules/javafx.graphics/src/main/java/javafx/scene/Node.java

+                set(true);
+            } else if (count == 0) {
+                set(false);
+            }


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.

On the one hand, this might make the code a little bit more robust, but on the other hand, it might hide a bug elsewhere. Surely count should never be negative. I lean slightly towards not protecting code against bugs in this way, mostly because it might expose potential bugs sooner.

If we want to validate that this method is not called with an incorrect argument, maybe just throwing an exception would be better.

I've created a ticket to investigate this: https://bugs.openjdk.org/browse/JDK-8301556

openjdk · 2023-01-31T17:01:47Z

@mstr2 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8300013: Node.focusWithin doesn't account for nested focused nodes

Reviewed-by: aghaisas, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 24 new commits pushed to the jfx20 branch:

05a67df: 8300664: Missing copyright header in ConditionalBinding.java file
1ccb586: Merge
8136b11: 8298167: Opacity in WebView not working anymore
1ac97fc: 8293119: Additional constrained resize policies for Tree/TableView
4a27801: 8217853: Cleanup in the D3D native pipeline
357cd85: 8282386: JavaFX media stubs rely on libav.org
bca1bfc: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes
ca29cc6: 8298728: Cells in VirtualFlow jump after resizing
e866a6c: 8296413: Tree/TableView with null focus model throws NPE in queryAccessibleAttribute()
0dbc448: 8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked
... and 14 more: https://git.openjdk.org/jfx/compare/a35c3bf78b86c57d6e80d592e99f16ab349b0d8c...jfx20

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the jfx20 branch, type /integrate in a new comment.

mstr2 · 2023-01-31T17:09:06Z

/integrate

openjdk · 2023-01-31T17:10:28Z

Going to push as commit a4bc9d1.
Since your change was applied there have been 24 commits pushed to the jfx20 branch:

05a67df: 8300664: Missing copyright header in ConditionalBinding.java file
1ccb586: Merge
8136b11: 8298167: Opacity in WebView not working anymore
1ac97fc: 8293119: Additional constrained resize policies for Tree/TableView
4a27801: 8217853: Cleanup in the D3D native pipeline
357cd85: 8282386: JavaFX media stubs rely on libav.org
bca1bfc: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes
ca29cc6: 8298728: Cells in VirtualFlow jump after resizing
e866a6c: 8296413: Tree/TableView with null focus model throws NPE in queryAccessibleAttribute()
0dbc448: 8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked
... and 14 more: https://git.openjdk.org/jfx/compare/a35c3bf78b86c57d6e80d592e99f16ab349b0d8c...jfx20

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-31T17:10:39Z

@mstr2 Pushed as commit a4bc9d1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

mstr2 added 2 commits January 12, 2023 04:00

Failing test

8dfadf6

focusWithin fix

9666162

openjdk bot added the rfr Ready for review label Jan 12, 2023

kevinrushforth requested review from aghaisas and arapte January 12, 2023 12:39

mstr2 changed the base branch from master to jfx20 January 12, 2023 16:33

refactoring

1508cdb

aghaisas approved these changes Jan 17, 2023

View reviewed changes

kevinrushforth approved these changes Jan 31, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jan 31, 2023

openjdk bot added the integrated Pull request has been integrated label Jan 31, 2023

openjdk bot closed this Jan 31, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Jan 31, 2023

mstr2 deleted the fixes/JDK-8300013 branch April 8, 2023 03:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8300013: Node.focusWithin doesn't account for nested focused nodes #993

8300013: Node.focusWithin doesn't account for nested focused nodes #993

mstr2 commented Jan 12, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 12, 2023

mstr2 commented Jan 12, 2023

mlbridge bot commented Jan 12, 2023 •

edited

Loading

kevinrushforth commented Jan 12, 2023

kevinrushforth commented Jan 12, 2023

openjdk bot commented Jan 12, 2023

tcfurrer commented Jan 16, 2023 •

edited by bridgekeeper bot

Loading

aghaisas left a comment

mstr2 commented Jan 26, 2023

kevinrushforth commented Jan 27, 2023

kevinrushforth left a comment

kevinrushforth Jan 31, 2023

kevinrushforth Jan 31, 2023

mstr2 Jan 31, 2023 •

edited

Loading

mstr2 Jan 31, 2023

openjdk bot commented Jan 31, 2023

mstr2 commented Jan 31, 2023

openjdk bot commented Jan 31, 2023

openjdk bot commented Jan 31, 2023

8300013: Node.focusWithin doesn't account for nested focused nodes #993

8300013: Node.focusWithin doesn't account for nested focused nodes #993

Conversation

mstr2 commented Jan 12, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 12, 2023

mstr2 commented Jan 12, 2023

mlbridge bot commented Jan 12, 2023 • edited Loading

Webrevs

kevinrushforth commented Jan 12, 2023

kevinrushforth commented Jan 12, 2023

openjdk bot commented Jan 12, 2023

tcfurrer commented Jan 16, 2023 • edited by bridgekeeper bot Loading

aghaisas left a comment

Choose a reason for hiding this comment

mstr2 commented Jan 26, 2023

kevinrushforth commented Jan 27, 2023

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Jan 31, 2023

Choose a reason for hiding this comment

kevinrushforth Jan 31, 2023

Choose a reason for hiding this comment

mstr2 Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

mstr2 Jan 31, 2023

Choose a reason for hiding this comment

openjdk bot commented Jan 31, 2023

mstr2 commented Jan 31, 2023

openjdk bot commented Jan 31, 2023

openjdk bot commented Jan 31, 2023

mstr2 commented Jan 12, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 12, 2023 •

edited

Loading

tcfurrer commented Jan 16, 2023 •

edited by bridgekeeper bot

Loading

mstr2 Jan 31, 2023 •

edited

Loading