Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8343956: Focus delegation API #1632

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mstr2
Copy link
Collaborator

@mstr2 mstr2 commented Nov 9, 2024

Implementation of focus delegation.


Progress

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

Issue

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1632

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 9, 2024

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

@openjdk
Copy link

openjdk bot commented Nov 9, 2024

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

@mstr2 mstr2 force-pushed the feature/focus-delegation branch from c6a0c3b to d110710 Compare November 9, 2024 07:42
@mstr2 mstr2 changed the title Focus delegation API 8343956: Focus delegation API Nov 11, 2024
@beldenfox
Copy link
Contributor

PopupWindows have to solve a similar problem; multiple nodes can receive key events and they all act as if they have focus. This gets (mildly) complicated when dealing with input methods. There may be multiple focused nodes but there can only be one caret so you have to have a mechanism to determine which node will respond to input method requests. You also have to determine when to enable and disable IM events at the OS level. Spinners have never dealt with input methods but ComboBoxes do.

I've submitted PR #1634 to get input methods working in popups. If you want input methods to work with focus delegation you would need to do similar bookkeeping.

I noticed that PopupWindows use EventRedirectors for forwarding messages. I didn't dig into the EventRedirector implementation but it is an existing class that seems to be tackling the same problem as this PR.

# Conflicts:
#	modules/javafx.controls/src/main/java/javafx/scene/control/Spinner.java
#	modules/javafx.controls/src/main/java/javafx/scene/control/skin/SpinnerSkin.java
@hjohn
Copy link
Collaborator

hjohn commented Jan 3, 2025

This looks really good. I'm wondering if this could be simplified further. Specifically, I think the hoistFocus flag and manual management of the focus delegate may not be needed.

It seems to me that a Control could share some similarities with a Scene, in that Control has properties that track a focus owner (similar to focus delegate). In effect, a Control is a focus root similar to scene. When a Scene receives focus, it determines the best Node to "delegate" focus to; similarly, when a Control receives focus, it determines which skin control should be focused. The normal focus rules should do the right thing here and for example select the TextField of a Spinner as the delegate automatically (some children may need to be marked as not focusable to guide the auto selection, but this is an already existing standard mechanism).

When determining where to send events, if the target is a focus root, it queries its focus owner (or focus delegate) and extends the event to that target. If that target is also a focus root, the process repeats.

The request focus function should operate differently as well. It should look for the closest focus root (a Control or Scene) and call the appropriate request focus function on the root it finds. If that root is Scene, everything works as usual. If it is another focus root like a Control, Control can determine the best way to focus one of its child nodes (likely you can just apply a normal search for an eligible focusable control for this).

Perhaps the focus root functionality can be captured in an interface that both Scene and Control implement. I think it would need to specify a requestFocus method and focusOwnerProperty. This interface would then replace the focusScope flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants