feat: Introduce FocusManager implementation #8814

BenHenning · 2025-03-21T00:39:36Z

The basics

I validated my changes

The details

Resolves

Proposed Changes

This is the bulk of the work for introducing the central logical unit for managing and sychronizing focus as a first-class Blockly concept with that of DOM focus.

While this introduces the core support needed for focus synchronization and management (along with its tests), there are some likely functional gaps that will need to be fixed in a subsequent change:

Ensuring clicks within Blockly's scope correctly sync back to focus changes.
Ensuring that FocusManager behaves in real world scenarios (i.e. those exposed by the experimental keyboard plugin). Early testing suggests that the manager handles focusing correctly, but that more work is likely needed around active/passive tracking and styling specifics, the former requiring much deeper changes in the plugin in order to properly test and validate than the surface-level changes tested so far.

Some design considerations:

Common algorithms for retrieving the current node or looking up a node by an element in a tree context were added to a new FocusableTreeTraverser to reduce code duplication and ensure direct testing (the latter of which was very handy as the traverser's tests exposed issues with its initial implementation). One open design question is whether it's preferable to remove getFocusedNode and findFocusableNodeFor from IFocusableTree in favor of just using the traverser directly. This reduces the customizability of tree implementations, but it's not obvious that trees will ever need to opt into a custom implementation, anyway (or, the manager could allow trees to pass custom functions during registration to override default traverser behavior).
- Note: The design question has been resolved. IFocusasbleTree's API has been simplified in favor of using the traverser. We can either extend its API in the future, or leverage the registry pattern to allow customization of the traverser itself.
The current CSS highlighting properties for active and passive focus are not final. They are expected to be reasonable placeholders that will be iterated upon as the system is more broadly integrated, and later after user testing.
IFocusableTree was updated to:
- Expand the contract of findFocusableNodeFor.
- Introduce a direct lookup method by element ID (which seems reasonable and simplifies FocusableTreeTraverser).
- Introduce direct tracking of nested trees. This was to avoid much more complex workarounds that would have been needed in the manager in order to ensure element->node matching worked correctly.
It's unclear whether tab index should be directly handled by FocusManager vs. making it a requirement of the Element provided by IFocusableNode. This remains an open question.
- Note: The design question has been resolved. IFocusableNode implementations will be responsible for ensuring their elements have a tab index of -1 set correctly.
The core test index page was updated to include a bunch of HTML and SVG focusable 'trees' and nodes for the purpose of testing both new components. These are elements that will exist on the page for all Blockly tests, but it seems other tests are unaffected by them. Note that this page expands the active/passive focus to make it much more obvious when running tests (though state is reset during test tear-down so this is only visible when disabling state resetting when investigating a specific test).

Reason for Changes

FocusManager is the core state management system for synchronizing DOM and Blockly focus. This will be leveraged by updating Blockly components to implement IFocusableTree and IFocusableNode, as needed, per #8771. The experimental keyboard plugin will also be updated to leverage FocusManager as a stand-in replacement for node highlighting and cursor management.

Test Coverage

The tests added to this PR are meant to be highly comprehensive and robust in testing the many different combinations of focus state and how those affect FocusManager. These tests actually had a substantial impact on fixing core issues with the manager, or adding better robustness checks to ensure the manager is always in a sane state.

Documentation

Not directly from this PR, but eventually we may want to consider adding formal documentation for focus management along with how and why to integrate with the system.

Additional Information

See also google/blockly-keyboard-experimentation#142 (comment) for broader technical design context on the introduction of these new interfaces.

This is the bulk of the work for introducing the central logical unit for managing and sychronizing focus as a first-class Blockly concept with that of DOM focus. There's a lot to do yet, including: - Ensuring clicks within Blockly's scope correctly sync back to focus changes. - Adding support for, and testing, cases when focus is lost from all registered trees. - Testing nested tree propagation. - Testing the traverser utility class. - Adding implementations for IFocusableTree and IFocusableNode throughout Blockly.

BenHenning · 2025-03-21T00:42:01Z

PTAL @gonfunko. I think it'll be useful for you to have a basis on the implementation. I still have a bit of the PR description to finish, plus more tests and some additional manager functionality.

I think the main contentious design change (or, rather, unexpected) detail is the addition of a utility class to help with tree implementations. FocusManager's test makes use of these which can be referenced as an example.

This adds new tests for the FocusableTreeTraverser and fixes a number of issues with the original implementation (one of which required two new API methods to be added to IFocusableTree). More tests have also been added for FocusManager, and defocusing tracked nodes/trees has been fully implemented in FocusManager.

BenHenning

Self-reviewed all but the tests.

tests/mocha/index.html

BenHenning · 2025-03-27T23:57:21Z

PTAL @gonfunko.

@maribethb feel free to defer to Aaron on the review, or take a pass if you'd like. I'd appreciate any amount of review coverage here as the system is fairly complex, and likely still has problems as indicated by my early testing.

/cc @rachel-fenichel as well. Rachel: I suspect you'll not want to review this, but in case you do here's the initial (and hopefully majority) implementation of FocusManager.

All: please let me know if you have any thoughts on the open design questions in the PR description. I'm happy to merge this PR as-is if no major issues are found during review, but those questions could influence the implementation.

maribethb

Will let Aaron leave a more thorough review since I haven't been working with this code much recently.

Regarding your open design question about the Traverser code: the idea is that Workspace and Flyout and whatever other Blockly methods all need to implement the findFocusableNodeFor method, and they'd all do it with an identical method, but it's not possible to have them all extend from the same base class that provides a default implementation of that method. Is that right?

If so this seems like a decent use case for mixins, but those don't seem like they're really a first-class feature of TS, and I don't know how much type hijinks we want to get up to. I don't think I know enough about which external objects are going to be implementing these interfaces to know if we want to retain the functionality of letting them do it in a custom way.

maribethb · 2025-03-31T22:04:15Z

core/focus_manager.ts

+        const matchingNodes = this.registeredTrees.map((tree) =>
+          tree.findFocusableNodeFor(activeElement),
+        );
+        newNode = matchingNodes.find((node) => !!node) ?? null;


nit, but I feel like there is something kind of funny about using the nullish coalescing operator to make the value null instead of undefined. Why not just use undefined? You could change line 48 to let newNode: IFocusableNode | undefined; and then remove this bit

I agree that it does seem odd. I suspect there's some argument here about using an intentional absent (null) vs. an unintentional absent (undefined). I'll think on this more, but I'm happy to defer to your suggestion if I can't come up with a strong argument.

Looks like it basically has to be null | undefined so I went ahead with that. Couldn't find strong advice on the best practices of one vs. the other, unfortunately.

maribethb · 2025-03-31T22:07:29Z

core/focus_manager.ts

+        // updated. Per the contract of findFocusableNodeFor only one tree
+        // should claim the element.
+        const matchingNodes = this.registeredTrees.map((tree) =>
+          tree.findFocusableNodeFor(activeElement),


Is this expensive? If so would it be better to abort early once we've found the one tree where the node is claimed?

It is, and aborting early is definitely possible. This code has changed a lot, and I like the simplification suggestion that's possible now. Will update.

gonfunko

Sending feedback now to mostly unblock things; still need to review the giant test file, but broadly this LGTM.

gonfunko · 2025-04-01T16:17:47Z

core/focus_manager.ts

+   * unregisterTree.
+   */
+  isRegistered(tree: IFocusableTree): boolean {
+    return this.registeredTrees.findIndex((reg) => reg == tree) !== -1;


Is the == (vs ===) intentional here and in unregisterTree?

It isn't, it's me forgetting that this is JavaScript. Will update.

On a similar note, I realized I wasn't consistent on test assertions for equality vs. strict equality. How much does that practically matter?

Updated--everything now uses strict equal where correct.

gonfunko · 2025-04-01T16:30:59Z

core/focus_manager.ts

+   *     focus.
+   */
+  focusNode(focusableNode: IFocusableNode): void {
+    const curTree = focusableNode.getFocusableTree();


This might be a me issue, but I read "current" as the one that existed prior to this method being called/whatever mutation it might perform; perhaps change this to newTree? Keeping prev* for the existing values would be fine in that case.

Definitely a preference, and I have no issues with using prev. Will update.

Also realized there were some inconsistencies in the naming here, too. Updated.

gonfunko · 2025-04-01T16:39:30Z

core/utils/focusable_tree_traverser.ts

+      passive = tree.findFocusableNodeFor(passiveEl);
+    }
+
+    return active ?? passive;


Perhaps just check and return active in the if statement where it's assigned so we don't go hunting for the passively focused node if it's not needed?

Definitely reasonable, will update.

Updated to exit earlier.

gonfunko · 2025-04-01T16:40:47Z

core/utils/focusable_tree_traverser.ts

+      dom.hasClass(root, 'blocklyActiveFocus') ||
+      dom.hasClass(root, 'blocklyPassiveFocus')


Especially given that they're used across several files, it might be good to make blocklyActiveFocus and blocklyPassiveFocus shared constants.

Is the typical way to do this just adding static values to FocusManager?

Done--added the constants.

I did need to duplicate between the focus manager and the traverser class, however, to avoid a circular dependency (now that FocusManager depends on the traverser class). I double checked and both suites have failures if the two constants diverge, fortunately, so it seems that the bit of duplication should have a low chance of resulting in breaking parity issues.

gonfunko · 2025-04-01T16:47:13Z

tests/mocha/focusable_tree_traverser_test.js

+    const FocusableNodeImpl = function (element, tree) {
+      this.getFocusableElement = function () {
+        return element;
+      };
+
+      this.getFocusableTree = function () {
+        return tree;
+      };
+    };


Can we not use ES6 classes here? That might save some effort if/when we migrate this to TS.

I read this backwards originally (advocating to not use ES6 classes) :). Will update--that seems completely reasonable.

Done--converted the helper classes for both new test suites to ES6 classes.

gonfunko · 2025-04-01T16:49:26Z

tests/mocha/focusable_tree_traverser_test.js

+    // Ensure all node CSS styles are reset so that state isn't leaked between tests.
+    removeFocusIndicators(document.getElementById('testFocusableTree1'));
+    removeFocusIndicators(document.getElementById('testFocusableTree1.node1'));
+    removeFocusIndicators(
+      document.getElementById('testFocusableTree1.node1.child1'),
+    );
+    removeFocusIndicators(document.getElementById('testFocusableTree1.node2'));
+    removeFocusIndicators(document.getElementById('testFocusableTree2'));
+    removeFocusIndicators(document.getElementById('testFocusableTree2.node1'));
+    removeFocusIndicators(document.getElementById('testFocusableNestedTree4'));
+    removeFocusIndicators(
+      document.getElementById('testFocusableNestedTree4.node1'),
+    );
+    removeFocusIndicators(document.getElementById('testFocusableNestedTree5'));
+    removeFocusIndicators(
+      document.getElementById('testFocusableNestedTree5.node1'),
+    );


Rather than removing the focus indicators on the various test elements, what about document.querySelectorAll() for .blocklyActiveFocus and .blocklyPassiveFocus and then remove the classes from the resulting elements? I'm imagining the case where somebody adds a new test element but misses that they need to explicitly clean it up here.

This is a tricky situation. Normally I'm a very strong advocate of avoiding logic in tests, but there are practical points where there are diminishing returns (and the logic is simple enough to verify by reading it). I agree and will update this accordingly.

Done in the latest changes.

gonfunko · 2025-04-01T17:35:06Z

tests/mocha/focusable_tree_traverser_test.js

+      assert.equal(finding, node);
+    });
+
+    test('for tree with nested tree root active no parent highlights returns null', function () {


For this and the next three tests, the traverser appears to be finding the focused node, not null?

Great catch. I also realized from this that I forgot to update the third test (it had the same name as the first). I also made sure to verify that the corrected names are what we want to be the behavior here, as well (in case I had the test names right but there was an issue with the test bodies or the implementation behaviors).

BenHenning · 2025-04-02T17:50:04Z

Will let Aaron leave a more thorough review since I haven't been working with this code much recently.

Thanks for taking a pass @maribethb!

Regarding your open design question about the Traverser code: the idea is that Workspace and Flyout and whatever other Blockly methods all need to implement the findFocusableNodeFor method, and they'd all do it with an identical method, but it's not possible to have them all extend from the same base class that provides a default implementation of that method. Is that right?

That's correct. TypeScript, unfortunately, doesn't seem to support default interface methods which would have otherwise been a good solution to the problem.

If so this seems like a decent use case for mixins, but those don't seem like they're really a first-class feature of TS, and I don't know how much type hijinks we want to get up to. I don't think I know enough about which external objects are going to be implementing these interfaces to know if we want to retain the functionality of letting them do it in a custom way.

That is a gnarly pattern haha. I'm very uncertain about using inheritance to compose state like this. It's interesting, but it would require wide implications for everything that implements IFocusableTree as I understand it, which likely will introduce a lot of future complexity.

I think it's the lack of an obvious pattern that has me lean toward "don't overcomplicate the API" (i.e. always use the traverser directly in FocusManager rather than soft requiring using it in each implementation override), but I also don't have a good sense on future proofing APIs that must maintain strong backward compatibility. There's a real trade-off here in that changing this decision later on will affect a whole lot of classes in a pretty aggressive breaking change (i.e. must implement new methods). I suspect these will largely be isolated, though. As you alluded to, I don't think we expect a lot of external classes to implement IFocusableNode or IFocusableTree, and in fact doing so would be difficult as the two are closely linked for identification.

maribethb · 2025-04-02T19:44:15Z

core/focus_manager.ts

+ *
+ * See FocusManager.takeEphemeralFocus for more details.
+ */
+export type ReturnEphemeralFocus = () => void;


If this is something we expect external developers to need to call, it should be exported from blockly.ts as well

rachel-fenichel · 2025-04-02T20:25:08Z

I think it's the lack of an obvious pattern that has me lean toward "don't overcomplicate the API" (i.e. always use the traverser directly in FocusManager rather than soft requiring using it in each implementation override), but I also don't have a good sense on future proofing APIs that must maintain strong backward compatibility.

I agree with always using the traverser directly, unless/until we have concrete examples of it being an issue. I think a benefit of centralizing this code into the Traverser is that it's easy to iterate on it in one place, as well.

There's a real trade-off here in that changing this decision later on will affect a whole lot of classes in a pretty aggressive breaking change (i.e. must implement new methods). I suspect these will largely be isolated, though.

Adding methods to classes now and removing them later would count as a big breaking change, but the opposite would not.

If we end up wanting some classes to define their own Traversers, we can use the registry, have them render custom traversers, and do a lookup based on type or a string. But let's not do that unless we actually need it, which we don't yet.

As you alluded to, I don't think we expect a lot of external classes to implement IFocusableNode or IFocusableTree, and in fact doing so would be difficult as the two are closely linked for identification.

Toolboxes and flyouts are the cases to consider here.

gonfunko · 2025-04-02T17:57:47Z

tests/mocha/focus_manager_test.js

+        const currNodeElem =
+          this.testFocusableNestedTree4Node1.getFocusableElement();
+        assert.notInclude(
+          Array.from(prevNodeElem.classList),


Note that classList supports contains() - it might be nicer to make these assert.true(node.classList.contains('blocklyActiveFocus')) or the like throughout, and removes the need for the array conversions fwiw.

Done, and moved both cases to helpers to try and improve readability a bit.

gonfunko · 2025-04-02T18:02:46Z

tests/mocha/focus_manager_test.js

+        assert.isNull(this.focusManager.getFocusedNode());
+      });
+
+      test('unfocuasble element focus()ed after registered node focused returns original node', function () {


Nit: unfocusable

Excellent catch--fixed.

BenHenning · 2025-04-03T23:10:20Z

Thanks @maribethb @gonfunko and @rachel-fenichel for the reviews & thoughts!

@gonfunko could you please take another pass on the changes and make sure they look good to you before I merge? There were a decent amount of changes, including some new documentation and documentation updates.

I think it's the lack of an obvious pattern that has me lean toward "don't overcomplicate the API" (i.e. always use the traverser directly in FocusManager rather than soft requiring using it in each implementation override), but I also don't have a good sense on future proofing APIs that must maintain strong backward compatibility.

I agree with always using the traverser directly, unless/until we have concrete examples of it being an issue. I think a benefit of centralizing this code into the Traverser is that it's easy to iterate on it in one place, as well.

There's a real trade-off here in that changing this decision later on will affect a whole lot of classes in a pretty aggressive breaking change (i.e. must implement new methods). I suspect these will largely be isolated, though.

Adding methods to classes now and removing them later would count as a big breaking change, but the opposite would not.

@rachel-fenichel wouldn't it be the case that adding methods to an export interface also is a breaking change? That's what I meant, anyway. It seems to be that any API changes to an exported interface is always breaking as it will require changes to downstream implementations (since TypeScript doesn't support default interface method).

If we end up wanting some classes to define their own Traversers, we can use the registry, have them render custom traversers, and do a lookup based on type or a string. But let's not do that unless we actually need it, which we don't yet.

Using the registry pattern is a really nice idea to solve this and one I hadn't considered. Updated the PR description to mention it, too, for future reference.

As you alluded to, I don't think we expect a lot of external classes to implement IFocusableNode or IFocusableTree, and in fact doing so would be difficult as the two are closely linked for identification.

Toolboxes and flyouts are the cases to consider here.

Good point--I could see these being cases when Blockly users may need to implement a custom IFocusableNode. However, it could get really tricky since the default behavior of, say, Toolbox would need to change (which would presumably mean subclassing it and overriding the IFocusableTree behaviors). I could see this getting quickly quite complicated, actually.

BenHenning

Self-reviewed latest changes as well.

BenHenning · 2025-04-03T23:20:18Z

From my self-review, I think everything has been addressed. If I missed anything, I'm happy to follow up fixes in a different PR @gonfunko and @maribethb.

Merging to help keep things moving.

BenHenning added 2 commits March 27, 2025 21:57

chore: fix formatting issues

9ab77ce

BenHenning commented Mar 27, 2025

View reviewed changes

tests/mocha/index.html Outdated Show resolved Hide resolved

Update tests/mocha/index.html

3dc4d17

BenHenning marked this pull request as ready for review March 27, 2025 23:54

BenHenning requested a review from a team as a code owner March 27, 2025 23:54

BenHenning requested a review from maribethb March 27, 2025 23:54

BenHenning assigned gonfunko Mar 27, 2025

BenHenning linked an issue Mar 27, 2025 that may be closed by this pull request

Support for a singleton, page-wide FocusManager. #8782

Closed

maribethb reviewed Mar 31, 2025

View reviewed changes

gonfunko reviewed Apr 1, 2025

View reviewed changes

maribethb reviewed Apr 2, 2025

View reviewed changes

gonfunko approved these changes Apr 2, 2025

View reviewed changes

BenHenning added 3 commits April 3, 2025 22:25

chore: part 1 of addressing reviewer comments.

902b26b

chore: part 2 of addressing reviewer comments.

720e8da

chore: lint fixes.

c5404af

BenHenning commented Apr 3, 2025

View reviewed changes

BenHenning merged commit 17171ab into google:rc/v12.0.0 Apr 3, 2025
7 of 8 checks passed

BenHenning deleted the introduce-focus-system-implementation branch April 3, 2025 23:20

gonfunko added the PR: feature Adds a feature label Apr 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Introduce FocusManager implementation #8814

feat: Introduce FocusManager implementation #8814

		dom.hasClass(root, 'blocklyActiveFocus') \|\|
		dom.hasClass(root, 'blocklyPassiveFocus')

feat: Introduce FocusManager implementation #8814

feat: Introduce FocusManager implementation #8814

Conversation

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Documentation

Additional Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment