8297130: ComboBox popup doesn't close after selecting value that was added with 'runLater' #956

mstr2 · 2022-11-19T18:01:41Z

This PR fixes a bug where multi-level focus is not correctly preserved.
The original implementation incorrectly assumed that there can only be a single focused node in the scene graph, which is not the case when a branch of the scene graph is presented by a PopupWindow. More specifically, when a focused node was removed from the scene graph, the focus flags of all parents were incorrectly cleared. The correct implementation only clears the focusWithin flag of parents (but not focused or focusVisible), and stops when another focused node is encountered along the way.

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-8297130: ComboBox popup doesn't close after selecting value that was added with 'runLater'

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Author)
Ajit Ghaisas (@aghaisas - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 956

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

Using diff file

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

bridgekeeper · 2022-11-19T18:02:47Z

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

mlbridge · 2022-11-19T18:09:05Z

Webrevs

00: Full (32c065db)

kevinrushforth · 2022-11-22T13:28:44Z

/reviewers 2

kevinrushforth · 2022-11-22T13:30:17Z

Reviewers: @aghaisas @andy-goryachev-oracle

openjdk · 2022-11-22T13:30:57Z

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

andy-goryachev-oracle

no ill effects observed with several test applications.

incidentally, this change has no effect on JDK-8292933 dual focus issue.

andy-goryachev-oracle · 2022-11-23T19:26:18Z

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

-            while (node != null) {
-                node.focused.set(false);
-                node.focusVisible.set(false);
+            while (node != null && !node.focused.get()) {


is there a reason not to use isFocused() ?

In this area of the code, the eagerly initialized fields focused, focusVisible and focusWithin are used directly. Other than that, there's no reason. If you prefer the convenience getters, I can change it.

andy-goryachev-oracle · 2022-11-23T19:26:31Z

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

@@ -8239,7 +8240,7 @@ public void set(boolean value) {
                do {
                    node.focusWithin.set(value);
                    node = node.getParent();
-                } while (node != null);
+                } while (node != null && !node.focused.get());


isFocused() ?

aghaisas

The fix looks good to me!

openjdk · 2022-11-24T11:29:22Z

@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:

8297130: ComboBox popup doesn't close after selecting value that was added with 'runLater'

Reviewed-by: angorya, aghaisas

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 17 new commits pushed to the master branch:

cce8580: 8254676: Alert disables Tab selection when TabDragPolicy REORDER is used
a0ea874: 8279214: Memory leak in Scene after dragging a cell
9819d45: 8297332: Remove easy warnings in javafx.base
086dac0: 8297213: Robot capture tests should move mouse to corner of screen
e3e0dfd: 8297166: [TestBug] Fix some ignored unit test from TableViewTest
626227d: 8296330: Tests: Add layout container snapping tests
1ec06a8: 8206430: Use consistent pattern for startup in FX system tests
74af45d: 8296556: Mark two RobotTest methods as unstable on HiDPI Windows systems
6cc9c4d: 8297042: gradle -PBUILD_SDK_FOR_TEST=false fails with gradle 7.6
a13630f: 8268877: TextInputControlSkin: incorrect inputMethod event handler after switching skin
... and 7 more: https://git.openjdk.org/jfx/compare/7f17447aa44fbf5b09aaa2b699266dac8b50cea1...master

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 master branch, type /integrate in a new comment.

mstr2 · 2022-11-24T15:51:51Z

/integrate

openjdk · 2022-11-24T15:52:50Z

Going to push as commit 4a697f1.
Since your change was applied there have been 17 commits pushed to the master branch:

cce8580: 8254676: Alert disables Tab selection when TabDragPolicy REORDER is used
a0ea874: 8279214: Memory leak in Scene after dragging a cell
9819d45: 8297332: Remove easy warnings in javafx.base
086dac0: 8297213: Robot capture tests should move mouse to corner of screen
e3e0dfd: 8297166: [TestBug] Fix some ignored unit test from TableViewTest
626227d: 8296330: Tests: Add layout container snapping tests
1ec06a8: 8206430: Use consistent pattern for startup in FX system tests
74af45d: 8296556: Mark two RobotTest methods as unstable on HiDPI Windows systems
6cc9c4d: 8297042: gradle -PBUILD_SDK_FOR_TEST=false fails with gradle 7.6
a13630f: 8268877: TextInputControlSkin: incorrect inputMethod event handler after switching skin
... and 7 more: https://git.openjdk.org/jfx/compare/7f17447aa44fbf5b09aaa2b699266dac8b50cea1...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-24T15:53:17Z

@mstr2 Pushed as commit 4a697f1.

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

przemSzer · 2023-04-05T13:45:10Z

Do you plan to add this fix to the version 19, of Java FX?

mstr2 added 2 commits November 19, 2022 18:50

Failing test

bb4e02d

Preserve multi-level focusWithin

32c065d

openjdk bot added the rfr Ready for review label Nov 19, 2022

kevinrushforth requested a review from aghaisas November 22, 2022 13:29

andy-goryachev-oracle approved these changes Nov 23, 2022

View reviewed changes

aghaisas approved these changes Nov 24, 2022

View reviewed changes

openjdk bot added the ready Ready to be integrated label Nov 24, 2022

openjdk bot added the integrated Pull request has been integrated label Nov 24, 2022

openjdk bot closed this Nov 24, 2022

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Nov 24, 2022

mstr2 deleted the JDK-8297130 branch November 24, 2022 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8297130: ComboBox popup doesn't close after selecting value that was added with 'runLater' #956

8297130: ComboBox popup doesn't close after selecting value that was added with 'runLater' #956

mstr2 commented Nov 19, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 19, 2022

mlbridge bot commented Nov 19, 2022

kevinrushforth commented Nov 22, 2022

kevinrushforth commented Nov 22, 2022

openjdk bot commented Nov 22, 2022

andy-goryachev-oracle left a comment

andy-goryachev-oracle Nov 23, 2022

mstr2 Nov 23, 2022

andy-goryachev-oracle Nov 23, 2022

aghaisas left a comment

openjdk bot commented Nov 24, 2022

mstr2 commented Nov 24, 2022

openjdk bot commented Nov 24, 2022

openjdk bot commented Nov 24, 2022

przemSzer commented Apr 5, 2023 •

edited by bridgekeeper bot

Loading

8297130: ComboBox popup doesn't close after selecting value that was added with 'runLater' #956

8297130: ComboBox popup doesn't close after selecting value that was added with 'runLater' #956

Conversation

mstr2 commented Nov 19, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 19, 2022

mlbridge bot commented Nov 19, 2022

Webrevs

kevinrushforth commented Nov 22, 2022

kevinrushforth commented Nov 22, 2022

openjdk bot commented Nov 22, 2022

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle Nov 23, 2022

Choose a reason for hiding this comment

mstr2 Nov 23, 2022

Choose a reason for hiding this comment

andy-goryachev-oracle Nov 23, 2022

Choose a reason for hiding this comment

aghaisas left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 24, 2022

mstr2 commented Nov 24, 2022

openjdk bot commented Nov 24, 2022

openjdk bot commented Nov 24, 2022

przemSzer commented Apr 5, 2023 • edited by bridgekeeper bot Loading

mstr2 commented Nov 19, 2022 •

edited by openjdk bot

Loading

przemSzer commented Apr 5, 2023 •

edited by bridgekeeper bot

Loading