8379942: Some non focus-traversable Controls do not request focus when clicking on it

[Maran23]

As also discussed back then in the mailing list, there are weird issues around gaining focus when a Control is not focus traversable but got a click event.

• Some Controls do not call requestFocus() when they are not focus traversable and receive a mouse click
• It is very inconsistent which Controls do it and which do not. Sometimes, just a part of a Control will request focus, while another one will not
• Manually calling requestFocus() always works

It seems like there is a misconception between beeing not focus traversable and not requesting focus. The focus traversable property should only affect keyboard navigation really. A mouse click should always request a focus.

Check the Ticket for a reproducer with all Controls and a short list which Controls do not behave (and which do).

This PR removes the pattern that was wrongly used in some Controls.
From:

    if (getNode().isFocusTraversable()) {
        getNode().requestFocus();
    }

To:

    getNode().requestFocus();

---

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-8379942: Some non focus-traversable Controls do not request focus when clicking on it (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2106

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[Maran23]

This method was unused.

[bridgekeeper]

👋 Welcome back mhanl! 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.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 02: Full - Incremental (d235d452)
• 01: Full - Incremental (c894248a)
• 00: Full (d7deb075)

[Maran23]

Thinking about this twice, I will revert this change here.

The focus traversable is not checked in the focus dirty code:

jfx/modules/javafx.graphics/src/main/java/javafx/scene/Scene.java

Lines 2637 to 2651 in 11d65f1

	private void focusCleanup() {
	if (Scene.this.isFocusDirty()) {
	final Node oldOwner = Scene.this.getFocusOwner();
	if (oldOwner == null) {
	Scene.this.focusInitial();
	} else if (oldOwner.getScene() != Scene.this) {
	Scene.this.requestFocus(null, false);
	Scene.this.focusInitial();
	} else if (!oldOwner.isCanReceiveFocus()) {
	Scene.this.requestFocus(null, false);
	Scene.this.focusIneligible(oldOwner);
	}
	Scene.this.setFocusDirty(false);
	}
	}

But the Scene.this.focusInitial(); might change the initial focused node when the focus traversable changed from false to true.

So the scenario could be:

1. Nothing is the focus owner
2. A Node is set to focus traversable true (from false)
3. Now this code runs and focusInitial() is called to traverse to the now traversable Node

[Maran23]

/reviewers 2

[openjdk]

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

[Maran23]

This exposed a problem inside VirtualFlow. This will very likely need a localized fix there. In any case, the concept of this PR can still be reviewed already.

[mstr2]

It seems like there is a misconception between beeing not focus traversable and not requesting focus. The focus traversable property should only affect keyboard navigation really. A mouse click should always request a focus.

Yes, the semantics of focusTraversable seem to be a bit unclear. There are three ways a control can receive focus: with keyboard navigation, with a mouse click, or programmatically by invoking Node.requestFocus(). (There's also the option of directly calling setFocused(boolean), which I consider to be a defect and we should seriously consider deprecating this method.)

I don't see a whole lot of value of having a control accept focus with a mouse click, but not via keyboard navigation. What's the use case here? I would imagine that if a control is focusable with a mouse click, it should also be focusable with keyboard navigation.

However, I see quite a bit of value of having a control not accept focus at all, but still be fully functional otherwise. For example, consider a "Copy" or "Paste" button that act on the currently focused control. It's clear that such a button can never accept focus, as that would break the intended function.

With your proposed change, a button will always accept focus on click. This subtly breaks the cut/copy/paste buttons of HTMLEditor: they will now steal the focus, which removes the text cursor from the editor.

I see two ways forward here:

1. We could redefine focusTraversable to mean focusable, that is, whether it can accept focus at all.
2. If we want focusTraversable to only apply to keyboard navigation, then we need another focusable property that can be used to prevent controls from stealing focus.

I don't think we can do what you propose and force a control to always accept focus when clicked, but not accept it when keyboard-traversed. That would be a functional regression for applications that rely on the non-stealing focus behavior of buttons.

In any case, we need to discuss this on the mailing list since it affects core semantics of JavaFX.

[Maran23]

However, I see quite a bit of value of having a control not accept focus at all, but still be fully functional otherwise. For example, consider a "Copy" or "Paste" button that act on the currently focused control. It's clear that such a button can never accept focus, as that would break the intended function.

With your proposed change, a button will always accept focus on click. This subtly breaks the cut/copy/paste buttons of HTMLEditor: they will now steal the focus, which removes the text cursor from the editor.

This is a very interesting usecase.

There are three ways a control can receive focus

Yes, other frameworks usually differ between keyboard and mouse focus.

I see two ways forward here:

1. We could redefine focusTraversable to mean focusable, that is, whether it can accept focus at all.
2. If we want focusTraversable to only apply to keyboard navigation, then we need another focusable property that can be used to prevent controls from stealing focus.

This seems like a good way. The last time I wrote about those findings, I got no answer (Me and John thought this was a bug).

[Maran23]

There's also the option of directly calling setFocused(boolean), which I consider to be a defect and we should seriously consider deprecating this method

+1, agree.

[bridgekeeper]

@Maran23 This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[Maran23]

closing because this is something we probably need to do after we settled with the focus changes.