feat: ephemeral focus public getter, use in shortcut precondition #9110
Conversation
| !focused.workspace.isFlyout && | ||
| focused.isDeletable() | ||
| focused.isDeletable() && | ||
| !getFocusManager().ephemeralFocusTaken() |
There was a problem hiding this comment.
Do we ever want shortcuts to work when ephemeral focus is taken? I'm wondering if it might make sense to filter this out at a higher level than this since we presumably will otherwise need to always check this for any shortcut precondition.
Understandable if we want to expose this as a customization for applications, though.
Edit: Ah, just noticed the fact that escape is already excluded from this. Perhaps we could make it an explicit configuration (that defaults to 'on'), but that's probably too heavy-handed.
There was a problem hiding this comment.
I think it might make sense to filter it out at a higher level, but it would be a breaking change. This is especially evident by the fact that escape should be excluded, so it's plausible that someone has other keyboard shortcuts they would want to take effect. I gave an example of one with the pitch field editor in the PR description (though I probably wouldn't implement that feature with keyboard shortcuts if it were me, it's plausible someone is)
No concerns with changing the name of the property--you make a good point on there being a potentially incorrect implication on what actually holds the focus. |
|
@maribethb We tested this earlier and it generally looks good. The only thing I've found is that for some reason, if you push 'M' while a dropdown is open, the passive focus ring around the field that triggered the dropdown goes away. |
|
@microbit-robert super weird! I think this is a pre-existing bug because when you just activate move mode normally (before this change), the focus indicator on the workspace goes away while move mode is active. I didn't dig into the cause of that when I noticed it while testing this. However I don't have any explanation for why it would occur if move mode doesn't even get activated. I'll file an issue and investigate separately as I don't want that to block this. issue: RaspberryPiFoundation/blockly-keyboard-experimentation#569 |
4 participants
The basics
The details
Resolves
Required for #9101
Proposed Changes
ephemeralFocusTakeninstead of matching thecurrentlyHoldsEphemeralFocusbecause I feel like the latter implies that the focus manager is holding it, when really, it's some other object that has taken it. let me know if you hate this though. the name of the property is private so we could probably also change that. cc @BenHenning for feedback on this.Reason for Changes
It's confusing when a keyboard shortcut acts in the "background" of an open field editor. Most shortcuts shouldn't do that. Specific widget divs or dropdown div editors can always bind their own key listeners, or bind a keyboard shortcut that doesn't have this in the precondition. For example, you could imagine the pitch field editor using the up/down arrow keys to move the note up and down the staff. In that case, you would definitely want to disable the keyboard-navigation behavior of the arrow keys while that field editor is open (so you don't navigate between blocks) but you could register your own shortcut that only applies when that editor is open, or just register keypress listeners directly on the field editor.
Test Coverage
Will add webdriver tests when I make the corresponding change in kbe to add this to that plugin's preconditions.
Documentation
We should probably include information about this when explaining ephemeral focus.
Additional Information
cc @RoboErikG this precondition should be included in the cut/copy/paste preconditions, so whichever of us merges second will have to include this in the merge conflict cleanup