feat: add toolbox and flyout navigation to the optional jump shortcuts - #10215
feat: add toolbox and flyout navigation to the optional jump shortcuts#10215MannXo wants to merge 2 commits into
Conversation
Page Up, Page Down, Ctrl/Cmd + Home and Ctrl/Cmd + End only acted on workspace blocks. In the toolbox they moved focus out of the toolbox and into the workspace, and in the flyout the page shortcuts did nothing. Ctrl/Cmd + Home and End now move to the first and last item of whichever of the workspace, toolbox or flyout holds focus. Page Up and Page Down move focus to the first or last currently visible item of the toolbox or flyout and scroll it to the edge of the viewport it moved towards, so that nothing between the old and new position is passed over unseen. No new key bindings are introduced, and everything stays inside the opt-in registerNavigationShortcuts bundle. Fixes RaspberryPiFoundation#10187
There was a problem hiding this comment.
Welcome! It looks like this is your first pull request in Blockly, so here are a couple of tips:
- You can find tips about contributing to Blockly and how to validate your changes on our developer site.
- We use conventional commits to make versioning the package easier. Make sure your commit message is in the proper format or learn how to fix it.
- If any of the other checks on this PR fail, you can click on them to learn why. It might be that your change caused a test failure, or that you need to double-check the style guide.
Thank you for opening this PR! A member of the Blockly team will review it soon.
mikeharv
left a comment
There was a problem hiding this comment.
Thanks for this great work! The overall approach looks good. A few behavior tweaks before we merge:
-
Ctrl/Cmd+Home/End focus the first/last category, but the category list doesn't scroll (focus uses preventScroll: true). This is easiest to see on horizontal layout toolboxes (or vertical toolboxes with a ton of categories). I think you could scroll in
ToolboxItem.onNodeFocuswithscrollIntoView({ block: 'nearest', inline: 'nearest' }), and haveToolboxCategorycallsuper.onNodeFocus(). -
Scrolling the newly focused item all the way to the opposite edge of the viewport can feel a bit jumpy when the target was only partially visible (e.g. Page Down from the top item). With a lot of items present, I'm seeing the newly focused item shift far from where it was. It might work to drop the
list.scrollByinjumpPageand rely on the existing focus scroll-into-view (scrollBoundsIntoView/scrollIntoView) so the item just moves enough to be fully visible and stays near that edge.
|
|
||
| Paging moves focus to the last item that is currently visible and scrolls it to | ||
| the edge of the viewport it moved towards, so that nothing between the old and | ||
| new position is passed over unseen. Home and End keep their workspace meaning |
There was a problem hiding this comment.
Nit: I wonder if it would be clearer to refer to Home and End here as "unmodified" or "plain" to distinguish them from the key bindings in the table above.
Toolbox items take focus with preventScroll, so Ctrl/Cmd + Home and End moved focus to a category that stayed off screen. ToolboxItem.onNodeFocus now scrolls the item into view with block and inline set to nearest. Paging no longer scrolls the newly focused item to the far edge of the viewport, which moved it a long way when it was already partly visible. The item's own scroll-into-view handles it instead, so it moves only as far as it takes to show it and stays near the edge it came from.
|
Thanks, both of those are right. Pushed a commit addressing them.
One test note: the flyout test that asserted the scroll offset is gone, since paging no longer scrolls, and there are two new ones asserting the item is scrolled into view, one through Page Down and one on focus alone in I also updated the PR description, which described the old scroll-to-the-edge behaviour. |
The basics
The details
Resolves
Fixes #10187
This is the toolbox and flyout counterpart to #10090, which added the same keys for the workspace.
Proposed Changes
No new key bindings. The four keys #10090 already registers become aware of which pane holds focus, and everything stays inside the opt-in
Blockly.ShortcutItems.registerNavigationShortcuts()bundle.Home and End keep their workspace meaning of block start and block end, and continue to do nothing in the toolbox and flyout. The issue asked for plain Home and End but noted that Ctrl/Cmd variants would be fine for consistency with the workspace, and that is what this takes.
Paging moves focus to the last currently visible item, which is what gives the assurance the issue asks for that nothing between the old and new position was passed over. A partially visible item counts as visible, so a page never steps over anything. Scrolling is left to the newly focused item, which moves the list only as far as it takes to bring itself fully into view.
Two supporting changes:
Navigator.getNavigableItems(root?)is new. It returns a tree's navigable top-level items in navigation order, which is what "first item" and "last item" mean for a toolbox or flyout. It filters out things that cannot be focused, so a toolbox separator or a flyout's trailing separator is never a jump target.Reason for Changes
The behaviour before this change was not simply missing:
workspace.getTopBlocks(), so pressing them in the toolbox threw focus out of the toolbox and into a workspace block. For the eye gaze users the issue is about, that is the opposite of what the key should do. There is a test covering this now, and it fails without the change.shouldDoBlockNavigation's!workspace.isFlyoutcheck, and Ctrl/Cmd + Home and End only ever considered blocks, so a flyout that opens with a label could not be reached by them.Test Coverage
New tests in
keyboard_navigation_test.js, against a real injected workspace with a real toolbox and flyout rather than stubs:Toolbox and flyout jump shortcuts(7 tests): first and last item in the toolbox and in the flyout, that the flyout's first and last navigable items are the labels rather than the blocks, that separators are excluded, that Ctrl/Cmd + Home no longer escapes the toolbox, and that the workspace behaviour is unchanged.Toolbox and flyout paging shortcuts(8 tests): which item each page lands on, that the newly focused item is scrolled into view, that a second Page Down advances by another page, and that paging past either end does nothing. Item geometry is stubbed to a known layout so the assertions are exact.Focusing an item scrolls it into viewintoolbox_test.js, covering the scroll on its own rather than through a shortcut.Two tests in
shortcut_items_test.jsthat asserted Page Up and Page Down "have no effect" in a flyout were retitled to say what they actually verify, that flyouts do not get the stack jumps, since they do now have a paging effect.Each new test was checked by reverting the change it covers. Disabling the toolbox and flyout path fails all 5 behaviour tests in the first suite; widening the page limit fails 5 of the paging tests; dropping the item's scroll-into-view fails the 2 that assert it.
Full unit suite: 3973 passing, 0 failing (3957 before, so +16).
npm run build,npm run lintandnpm run format:checkare clean at the repo root.I also drove the uncompiled playground in Chrome to check the real geometry, since the unit tests stub it. With a 30 category toolbox in a 475px tall list, Cmd + End moved focus to the last category and scrolled the list to it,
scrollTopgoing from 0 to 279 of a 283 maximum, and Cmd + Home came back to the first. Page Down walked 0, 18, 29 and Page Up walked back 29, 11, 0, with the list scrolling only when the target was not already visible. In a flyout of 25 blocks, Page Down walked 0, 5, 10, 15 and Page Up came back by the same step.I did not run the browser (WebdriverIO) suite locally, as chromedriver could not be downloaded in this environment. That failure happens before any test runs and is unrelated to the change, so I am relying on CI for it.
Documentation
packages/docs/docs/guides/configure/keyboard-nav.mdxgains a second table for the toolbox and flyout meanings, a note on what paging does, and a note that Home and End are workspace only. New message tokensSHORTCUTS_JUMP_PREVIOUS_PAGEandSHORTCUTS_JUMP_NEXT_PAGEare added tomessages.js,en.jsonandqqq.json.Additional Information
On the breaking change policy. The review guidelines treat new keyboard shortcuts as breaking. This adds no new bindings, and every change is confined to
registerNavigationShortcuts(), which is opt-in and not registered by default, so I have marked itfeat:rather thanfeat!:. It is still a behaviour change for anyone who already calls that function, in the two ways described above. Happy to relabel if you would rather treat it as breaking.Two things I would like your read on:
Screenshots of the flyout before and after a Page Down are attached below.