jfx
jfx copied to clipboard
openjdk
8087863: Mac: "Select All" within ListView/TreeView is handled differently depending on the useSystemMenuBar value
macOS processes a shortcut key like Cmd+A in two phases. In the first phase it’s shopped around as a “key equivalent”. If it’s not consumed as a key equivalent it enters the second phase and processed as a normal keyDown event. Among other things the key equivalent phase ensures the shortcut will be seen by the system menu bar before being treated as a keyDown. This is the opposite of how JavaFX works; it expects a key event to be fired at the current focus node which gets first crack at the event before it works its way out to the menu bar.
We can’t really opt out of the key equivalent phase but we can get the event before the system menu bar does. Our implementation of performKeyEquivalent pushes the event through the JavaFX scene graph but has no way of knowing if the scene graph consumed it. The result is that a consumed event is always handed to the system menu bar where it can also trigger a menu item.
This PR introduces a variant of notifyKey that returns a boolean indicating whether the event was consumed or not. If the event was consumed performKeyEquivalent doesn’t allow it to continue on to the system menu bar.
I’m trying to fix this old, old problem because I’ve seen at least one JavaFX app tie itself up in knots trying to work around this. Despite the number of files being touched it is not a deep fix; there’s just a boolean return value that needs to be plumbed through multiple layers.
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)
Issue
- JDK-8087863: Mac: "Select All" within ListView/TreeView is handled differently depending on the useSystemMenuBar value (Bug - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1528/head:pull/1528
$ git checkout pull/1528
Update a local copy of the PR:
$ git checkout pull/1528
$ git pull https://git.openjdk.org/jfx.git pull/1528/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1528
View PR using the GUI difftool:
$ git pr show -t 1528
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1528.diff
Webrev
:wave: 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@beldenfox 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:
8087863: Mac: "Select All" within ListView/TreeView is handled differently depending on the useSystemMenuBar value
Reviewed-by: kcr, angorya
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 2 new commits pushed to the master branch:
- 1637f77533c11c435a7013e2ad940481beaf8d9e: 8341418: Prism/es2 DrawableInfo is never freed (leak)
- 23e25954f2cbd8dda9afea7f257d22156233894e: 8339068: [Linux] NPE: Cannot read field "firstFont" because "
" is null
Please see this link for an up-to-date comparison between the source branch of this pull request and the master 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 master branch, type /integrate in a new comment.
Webrevs
- 03: Full - Incremental (447a70a9)
- 02: Full - Incremental (d1947a5c)
- 01: Full - Incremental (592b317e)
- 00: Full (06d9422c)
@andy-goryachev-oracle 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).
Thinking out loud for a moment: wouldn't it be better to use a kind of EventFilter specifically for the shortcuts, before the normal dispatching happens?
Thinking out loud for a moment: wouldn't it be better to use a kind of EventFilter specifically for the shortcuts, before the normal dispatching happens?
I think I see where you’re going here. We could send the KeyEvent into the scene graph and then catch it on the back end to be processed by the system menu bar. If the KeyEvent reaches the KeyboardAcceleratorHandler we know that it’s time for the menu bar to process it.
I’ve thought about this but it would tricky to implement. Currently we only send a subset of key events to the system menu bar (the one’s that are delivered to us via performKeyEquivalent). Once a KeyEvent is injected into the scene graph we lose track of whether it was generated by performKeyEquivalent or keyDown. We could also see KeyEvents that weren’t created by glass (anyone can create one). We would have to find a way to sort this out on the back end.
What truly makes me nervous is how we would have to alter the normal performKeyEquivalent flow. Usually NSApplication controls the order in which key equivalents are presented to different components (windows, views, the system menu bar, etc.) To implement this alternative approach we would have to short circuit the NSApplication flow and present events to the system menu bar manually.
This PR is the most conservative one I could come up with since it doesn’t change the order of event processing and fits into the normal performKeyEquivalent flow that NSApplication expects. It just avoids double-processing of the same event.
I'm marking this PR as draft for now. Turns out we're relying on double-processing of key events to make IM's work.
macOS sends a navigation key to us via performKeyEquivalent. That includes Home, End, and any arrow key. Currently we let the scene graph act on the event first and if a TextInput control has the focus the event is usually consumed. If we stop processing the event (and we should since it's consumed) it will never reach the IM.
In other words, the current system relies on having the same navigation key event sent to both performKeyEquivalent and keyDown. It gets acted on twice, once by the TextInput control and again by the IM. This is wrong but prioritizing the TextInput control over the IM is more wrong.
is there a compelling reason we are not using Event::isConsumed?
is there a compelling reason we are not using Event::isConsumed?
As detailed in the discussion of PR #1523 the isConsumed flag doesn't work because the consumed event is a copy of the original one. The reliable way is to compare the result of the dispatch against null.
Maybe this case should also propagate ::isConsumed flag similarly to #1523
It looks to me we are trying to invent band-aids to what appears to be a design problem (cloning the events regardless of their ::isConsumed state)
Taking this out of Draft. To ensure consistent handling of the IM both keyDown: and performKeyEquivalent: use the same routine. There's a check to ensure that the same event doesn't get processed twice which mimics an older check that used to be in the view delegate.
@beldenfox This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
so here is an unrelated question - if we assume that the controls should only consume the event if it effected a change in said control, what should command-A do in case of a focused empty ListView? should it consume the vent or not?
so here is an unrelated question - if we assume that the controls should only consume the event if it effected a change in said control, what should command-A do in case of a focused empty ListView? should it consume the vent or not?
If the focus is clearly in the list view I think it would be surprising if Cmd+A was acted on further up the scene graph. Most of the discussion of event consumption has been around keys like ESCAPE where the user sometimes expects it to act locally (like canceling editing) and sometimes further out (like dismissing a dialog). I don't think that expectation applies to Cmd+A.
On the other hand a user might not perceive an empty list view as being the center of attention. It's not like an empty text field that has a blinking cursor.
Going to push as commit ec60af479b824da521b14522c07d814e08dea3e5.
Since your change was applied there have been 2 commits pushed to the master branch:
- 1637f77533c11c435a7013e2ad940481beaf8d9e: 8341418: Prism/es2 DrawableInfo is never freed (leak)
- 23e25954f2cbd8dda9afea7f257d22156233894e: 8339068: [Linux] NPE: Cannot read field "firstFont" because "
" is null
Your commit was automatically rebased without conflicts.
@beldenfox Pushed as commit ec60af479b824da521b14522c07d814e08dea3e5.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.