Cursor test fails: In - From output connection #8955

rachel-fenichel · 2025-05-01T20:54:07Z

Check for duplicates

I have searched for similar issues before opening a new one.

Description

This test fails after @BenHenning's focus manager changes to the line cursor:

    test('In - From output connection', function () {
      const fieldBlock = this.blocks.E;
      const outputNode = ASTNode.createConnectionNode(
        fieldBlock.outputConnection,
      );
      this.cursor.setCurNode(outputNode);
      this.cursor.in();
      const curNode = this.cursor.getCurNode();
      assert.equal(curNode.getLocation(), fieldBlock);
    });

The proximate cause is that output connections are not given highlight paths in drawer.updateConnectionHighlights because they live at the block level instead of in the rows.

In the keyboard experimentation plugin we sometimes (always?) use the block's node or the parent input connection's node, rather than placing the cursor on an output connection.

If the cursor is allowed to go to an output connection, updateConnectionHighlights needs to be updated.

If the cursor cannot ever be placed at an output connection, the direct call to this.cursor.setCurNode(outputNode); should throw an error.

Local test updates

To make this test useful again, the initial cursor location can be block A's first input connection. Moving in from there should reach the field block node.

Reproduction steps

Stack trace

Screenshots

No response

Browsers

No response

The text was updated successfully, but these errors were encountered:

@rachel-fenichel

## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #8940 Fixes #8954 Fixes #8955 ### Proposed Changes This updates `LineCursor` to use `FocusManager` rather 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 `FocusManager` has been designed to do. Since these nodes are already focused, `FocusManager` can be used as the primary source of truth for determining where the user currently has navigated, and where to go next. Previously, `LineCursor` relied on selection for this purpose, but selection is now automatically updated (for blocks) using focus-controlled `focus` and `blur` callbacks. Note that the cursor will still fall back to synchronizing with selection state, though this will be removed once the remaining work to eliminate `MarkerSvg` has 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: - Some special handling exists for flyouts to handle navigating across stacks (per the current cursor design). - `FocusableTreeTraverser` is needed by the keyboard navigation plugin (in google/blockly-keyboard-experimentation#482) so it's now being exported. - `FocusManager` had 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). - `FocusManager` had some issues from #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 #8910). - `FocusManager` was 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 unique `FocusManager` exists _per-test_. It's possible for DOM focus state to still bleed across tests, but `FocusManager` largely guarantees eventual consistency. This change prevents a class of focus errors from being possible when running tests. - A number of cursor tests needed to be updated to ensure that a connections are properly rendered (as this is a requirement for focusable nodes, and cursor is now focusing nodes). One test for output connections was changed to use an input connection, instead, since output connections can no longer be navigated to (and aren't rendered, thus are not focusable). It's possible this will need to be changed in the future if we decide to reintroduce support for output connections in cursor, but it seems like a reasonable stopgap. Huge thanks to @rachel-fenichel for helping investigate and providing an alternative for the output connection test. **Current gaps** to be fixed after this PR is merged: - The flyout automatically closes when creating a variable with with keyboard or mouse (I think this is only for the keyboard navigation plugin). I believe this is a regression from previous behavior due to how the navigation plugin is managing state. It would know the flyout should be open and thus ensure it stays open even when things like dialog prompts try to close it with a blur event. However, the new implementation in google/blockly-keyboard-experimentation#482 complicates this since state is now inferred from `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 importing `WorkspaceSvg` directly rather than its type. - The flyout, while it stays open, does not automatically update past the first variable being created without closing and reopening it. I'm actually not at all sure why this particular behavior has regressed. ### Test Coverage No new non-`FocusManager` tests 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 `FocusManager` tests 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.

BenHenning · 2025-05-02T05:26:08Z

Fixed in #8941.

rachel-fenichel added issue: bug Describes why the code or behaviour is wrong issue: triage Issues awaiting triage by a Blockly team member labels May 1, 2025

rachel-fenichel added this to Blockly Accessibility May 1, 2025

rachel-fenichel moved this to Todo in Blockly Accessibility May 1, 2025

BenHenning mentioned this issue May 1, 2025

feat: Update line cursor to use focus manager #8941

Merged

1 task

BenHenning self-assigned this May 2, 2025

BenHenning closed this as completed May 2, 2025

github-project-automation bot moved this from Todo to Done in Blockly Accessibility May 2, 2025

sappm01 removed the issue: triage Issues awaiting triage by a Blockly team member label May 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cursor test fails: In - From output connection #8955

Cursor test fails: In - From output connection #8955

Cursor test fails: In - From output connection #8955

Cursor test fails: In - From output connection #8955

Comments

Check for duplicates

Description

Local test updates

Reproduction steps

Stack trace

Screenshots

Browsers