fix: Synchronize gestures and focus #8981
Conversation
These fixes cover a number of specific cases, plus some additional safety improvements were added for widget and drop-down divs, plus ephemeral focus management.
Addresses self-review comments.
core/clipboard/block_paster.ts
Outdated
| // Sometimes there's a delay before the block is fully created and ready for | ||
| // focusing, so wait slightly before focusing the newly pasted block. | ||
| const nodeToFocus: IFocusableNode = block; | ||
| setTimeout(() => getFocusManager().focusNode(nodeToFocus), 0); |
There was a problem hiding this comment.
This delay may be related to the bumpNeighbours behaviour.
There was a problem hiding this comment.
Hmm that seems really plausible, though I can't find a call to bump in this call path but I'm sure I'm missing large portions of what's happening here.
This does make me think that it might be nice to have a way to attach logic to run with the next rendering pass so that we can predictably run things that depend on rendering (vs. using a timeout and hoping it's correct).
There was a problem hiding this comment.
/**
* @returns A promise that resolves after the currently queued renders have
* been completed.
*/
export function finishQueuedRenders(): Promise<void> {
// If there are no queued renders, return a resolved promise so `then`
// callbacks trigger immediately.
return afterRendersPromise ? afterRendersPromise : Promise.resolve();
}
You can find examples of this pattern in the codebase. I think it will do what you want.
There was a problem hiding this comment.
After looking at this again: please switch to using finishQueuedRenders before merging. It is a stronger guarantee on timing.
There was a problem hiding this comment.
I evidently didn't look hard enough for this exact mechanism. Nice suggestion--I updated the code and tested pasting again. Selection seems to correctly select the most recently pasted block as intended.
core/dragging/dragger.ts
Outdated
| @@ -129,6 +131,12 @@ export class Dragger implements IDragger { | |||
| root.dispose(); | |||
| } | |||
| eventUtils.setGroup(false); | |||
|
|
|||
| if (!wouldDelete && isFocusableNode(this.draggable)) { | |||
| // Ensure focusable nodes that have finished dragging (but aren't being) | |||
There was a problem hiding this comment.
"(but aren't being) deleted" -> "(but aren't being deleted)"
There was a problem hiding this comment.
Nice catch--fixed!
Addresses reviewer comment.
I can re-run these steps with a video recording if that's helpful, but here's what I did: Core playground
Keyboard nav environment
There may be other things I've tried, but I think this covers the majority. If there's anything else that you think is worth testing or re-testing, please let me know. |
|
PTAL @rachel-fenichel to the latest changes & my comment above. |
|
Thanks for the list--that gives us things to turn into automated tests down the line, and means that you can ask other folks to reverify for you if issues pop up. |
|
Enabling auto-merge. Please only approve if you're happy with this being merged. |
This addresses a review comment.
|
PTAL again @rachel-fenichel. I think everything's been addressed so far. |
## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #8994 ### Proposed Changes This removes an error that was previously thrown by `FocusManager` when attempting to focus an invalid node (such as one that's been removed from its parent). ### Reason for Changes #8994 (comment) goes into more detail. While this error did cover legitimately wrong cases to try and focus things (and helped to catch some real problems), fixing this 'properly' may become a leaky boat problem where we have to track down every possible asynchronous scenario that could produce such a case. One class of this is ephemeral focus which had robustness improvements itself in #8981 that, by effect, caused this issue in the first place. Holistically fixing this with enforced API contracts alone isn't simple due to the nature of how these components interact. This change ensures that there's a sane default to fall back on if an invalid node is passed in. Note that `FocusManager` was designed specifically to disallow defocusing a node (since fallbacks can get messy and introduce unpredictable user experiences), and this sort of allows that now. However, this seems like a reasonable approach as it defaults to the behavior when focusing a tree explicitly which allows the tree to fallback to a more suitable default (such as the first item to select in the toolbox for that particular tree). In many cases this will default back to the tree's root node (such as the workspace root group) since sometimes the removed node is still the "last focused node" of the tree (and is considered valid for the purpose of determining a fallback; tree implementations could further specialize by checking whether that node is still valid). ### Test Coverage Some new tests were added to cover this case, but more may be useful to add as part of #8910. ### Documentation No documentation needs to be added or updated as part of this (beyond code documentation changes). ### Additional Information This original issue was found by @RoboErikG when testing #8995. I also verified this against the keyboard navigation plugin repository.
The basics
The details
Resolves
Fixes #8952
Fixes #8950
Fixes #8971
Fixes a bunch of other keyboard + mouse synchronization issues found during the testing discussed in google/blockly-keyboard-experimentation#482 (comment).
Proposed Changes
This introduces a number of changes to:
Reason for Changes
There are some ephemeral focus issues that can come up with certain actions (like clicking or dragging) don't properly change focus. Beyond that, some users will likely mix clicking and keyboard navigation, so it's essential that focus and selection state stay in sync when switching between these two types of navigation modalities.
Other changes:
BlockSvgself-managing its selection state based on focus, so focusing is sufficient. I've also centralized some of the focusing calls (such as putting one inbringBlockToFrontto ensure focusing happens after potential DOM changes).BlockSvg's ownbringToFronthas been updated to automatically restore focus after the operation completes. SincebringToFrontcan always result in focus loss, this provides robustness to ensure focus is restored.doWorkspaceClickfunction. As far as I can tell, this won't be needed anymore since blocks self-regulate their selection state now.FocusManagerhas a new extra check for ephemeral focus to catch a specific class of failure where the browser takes away focus immediately after it's returned. This can happen if there are delay timing situations (like animations) which result in a focused node being deleted (which then results in the document body receiving focus). The robustness check is possibly not needed, but it help discover the animation issue with drop-down div and shows some promise for helping to fix the variables-closing-flyout problem.Some caveats:
focusNodebeing called more than it needs to be.FocusManagerdoes avoid callingfocus()more than once for the same node, but it's possible for focus state to switch between multiple nodes or elements even for a single gesture (for example, due to bringing the block to the front causing a DOM refresh). While the eventual consistency nature of the manager means this isn't a real problem, it may cause problems with screen reader output 8000 in the future and warrant another pass at reducingfocusNodecalls (particularly for gestures and the click event pipeline).Test Coverage
This PR is largely relying on existing tests for regression verification, though no new tests have been added for the specific regression cases.
#8910 is tracking improving
FocusManagertests which could include some cases for the new ephemeral focus improvements.#8915 is tracking general accessibility testing which could include adding tests for these specific regression cases.
Documentation
No new documentation is expected to be needed here.
Additional Information
These changes originate from both #8875 and from a branch @rachel-fenichel created to investigate some of the failures this PR addresses. These changes have also been verified against both Core's playground and the keyboard navigation plugin's test environment.