8291502: Mouse or touch presses on a non-focusable region don't clear the focusVisible flag of the current focus owner

[mstr2]

The focusVisible flag is only set on a node when it acquires input focus using a keyboard interaction, and it is cleared by mouse and touch interactions.

It is not necessary for a node to lose input focus in order to lose focusVisible. Currently, clicking on a region of the scene that does not contain a focusable node does not clear the focusVisible flag.

Detecting clicks or touches that should clear focusVisible can be achieved by adding an event filter for MOUSE_PRESSED and TOUCH_PRESSED to the Scene.

There is an additional noteworthy change with this PR:
Adding event filters to Scene causes many tests to fail due to a bug in MouseEventFirer that was identified in 8253769. Previously, mouse events generated by MouseEventFirer skipped a code path that causes test failures due to incorrect coordinates. With the added event filters, the code path for mouse events is slightly different, exposing the incorrect coordinates and causing tests to fail.
This problem can be solved by making the "alternative" MouseEvent generation path the default path.

Note: Since this is a follow-up fix for 8268225, I'm targeting this fix for jfx19.

---

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-8291502: Mouse or touch presses on a non-focusable region don't clear the focusVisible flag of the current focus owner

Reviewers

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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 852

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

Using diff file

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

[bridgekeeper]

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

[mlbridge]

Webrevs

• 02: Full - Incremental (56d2d6ea)
• 01: Full - Incremental (e9d5a822)
• 00: Full (9d0f98f8)

[kevinrushforth]

This will need to be very well-tested to ensure that there are no regressions. As part of the review, we will evaluate whether it is a safe enough fix to go into 19.

/reviewers 2

[openjdk]

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

[mstr2]

In addition to automated testing, I also manually tested this change by building and running SceneBuilder with it.
As far as I can tell, there's no regression in functionality.

The behavior of the feature after this PR is applied mostly resembles how Windows 10/11, Chrome, and Firefox handle visible focus indicators. There are some very minor differences, which can be addressed later (for example, clicking on scroll bars doesn't seem to count as a mouse interaction Windows/Chrome/Firefox, where it does in this iteration of the focusVisible feature).

[kevinrushforth]

The changes around KeyHandler might seem to be excessive for the scope of this PR.

They do indeed.

I think that moving the focus-related functionality next to the other focus functions in the Scene class is a safe and straightforward change, and it makes it easier to work with the code in the future.

Be that as it may, this sort of refactoring is discouraged, especially in the case of a fix you want to get in late in the release (we have 3 days left before the RDP2 deadline of JavaFX 19). This will cause extra time for the reviewers and could jeopardize getting it into the release. It would have been better to save this change for a future bug fix.

[mstr2]

Be that as it may, this sort of refactoring is discouraged, especially in the case of a fix you want to get in late in the release (we have 3 days left before the RDP2 deadline of JavaFX 19). This will cause extra time for the reviewers and could jeopardize getting it into the release. It would have been better to save this change for a future bug fix.

I've reverted most of the changes around KeyHandler.

[kevinrushforth]

I've reverted most of the changes around KeyHandler.

Thank you. That will make this much easier to review.

[kevinrushforth]

This looks good, with a possible follow-up item to clarify the docs. I think it would be worth updating the spec to say that focusVisible is cleared on a MOUSE_PRESSED or TOUCH_PRESSED event (in addition to the other events that can clear it). I recommend doing this in a follow-up bug, so it doesn't derail getting this bug fix in.

Since this would be a doc-only change, it could be done during RDP2 of JavaFX 19 (or even deferred to JavaFX 20, but it seems best to shoot for 19). Either way, it will need a CSR.

[kevinrushforth]

As a possible additional follow-on bug for the original feature, in a future release, is that navigating using the Windows Narrator's focus traversal keys doesn't set focusVisible. This is independent of this bug (this PR doesn't change that behavior).

[aghaisas]

Looks good to me.
I have identified potential cleanup that may be required. It can be done as part of JDK-8253769

[aghaisas]

If the alternative path is made the default then this class needs some cleanup. That can be done as part of JDK-8253769. Request you to add a comment in JBS on JDK-8253769 to clean up the unused portion of MouseEventFirer.java

[openjdk]

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

8291502: Mouse or touch presses on a non-focusable region don't clear the focusVisible flag of the current focus owner

Reviewed-by: kcr, 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 1 new commit pushed to the jfx19 branch:

• 8fa8919: 8291588: Update boot JDK to 18.0.2

Please see this link for an up-to-date comparison between the source branch of this pull request and the jfx19 branch.
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 jfx19 branch, type /integrate in a new comment.

[mstr2]

/integrate

[openjdk]

Going to push as commit 5febaca.
Since your change was applied there has been 1 commit pushed to the jfx19 branch:

• 8fa8919: 8291588: Update boot JDK to 18.0.2

Your commit was automatically rebased without conflicts.

[openjdk]

@mstr2 Pushed as commit 5febaca.

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