feat: Automatically manage focus tree tab indexes #9079

BenHenning · 2025-05-20T23:08:36Z

The basics

I validated my changes

The details

Resolves

Fixes #8965
Fixes #8978
Fixes #8970
Fixes google/blockly-keyboard-experimentation#523
Fixes google/blockly-keyboard-experimentation#547
Fixes part of #8910

Proposed Changes

Fives groups of changes are included in this PR:

Support for automatic tab index management for focusable trees.
Support for automatic tab index management for focusable nodes.
Support for automatically hiding the flyout when back navigating from the toolbox.
A fix for FocusManager losing DOM syncing that was introduced in fix: Update focusNode to self correct focus #9082.
Some cleanups for flyout and some tests for previous behavior changes to FocusManager.

Reason for Changes

Infrastructure changes reasoning:

Automatically managing tab indexes for both focusable trees and roots can largely reduce the difficulty of providing focusable nodes/trees and generally interacting with FocusManager. This facilitates a more automated navigation experience.
The fix for losing DOM syncing is possibly not reliable, but there are at least now tests to cover for it. This may be a case where a try{} finally{} could be warranted, but the code will stay as-is unless requested otherwise.

Flyout changes:

Flyout no longer needs to be a focusable tree, but removing that would be an API breakage. Instead, it throws for most of the normal tree/node calls as it should no longer be used as such. Instead, its workspace has been made top-level tabbable (in addition to the main workspace) which solves the extra tab stop issues and general confusing inconsistencies between the flyout, toolbox, and workspace.
Flyout now correctly auto-selects the first block (Flyout only default focuses blocks #9103 notwithstanding). Technically it did before, however the extra Flyout tabstop before its workspace caused the inconsistency (since focusing the Flyout itself did not auto-select, only selecting its workspace did).

Important caveats:

getAttribute is used in place of directly fetching .tabIndex since the latter can apparently default to -1 (and possibly 0) in cases when it's not actually set. This is a very surprising behavior that leads to incorrect test results.
Sometimes tab index still needs to be introduced (such as in cases where native DOM focus is needed, e.g. via focus() calls or clicking). This is demonstrated both by updates to FocusManager's tests as well as toolbox's category and separator. This can be slightly tricky to miss as large parts of Blockly now depend on focus to represent their state, so clicking either needs to be managed by Blockly (with corresponding focusNode calls) or automatic (with a tab index defined for the element that can be clicked, or which has a child that can be clicked).

Note that nearly all elements used for testing focus in the test index.html page have had their tab indexes removed to lean on FocusManager's automatic tab management (though as mentioned above there is still some manual tab index management required for focus()-specific tests).

Test Coverage

New tests were added for all of the updated behaviors to FocusManager, including a new need to explicitly provide (and reset) tab indexes for all focus()-esque tests. This also includes adding new tests for some behaviors introduced in past PRs (a la #8910).

Note that all of the new and affected conditionals in FocusManager have been verified as having at least 1 test that breaks when it's removed (inverted conditions weren't thoroughly tested, but it's expected that they should also be well covered now).

Additional tests to cover the actual navigation flows will be added to the keyboard experimentation plugin repository as part of google/blockly-keyboard-experimentation#557 (this PR needs to be merged first).

For manual testing, I mainly verified keyboard navigation with some cursory mouse & click testing in the simple playground. @rachel-fenichel also performed more thorough mouse & click testing (that yielded an actual issue that was fixed--see discussion below).

The core webdriver tests have been verified to have seemingly the same existing failures with and without these changes.

All of the following new keyboard navigation plugin tests have been verified as failing without the fixes introduced in this branch (and passing with them):

Tab navigating to flyout should auto-select first block
Keyboard nav to different toolbox category should auto-select first block
Keyboard nav to different toolbox category and block should select different block
Tab navigate away from toolbox restores focus to initial element
Tab navigate away from toolbox closes flyout
Tab navigate away from flyout to toolbox and away closes flyout
Tabbing to the workspace after selecting flyout block should close the flyout
Tabbing to the workspace after selecting flyout block via workspace toolbox shortcut should close the flyout
Tabbing back from workspace should reopen the flyout
Navigation position in workspace should be retained when tabbing to flyout and back
Clicking outside Blockly with focused toolbox closes the flyout
Clicking outside Blockly with focused flyout closes the flyout
Clicking on toolbox category focuses it and opens flyout

Documentation

No documentation changes are needed beyond the code doc changes included in the PR.

Additional Information

An additional PR will be introduced for the keyboard experimentation plugin repository to add tests there (see test coverage above). This description will be updated with a link to that PR once it exists.

This is still a work-in-progress change, and it may be adapted to include managing node tab indexes.

rachel-fenichel · 2025-05-23T01:10:57Z

@BenHenning what are the next steps for this to be mergeable?

BenHenning · 2025-05-27T19:24:08Z

@BenHenning what are the next steps for this to be mergeable?

@rachel-fenichel it can probably go into review now (and I will do that later today). I want to spend a bit of time seeing if I can get automatically managed node tab indexes working before fully giving up on that.

Edit: Actually there is one bug I'm re-remembering now that I'm testing this locally that still needs to be fixed yet (sometimes the flyout doesn't auto-collapse).

BenHenning · 2025-05-27T19:28:32Z

The following scenario breaks still:

Tab-navigate to the workspace
Press 'T' to open the flyout (via toolbox focus)
Press 'right' to navigate to the flyout
Press 'tab'
Observe that there's a weird in-between state with the flyout still focused but the workspace having an outline (local debugging confirms that the workspace svg holds DOM focus but the FocusManager still thinks the flyout block should hold active focus)

This is related to google/blockly-keyboard-experimentation#547.

BenHenning · 2025-05-27T20:31:30Z

Uh oh. FocusManager can get stuck in a state updating the focused node which basically disables DOM syncing. 🙃 This could cause all sorts of interesting issues. I'll fix it in this PR alongside the main issue.

This removes Flyout's ability to be tab-focused and insteads relies on treating it as a workspace, removing the extra tab stop. This fixes an issue with FocusManager getting semi-permanently out of sync with the DOM. This reintroduces automatic tab management for nodes, and fixes an issue with auto tab management for trees that would break tabbing order.

BenHenning · 2025-05-27T21:35:18Z

NB: This should be fully working now, but tests & some clean-up need to come yet.

I also think that I'm going to be a bit stricter on tab management in FocusManager. Right now trees need to make themselves focusable but non-tabbable if they disable auto management, but nodes no longer need to. This inconsistency feels weird and a bit bad--it's preferable just to have FocusManager be entirely responsible for managing tabs, I think.

Edit: Actually, I just realized that focusNode already automatically solves this, so setting a tree's root to a -1 tab index is simply redundant. Cool.

BenHenning · 2025-05-27T22:45:39Z

Decided to include a fix for #8970 since I was in the neighborhood.

BenHenning · 2025-05-27T23:53:30Z

From @rachel-fenichel's testing:

Interacting with the workspace and then clicking on a toolbox category opens the first category instead of the one I clicked

Also fix a potential edge case with the auto tab index management overwriting an existing tab index (since it can sometimes be necessary as indicated by the doc update for IFocusableNode).

core/toolbox/category.ts

BenHenning

Self-reviewed (mostly a spot check for anything obvious).

core/toolbox/category.ts

BenHenning · 2025-05-28T01:43:18Z

Things left to do in/for this PR:

Add the tests mentioned above for toolbox. Added 1 new test in chore: Introduce tests for flyout & toolbox blockly-keyboard-experimentation#557.
Fix behaviors to get the keyboard nav tests to pass. Fix was simple: need to use focusNode() now instead of .focus().
Add new keyboard nav tests. Done in chore: Introduce tests for flyout & toolbox blockly-keyboard-experimentation#557.
Verify core webdriver tests with these changes. This is tricky since they're still failing at tip-of-tree (I see 28-29 pass on each run without these changes). The results appear the same with these changes. Unfortunately some of the failing tests are related to toolbox categories so we can't test those effectively.
~~Add video demonstrations of each of the linked bugs now being fixed.~~ (skipping since the behavior can be observed via the tests).
Fix that clicking out of the injection div doesn't auto-close the Flyout when it's focused (but does work fine when toolbox is focused). Found by @microbit-robert. Fixed this without too much trouble, but still need to add a test for it. Tests have been added to keyboard nav and are now tracked as part of that item above.
Address feedback & get back into review (after everything else is done).

BenHenning · 2025-05-28T01:43:36Z

I think this can be reviewed now, though please note there will need to probably be 1 more pass once all of the above items are addressed.

maribethb · 2025-05-28T17:55:10Z

core/flyout_base.ts

  }

  /** See IFocusableTree.getRootFocusableNode. */
  getRootFocusableNode(): IFocusableNode {
-    return this;
+    throw new Error('Flyouts are not directly focusable.');


Can you add a deprecation warning to these so we can remove them in v13?

Done in latest.

maribethb · 2025-05-28T17:58:46Z

core/focus_manager.ts

+   * not currently registered.
+   */
+  private lookUpRegistration(tree: IFocusableTree): TreeRegistration | null {
+    const index = this.registeredTrees.findIndex((reg) => reg.tree === tree);


Why are you using findIndex instead of find since you're returning the element anyway, not the index?

Though I'm wondering if this should be a map of registered trees to IDs and not an array. Does the order in which trees are registered matter?

No reason not to use find honestly--updated.

As for order, it doesn't matter* and the size of the array will be quite small (there aren't many trees). I'm also not keen on relying on the stability of the tree's root's element ID for registration correctness (though it should technically be reliable per contracts).

* The order can theoretically matter, but only if trees are violating their contracts.

maribethb · 2025-05-28T18:04:05Z

core/focus_manager.ts

   * @param focusableNode The node that should receive active focus.
   */
  focusNode(focusableNode: IFocusableNode): void {
    this.ensureManagerIsUnlocked();
-    if (!this.currentlyHoldsEphemeralFocus) {
+    const mustRestoreUpdatingNodeFlag = !this.currentlyHoldsEphemeralFocus;


nit: don't use the word flag in the name, we already know it's a boolean

Ah nice catch--surprised I did that. Fixed!

maribethb · 2025-05-28T18:16:01Z

core/focus_manager.ts

    }
    node.onNodeFocus();
    this.lockFocusStateChanges = false;

+    // Only overwrite the tab index if it isn't a tree root that's auto managed.


I find this confusing, could you explain it and consider rewriting the comment?

You say you're going to overwrite the tab index, but you only set it if there wasn't a tab index attribute to start with. It's hard for me to call that overwriting. I think you have to add this because you can't focus an element that doesn't have a tabindex, and you removed the tabindex from a bunch of the html for core blockly elements like fields. Is that right? If so, that would be helpful context to put in the comment instead of saying "overwrite"

On a separate confusion, it would help if you explained why you don't set it if it is a tree root that's auto managed. Is the only reason because you already set it to -1 above?

This is, admittedly, a very confusing comment. :)

... I think you have to add this because you can't focus an element that doesn't have a tabindex, and you removed the tabindex from a bunch of the html for core blockly elements like fields. Is that right? If so, that would be helpful context to put in the comment instead of saying "overwrite"

This is correct. It's documented in IFocusableNode (where I think it's much more important to explain), but I completely agree that 'overwrite' is the wrong terminology here and the context is important. I've tried to clarify the different cases that this bit affects, and why.

On a separate confusion, it would help if you explained why you don't set it if it is a tree root that's auto managed. Is the only reason because you already set it to -1 above?

Yep, this is the reason, and I've attempted to also clarify it in the latest changes.

Hopefully this is much more clear now.

BenHenning · 2025-05-28T22:25:59Z

NB: The plan to fix #9105 is no longer part of this PR. It's not blocking MakeCode (since they use their own flyout implementation), and the fix is actually likely to be a bit tricky with the way we automatically open/close the flyout based on focus.

Main documentation and code clarity improvements.

BenHenning

Self-reviewed all the latest changes.

BenHenning · 2025-05-29T01:36:02Z

With google/blockly-keyboard-experimentation#557 in place and all the tasks listed above addressed (including reviewer comments), I think this is ready again for review. Fortunately, there weren't many changes needed here.

PTAL @maribethb.

BenHenning · 2025-05-29T19:10:14Z

Thanks @maribethb for the review, and @rachel-fenichel for helping with testing!

Fixes part of google/blockly#8915 Fixes part of google/blockly#9020 This PR does a few things: - It introduces some new helpers to simplify arrow key inputs across all of the webdriver tests. - It introduces a new focusable div element that exists in the tab order before toolbox (so that back navigation can be tested). - It renames a few functions for clarity: `setCurrentCursorNodeById` -> `focusOnBlock` and `setCurrentCursorNodeByIdAndFieldName` -> `focusOnBlockField`. This is closer to what the functions are actually doing, and it moves away from cursor verbiage (which could become confusing in the future once the cursor is removed). - It introduces some robustness sanity checks for test utility functions. These should fail if an assumption isn't met rather than return null or undefined--failing fast is really useful in tests to avoid hiding legitimate failures. - It introduces a whole test suite for toolbox and flyout (though a lot more tests can be added). This specifically emphasizes regressions fixed by google/blockly#9079.

feat: Automatically manage focus tree tab indexes.

5c28b15

This is still a work-in-progress change, and it may be adapted to include managing node tab indexes.

github-actions bot added the PR: feature Adds a feature label May 20, 2025

This was referenced May 20, 2025

Extra toolbox tab position google/blockly-keyboard-experimentation#523

Closed

Shift+Tab after keyboard navigating to a block doesn’t open flyout as expected #8965

Closed

Merge branch 'develop' into auto-manage-tab-indexes

227ee12

BenHenning mentioned this pull request May 27, 2025

Focus is not moving properly from flyout to workspace in one circumstance google/blockly-keyboard-experimentation#547

Closed

1 task

github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels May 27, 2025

chore: Add docs + redundant code removal.

d8bf9a9

github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels May 27, 2025

fix: Auto-close flyout on back nav.

6ae6c88

chore: Add and fix tests.

c551191

Also fix a potential edge case with the auto tab index management overwriting an existing tab index (since it can sometimes be necessary as indicated by the doc update for IFocusableNode).

BenHenning commented May 28, 2025

View reviewed changes

core/toolbox/category.ts Show resolved Hide resolved

fix: Resetting issue with toolbox categories.

fc3001b

github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels May 28, 2025

BenHenning commented May 28, 2025

View reviewed changes

core/toolbox/category.ts Show resolved Hide resolved

core/toolbox/category.ts Show resolved Hide resolved

BenHenning marked this pull request as ready for review May 28, 2025 01:43

BenHenning requested a review from a team as a code owner May 28, 2025 01:43

BenHenning requested a review from maribethb May 28, 2025 01:43

github-actions bot assigned maribethb May 28, 2025

Merge branch 'develop' into auto-manage-tab-indexes

265c422

github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels May 28, 2025

maribethb reviewed May 28, 2025

View reviewed changes

github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels May 28, 2025

fix: Clicking outside toolbox should close flyout.

3fc6b22

github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels May 29, 2025

chore: Address reviewer comments.

f4b69c6

Main documentation and code clarity improvements.

BenHenning commented May 29, 2025

View reviewed changes

BenHenning mentioned this pull request May 29, 2025

chore: Introduce tests for flyout & toolbox google/blockly-keyboard-experimentation#557

Merged

maribethb approved these changes May 29, 2025

View reviewed changes

BenHenning merged commit 3cbca8e into google:develop May 29, 2025
7 checks passed

BenHenning deleted the auto-manage-tab-indexes branch May 29, 2025 19:10

microbit-robert mentioned this pull request May 30, 2025

Opening the context menu on a block in the flyout causes the flyout to close #9111

Closed

1 task

feat: Automatically manage focus tree tab indexes #9079

feat: Automatically manage focus tree tab indexes #9079

Uh oh!

Conversation

BenHenning commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Documentation

Additional Information

Uh oh!

rachel-fenichel commented May 23, 2025

Uh oh!

BenHenning commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenHenning commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenHenning commented May 27, 2025

Uh oh!

BenHenning commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenHenning commented May 27, 2025

Uh oh!

BenHenning commented May 27, 2025

Uh oh!

Uh oh!

BenHenning left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BenHenning commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenHenning commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenHenning commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenHenning left a comment

Choose a reason for hiding this comment

Uh oh!

BenHenning commented May 29, 2025

Uh oh!

Uh oh!

BenHenning commented May 29, 2025

Uh oh!

Uh oh!

BenHenning commented May 20, 2025 •

edited

Loading

BenHenning commented May 27, 2025 •

edited

Loading

BenHenning commented May 27, 2025 •

edited

Loading

BenHenning commented May 27, 2025 •

edited

Loading

BenHenning commented May 28, 2025 •

edited

Loading

BenHenning commented May 28, 2025 •

edited

Loading

BenHenning commented May 28, 2025 •

edited

Loading