8354539: [macOS] ComboBox and Spinner disable system menu bar shortcuts

[beldenfox]

This is a DRAFT PR for discussion purposes only.

This code implements a new Control subclass, FocusDelegatingControl. A FocusDelegatingControl can delegate keyboard related events to a descendant node. In this PR ComboBox is derived from FocusDelegatingControl so it can delegate keyboard events to its TextField.

A FocusDelegatingControl can set a delegate by calling:

protected void setFocusDelegate(Node node);

It can control which KeyEvents are sent to the delegate by overriding:

protected boolean shouldDelegateEvent(KeyEvent event);

ComboBox wants to handle ENTER and ESCAPE directly and send all other KeyEvents to the delegate.

The FocusDelegatingControl class handles all the rest. The code works by overriding buildEventDispatchChain. By default it builds a dispatch chain which terminates at the delegating control. The first dispatcher in the chain checks whether the event should be sent to the delegate; if so it builds a new chain which terminates at the delegate and dispatches the event to that chain instead. Any given dispatcher will only see one event pass through.

If the delegate handles InputMethodRequests or InputMethodEvents they are automatically forwarded to the delegate.

When an event is delegated it’s target will initially be set to the delegating control (e.g. the ComboBox). By the time that event reaches the delegate the target will have been updated to point to the delegate. I’m not entirely sure that’s necessary but I wanted to verify that it could be done.

It should be possible for the delegate to be another FocusDelegatingControl but I haven’t tested that.

The code doesn’t handle anything related to setting the focused property on the delegate or ensuring that requestFocus on the delegate is redirected to the delegating control. This PR still relies on the FakeFocusTextField for that.

Again, this is a draft PR. The way ComboBox implements shouldDelegateEvent is wrong; I just hard-coded the answer in the ComboBox base class. In a real implementation the ComboBox control would be asking its skin which events it wants to delegate but that involves expanding the Skin API and I didn’t want to tackle that. This PR fixes Spinner but not by making it a FocusDelegatingControl since that would require completely re-writing the way it handles ENTER.

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

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

• JDK-8354539: [macOS] ComboBox and Spinner disable system menu bar shortcuts (Bug - P3) ⚠️ Issue is already resolved. Consider making this a "backport pull request" by setting the PR title to Backport <hash> with the hash of the original commit. See Backports.

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2181

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

Using diff file

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

[bridgekeeper]

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

[andy-goryachev-oracle]

I do not like this idea at all. It seems that if we want to handle a subset of events differently, we should do it through the skin and/or input map, and not invent a whole new Control subclass.

I feel like we need to hash out the focus delegation concept first.

[beldenfox]

We could fold all the logic in FocusDelegatingControl into Control. If no delegate is set it should behave the same.

In theory this could be folded into Node but I'm not convinced a node-level focus delegation framework is the way to go. It might make more sense to roll a custom solution in Control that meets its specialized needs. For example with some tweaking this code could route events through the skin directly to the behavior/input map without filters or handlers.

You are not going to be able to fix this with skins. They can only use filters and handlers and those are not up to the task. That's how ComboBox and Spinner got into such a messy state to begin with.

[andy-goryachev-oracle]

The way I see it, Control is just a façade that provides useful APIs to the application. The visuals are provided by the Skin. The behavior, since it's intimately linked to the particular visual surfaces, is a part of the Skin.

Control does not offer anything specific when it comes to focus - it neither declares new properties nor has any specific APIs related to focus. The Node class declares the focused property, the Scene knows which Node is the focus owner - notice the focus owner is a Node and not a Control.

The focus processing is done by the behavior, which is a part of a particular skin (a different skins might have completely different behavior in relation to focus or focus-related user input).

Based on that, I would say placing focus delegation thing in the Control is wrong. It's the responsibility of the skin to control the focus, possibly delegating it to one (or more than one) Nodes within it. This is the reason why I like the idea expressed in the original PR where the events are fired toward the internal Node (which happens to be a Control, by the way), and these events do not traverse the hierarchy by design since they are not external.

[beldenfox]

It's not possible to isolate delegation entirely to the skin. The focusOwner is responsible for handling input method requests and that can't be delegated using filters. ComboBox handles this but Spinner doesn't so IM requests aren't working and that affects more than just Asian-language input.

ComboBox and Spinner have a lot of constraints on how they handle the ENTER and ESCAPE keys. They've been dealing with those constraints using ad-hoc methods and it's created a bit of a mess. Anyone designing a focus delegation framework should start with a detailed analysis of how these controls currently work and what it will take to clean up the implementation. If the goal to avoid firing a second event I don't think it can be done entirely within the skin.

(The thumbnail summary: both controls want to act on ENTER and ESCAPE without consuming the events. These events need to bubble up to the scene unconsumed so they can trigger default buttons in dialogs. This limits how event filters and handlers can work since the only way to keep an event from progressing through the dispatch chain is to consume it and, again, these events shouldn't normally be consumed. Firing a second event is one way to get around this; the second event is consumed and the original is allowed to bubble up.)

[andy-goryachev-oracle]

I don't understand this argument. The skin can decide whether to consume an event based on its internal state, no?

[beldenfox]

Imagine a focus delegation framework that fires all KeyEvents at the delegate (the FakeFocusTextField). The ComboBox skin wants to deal with ENTER and not send it on to the FakeFocusTextField. The only way to do this in a filter is to consume the event so it stops traveling down the event dispatch chain. But if you consume the event it also won't make its way up to the scene to trigger dialog buttons. Which is a bug.

These skins want to deal with some key events directly and pass the rest on to the TextField. They're currently doing this by being selective about event re-firing. If the goal is to get rid of event re-firing there needs to be an alternative solution in whatever focus delegation framework we come up with.

That's a requirement that should be spelled out somewhere. The real problem is that we don't have a list of such requirements to refer to when evaluating potential approaches to focus delegation.

[andy-goryachev-oracle]

In this example, the skin should have added a listener to its inner FakeFocusTextField then. AND there should be a way to send an event directly to the Node without filters and bubbling up, so the skin can control the inner parts directly (or have the controls expose the methods to avoid sending events altogether).

[openjdk]

@beldenfox this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout focusdelegatingctrl
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push