feat: Update line cursor to use focus manager #8941
Conversation
This adds basic support for WorkspaceSvg being focusable via its blocks, though fields and connections are not yet supported.
This introduces the fundamental support needed to ensure that both toolboxes and flyouts are focusable using FocusManager.
…-workspace-focusable
This ensures that fields with editors properly signal when the editor is shown/hidden, and introduces show/hide callbacks that can be overridden to properly trigger ephemeral focus.
Note that this doesn't yet contain all of the changes needed in order to ensure that this works correctly.
…-workspace-focusable
Addresses review comment.
Addresses reviewer comment.
This is a more general purpose alternative to making fields explicitly focusable (at least making them hook up properly to ephemeral focus).
With widget and drop down divs focusable directly, all field editors should automatically inherit this benefit.
Also, remove unnecessary casting.
There's a lot of clean-up and simplification work needed yet.
Addresses a reviewer comment.
|
PTAL @rachel-fenichel. Note that this is not mergeable yet, but it's at least in a good state for review. There's a 'current gaps' section in the PR description that explains the current items that will need to fixed before this can be merged. Aside from that, there may be additional changes required to Core as part of testing google/blockly-keyboard-experimentation#482 which I'll likely pull into this PR to avoid another PR to be created. However, if you have strong opinions about getting this merged sooner (to reduce conflicts with @gonfunko's changes), I can pull out the variables change in this PR and focus on fixing the tests and merging it. |
These will be replaced with better styling in the keyboard navigation plugin.
This fixes rendering for connection tests, switches out a now-contrived output connection test, and revises FocusManager state such that a unique FocusManager is created for every individual test to avoid cross-test state bleeding. Focus state can still bleed, unfortunately, but FocusManager generally guarantees eventual consistency.
Conflicts: core/css.ts
Workspace clean-up must happen before resetting FocusManager otherwise it can't be properly unregistered.
|
Mocha's passing, but advanced compilation in browser isn't. Digging. |
Besides the fact that this wasn't a complete solution, switching the workspace import to be non-type caused a circular dependency which caused the advanced compilation tests to fail.
|
Turns out the failure was due to importing |
Dismissing so that I can get one more pass on the PR with auto-merge enabled.
|
PTAL @gonfunko for the latest few changes or @rachel-fenichel. One approval will auto-merge the PR. I want to get a double check on the test changes since it affects every Mocha test (though they are, fortunately, passing). |
There was a problem hiding this comment.
LGTM with a potential follow-up down the line: as I understand it, you've changed the test code so that the focus manager won't keep state between tests. There is still a potential problem when a workspace has focus, the workspace is deleted, and the focus manager tries to mark the (dead) node as passively focused. I think you're going to need to fix that case in focus manager. If that's correct, please file a bug for it.
I've filed #8957 for this case. I'm...not sure that this scenario is actually possible outside of tests (at least realistically). I definitely saw the same stack traces when running the tests, but I think the test environment creates a situation that's very different from a real production use case, specifically in how it manages its DOM. My tests so far in trying to deliberately break |
Fixes #142 Fixes #481 This removes nearly all custom focus/blur management and instead leverages `FocusManager` as the source of truth and primary system for focusing/selecting elements. This largely continues the work in Core starting with google/blockly#8938 and ending with google/blockly#8941 in order to bring basic parity in the keyboard navigation plugin using `FocusManager`. Specifically: - This replaces all cases where explicit focus/opening logic is needed with `focusNode`/`focusTree` calls via `FocusManager`. - `Navigation` now infers state based on what's currently focused rather than tracking it separately. - All of the existing focus/blur response logic has been made redundant due to the inferred navigation mode and focus being tightly coupled with the components themselves. - The gesture monkey patch seems no longer necessary since, as far as I can tell, focus gets forced back to where it's supposed to be at specific lifecycle moments (which helps undo many of the problems caused by `WorkspaceSvg`'s `markFocused`). - The passive indicator is no longer necessary now that the `FocusManager`-driven `blocklyPassiveFocus` CSS class exists for styling. - Note that there's a lot of automatic state synchronizing that has been purposely pushed into Core to simplify the navigation pieces, particularly: - Blocks will auto-select/unselect themselves based on focus. - `Toolbox` will automatically synchronize its selected item state with its item that has active focus. - `Toolbox` and `Flyout` cooperate for automatically closing the `Flyout` at the correct times. - The new CSS classes are not yet final as the overall strategy for indicating active/passive focus isn't final. There's also additional work needed to ensure selection and active focus can be properly stylized as a combined state, but this is blocked on the completion of google/blockly#8942. - `Toolbox` navigation was moved to using its methods directly instead of its event since selection logic now needs to be paired with `focusNode`. - There are a number of changes in this PR that could probably go into Core, but these can wait for a future PR as they shouldn't be API breaking. Some of the changes include: - `Toolbox` item selection could auto-focus within `Toolbox` itself, or this could be done as part of `Toolbox`-specific navigation. - `Flyout` focus could automatically focus its first item if nothing is currently focused. - For regression testing, the following has been checked: - 'T' and 'I' do work as expected (ensuring google/blockly#8933 (review) is no longer an issue). - Verified #227, #200, #229, #222, and #287 have not regressed. google/blockly-samples#2498 shouldn't be possible anymore (even with the fix) due to ephemeral focus, and we seem to be matching parity with #326 per the `handleFocus*()` functions, though there may be cases from `handleFocusWorkspace` yet to check. The test updates were necessary because 9C88 , at present, clicking does not force focus which means the tests need to rely purely on keys for navigation. I think these changes are actually an improvement since it closer aligns them with keyboard-only usage. **Current gaps** that may need resolution prior to this PR being mergeable: - There's a regression with clicking to create a variable closing the flyout that I'm still investigating. This is detailed more in google/blockly#8941. - At some point between the latest changes to this branch and the ones in google/blockly#8941 block movement regressed slightly: finishing a movement gesture now defocuses the workspace entirely (even though this was working earlier). - There are a lot of inconsistencies between the focus and selection styling before this PR and with this PR. It's unclear how much of this requires actual resolution. - Much more testing is needed to understand gaps as this changes a significant amount of state handling for the plugin.
The basics
The details
Resolves
Fixes #8940
Fixes #8954
Fixes #8955
Proposed Changes
This updates
LineCursorto useFocusManagerrather than selection (principally) as the source of truth.Reason for Changes
Ensuring that keyboard navigation works correctly with eventual screen reader support requires ensuring that ever navigated component is focused, and this is primarily what
FocusManagerhas been designed to do. Since these nodes are already focused,FocusManagercan be used as the primary source of truth for determining where the user currently has navigated, and where to go next.Previously,
LineCursorrelied on selection for this purpose, but selection is now automatically updated (for blocks) using focus-controlledfocusandblurcallbacks. Note that the cursor will still fall back to synchronizing with selection state, though this will be removed once the remaining work to eliminateMarkerSvghas concluded (which requires further consideration on the keyboard navigation side viz-a-viz styling and CSS decisions) and once mouse clicks are synchronized with focus management.Note that the changes in this PR are closely tied to google/blockly-keyboard-experimentation#482 as both are necessary in order for the keyboard navigation plugin to correctly work with
FocusManager.Some other noteworthy changes:
FocusableTreeTraverseris needed by the keyboard navigation plugin (in feat: Use FocusManager for navigation & state management blockly-keyboard-experimentation#482) so it's now being exported.FocusManagerhad one bug that's now patched and tested in this PR: it didn't handle the case of the browser completely forcing focus loss. It would continue to maintain active focus even though no tracked elements now hold focus. One such case is the element being deleted, but there are other cases where this can happen (such as with dialog prompts).FocusManagerhad some issues from feat!: Introduce new focus tree/node functions. #8909 wherein it would overeagerly call tree focus callbacks and slightly mismanage the passive node. Since tests haven't yet been added for these lifecycle callbacks, these cases weren't originally caught (per Shore up testing coverage for FocusManager #8910).FocusManagerwas updated to move the tracked manager into a static function so that it can be replaced in tests. This was done to facilitate changes to setup_teardown.js to ensure that a uniqueFocusManagerexists per-test. It's possible for DOM focus state to still bleed across tests, butFocusManagerlargely guarantees eventual consistency. This change prevents a class of focus errors from being possible when running tests.Current gaps to be fixed after this PR is merged:
FocusManager, and the flyout losing focus will force it closed. There was a fix introduced in this PR to fix it for keyboard navigation, but fails for clicks because the flyout never receives focus when the create variable button is clicked. It also caused the advanced compilation tests to fail due to a subtle circular dependency from importingWorkspaceSvgdirectly rather than its type.Test Coverage
No new non-
FocusManagertests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of #8915.Some new
FocusManagertests were added, but more are still needed and this is tracked as part of #8910.Documentation
No new documentation should be needed for these changes.
Additional Information
This includes changes that have been pulled from #8875.