feat: consistent focus handling; remove markedNode #326
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ensure we always focus the same element when keyboard navigation works in that element. Previously the toolbox sometimes was and sometimes wasn't focused when it navigated, the workspace or group or the workspace SVG could be focused and the flyout never took the focus. With this approach the relevant container takes focus. Manage the passive focus indicator based on focus rather than markedNode state. Integrate with Gesture to determine how mouse behaviours relate to focus. This prevents issues where a gesture would cause a focus change which would close the flyout, interrupting a normal drag. In time hopefully this code can move to core where it will be more natural and less fragile. Fixes RaspberryPiFoundation#227 Fixes RaspberryPiFoundation#200 Part of RaspberryPiFoundation#229 (scrollbars still an issue) Fixes RaspberryPiFoundation#222
microbit-matt-hillsdon
requested review from
BenHenning
and removed request for
a team
rachel-fenichel
requested review from
rachel-fenichel
and removed request for
BenHenning
Contributor
Author
|
So this might conflict badly with ongoing work to rejig focus with changes in core, but I think it's worth some thought. Tagging @BenHenning where I expect some overlap. |
| * @this {Blockly.Gesture} | ||
| * @override | ||
| */ | ||
| Blockly.Gesture.prototype.doWorkspaceClick_ = function (e) { |
Contributor
Author
There was a problem hiding this comment.
This removed code has been broken for a while (underscore change) and I don't think we want this behaviour now selection is better integrated with focus.
This can interfere with in-progress gestures causing them to act in unexpected ways. We now clear the current node when focus moves away. Avoid altering the selection in cursor.draw to prevent similar poorly timed selections. If the cursor position is lost, e.g. by click in the workspace, the arrow keys now default it if needed. Tabbing to the workspace always shows a cursor. Fixes: - Click in workspace blank space now doesn't select a block on mouse down - The mini-workspace for "if" and similar now behaves correctly again (no keyboard nav still though) - Also fixes RaspberryPiFoundation#287
microbit-matt-hillsdon
force-pushed
the
rejig-focus
branch
from
d77f320 to
3727db5
Compare
Contributor
Author
|
Latest change introduced an issue returning after using field editors. Reconsidering and moving to draft. |
rachel-fenichel
approved these changes
Mar 21, 2025
This was referenced Mar 21, 2025
microbit-matt-hillsdon
pushed a commit
to microbit-matt-hillsdon/blockly-keyboard-experimentation
that referenced
this pull request
Mar 24, 2025
showing the context menu by mouse. It's only possible to fix this by reinstating the updating of curNode from the selection. We also attempt to avoid showing passive focus when using the context menu and widgets (via mouse and keyboard) for consistency with the normal Blockly experience. Fixes an issue clicking on shadow blocks which could previously show an outline around the shadow and parent block. RaspberryPiFoundation#326
microbit-matt-hillsdon
pushed a commit
to microbit-matt-hillsdon/blockly-keyboard-experimentation
that referenced
this pull request
Mar 24, 2025
PR RaspberryPiFoundation#326 introduced an issue with stale current node when showing the context menu by mouse. It's only possible to fix this by reinstating the updating of curNode from the selection. We also attempt to avoid showing passive focus when using the context menu and widgets (via mouse and keyboard) for consistency with the normal Blockly experience. Fixes an issue clicking on shadow blocks which could previously show an outline around the shadow and parent block. RaspberryPiFoundation#326
microbit-matt-hillsdon
pushed a commit
to microbit-matt-hillsdon/blockly-keyboard-experimentation
that referenced
this pull request
Mar 24, 2025
PR RaspberryPiFoundation#326 introduced an issue with stale current node when showing the context menu by mouse. It's only possible to fix this by reinstating the updating of curNode from the selection. We also attempt to avoid showing passive focus when using the context menu and widgets (via mouse and keyboard) for consistency with the normal Blockly experience. Fixes an issue clicking on shadow blocks which could previously show an outline around the shadow and parent block.
BenHenning
pushed a commit
that referenced
this pull request
Mar 27, 2025
* fix: right click menu issues PR #326 introduced an issue with stale current node when showing the context menu by mouse. It's only possible to fix this by reinstating the updating of curNode from the selection. We also attempt to avoid showing passive focus when using the context menu and widgets (via mouse and keyboard) for consistency with the normal Blockly experience. Fixes an issue clicking on shadow blocks which could previously show an outline around the shadow and parent block. * chore: remove reference to fixed bug * chore: typo in comment * fix: null node case for action menu * fix: stale curNode when clicking the workspace Due to an unintentially commented out line, clicking the workspace wouldn't clear the current node so e.g. delete would act on a block that didn't have a visual selection. Re-enabling that required revisiting shadow blocks to prevent getCurNode unexpectedly clearing based on a null selection. We now always set/use the selection for shadow blocks and take care not to move the cursor unexpectedly as it can be in two positions for a shadow block selection. --------- Co-authored-by: Grace <grace@microbit.org>
This was referenced Apr 29, 2025
BenHenning
added a commit
that referenced
this pull request
May 2, 2025
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 RaspberryPiFoundation/blockly#8938 and ending with RaspberryPiFoundation/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 RaspberryPiFoundation/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 RaspberryPiFoundation/blockly#8933 (review) is no longer an issue). - Verified #227, #200, #229, #222, and #287 have not regressed. RaspberryPiFoundation/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, 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 RaspberryPiFoundation/blockly#8941. - At some point between the latest changes to this branch and the ones in RaspberryPiFoundation/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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ensure we always focus the same element when keyboard navigation works in that element. Previously the toolbox sometimes was and sometimes wasn't focused when it navigated, the workspace group or the workspace SVG could be focused and the flyout never took the focus.
With this approach the relevant container always takes focus and we explicitly move browser focus more often and let the focus/blur handlers do their work.
Manage the passive focus indicator based on focus rather than markedNode state. But we take care not to make a selection when focus returns due to a click as that can interrupt gestures.
Integrate with Gesture to determine how mouse behaviours relate to focus. This prevents issues where a gesture would cause a focus change which would close the flyout, interrupting a normal drag. In time hopefully this code can move to core where it will be more natural and less fragile.
Fixes #227
Fixes #200
Part of #229 (scrollbars still an issue)
Fixes #222
Fixes #287
Compromises:
Future steps:
Demo: https://rejig-focus.blockly-keyboard-experimentation.pages.dev/