Skip to content

Conversation

@BenHenning
Copy link
Collaborator

@BenHenning BenHenning commented Jun 26, 2025

The basics

The details

Resolves

Fixes RaspberryPiFoundation/blockly-keyboard-experimentation#563

Proposed Changes

This introduces support in FocusManager to receive feedback on when an ephemerally focused element entirely loses focus (that is, neither it nor any of its descendants have focus).

This also introduces a behavior change for drop-down divs using the previously mentioned functionality to automatically close themselves when they lose focus for any reason (e.g. clicking outside of the div or tab navigating away from it).

Finally, and importantly, this adds a case where ephemeral focus does not automatically return to the previously focused node: when focus is lost to the ephemerally focused element's tree and isn't instead put on another focused node.

Reason for Changes

Ultimately, focus is probably the best proxy for cases when a drop-down div ought to no longer be open. However, tracking focus only within the scope of the drop-down div utility is rather difficult since a lot of the same problems that FocusManager handles also occur here (with regards to both descendants and outside elements receiving focus). It made more sense to expand FocusManager's ephemeral focus support:

  • It was easier to implement this FocusManager and in a way that's much more robust (since it's leveraging existing event handlers).
  • Using FocusManager trivialized the solution for drop-down divs.
  • There could be other use cases where custom ephemeral focus uses might benefit from knowing when they lose focus.

This new support is enabled by default for all drop-down divs, but can be disabled by callers if they wish to revert to the previous behavior of not auto-closing.

The change for whether to restore ephemeral focus was needed to fix a drawback that arises from the automatic returning of ephemeral focus introduced in this PR: when a user clicks out of an open drop-down menu it will restore focus back to the node that held focus prior to taking ephemeral focus (since it properly hides the drop-down div and restores focus). This creates awkward behavior issues for both mouse and keyboard users:

  • For mouse: trying to open a drop-down outside of Blockly will automatically close the drop-down when the Blockly drop-down finishes closing (since focus is stolen back away from the thing the user clicked on).
  • For keyboard: tab navigating out of Blockly tries to force focus back to Blockly.

Test Coverage

New tests have been added for both the drop-down div and FocusManager components, and have been verified as failing without the new behaviors in place.

There may be other edge cases worth testing for FocusManager in particular, but the tests introduced in this PR seem to cover the most important cases.

Demonstration of the new behavior:

Screen.recording.2025-07-01.6.28.37.PM.webm

Documentation

No new documentation changes seem needed beyond the code documentation updates.

Additional Information

It's also possible to change the automatic restoration behavior to be conditional instead of always assuming focus shouldn't be reset if focus leaves the ephemeral element, but that's probably a better change if there's an actual user issue discovered with this approach.

@github-actions github-actions bot added the PR: fix Fixes a bug label Jun 26, 2025
Copy link
Collaborator Author

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spot checked changes.

@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Jul 2, 2025
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Jul 2, 2025
Copy link
Collaborator Author

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spot checked latest changes.

@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Jul 2, 2025
Copy link
Collaborator Author

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spot checked latest changes.

There is an ordering change to drop-down's disposal to avoid desyncing the new FocusManager state checks on ending ephemeral focus. It seems this doesn't affect widget divs so that didn't need to update (probably because there isn't actually a DOM change happening in those cases).

@BenHenning BenHenning marked this pull request as ready for review July 2, 2025 01:33
@BenHenning BenHenning requested a review from a team as a code owner July 2, 2025 01:33
@BenHenning BenHenning requested a review from RoboErikG July 2, 2025 01:33
@BenHenning BenHenning removed the request for review from RoboErikG July 2, 2025 01:33
@BenHenning BenHenning marked this pull request as draft July 2, 2025 01:33
@BenHenning BenHenning marked this pull request as ready for review July 2, 2025 01:33
@BenHenning BenHenning marked this pull request as draft July 2, 2025 01:34
@BenHenning BenHenning marked this pull request as ready for review July 2, 2025 01:34
@BenHenning BenHenning requested a review from gonfunko July 2, 2025 01:34
@BenHenning BenHenning merged commit 4c78c1d into RaspberryPiFoundation:develop Jul 2, 2025
12 checks passed
@BenHenning BenHenning deleted the auto-close-drop-down-divs-on-lost-focus branch July 2, 2025 23:11
microbit-matt-hillsdon added a commit to microbit-matt-hillsdon/blockly that referenced this pull request Jul 4, 2025
@cpcallen
Copy link
Collaborator

cpcallen commented Jul 4, 2025

It seems that this PR has caused some regressions, as reported by @microbit-matt-hillsdon in #9203 and additionally has caused mocha tests failures in develop:

Text Input Fields
  Use editor
    Geras theme
      ✖️ Finishing editing returns ephemeral focus
        AssertionError: expected null to equal FieldNumber{ …(37) }
            at Context.<anonymous> (field_textinput_test.js:440:16)
      ✖️ Opening an editor, tabbing, then editing changes focus to the second field
        AssertionError: expected null to equal FieldNumber{ …(37) }
            at Context.<anonymous> (field_textinput_test.js:500:16)
    Zelos theme
      ✖️ Opening an editor, tabbing, then editing full block field changes focus to the second field
        AssertionError: expected null to equal BlockSvg{ data: null, …(51) }
            at Context.<anonymous> (field_textinput_test.js:582:16)

I'm not quite sure why the CI tests for this PR didn't themselves fail; perhaps it only manifests itself due to another PR that was merged between when the CI tests for this PR were last run and when it was itself merged.

N.B. AFAICT we currently have GitHub configured to test the result of merging a branch into the target branch (i.e. in this case merging BenHenning:auto-cl-s-drop-down-div into google:develop), not just the PR branch itself. (That's why CI for #9201 is failing even though I did not observe test failures locally.)

I'm going to open a PR to revert this one for the time being.

cpcallen added a commit that referenced this pull request Jul 4, 2025
cpcallen added a commit that referenced this pull request Jul 7, 2025
BenHenning added a commit to BenHenning/blockly that referenced this pull request Jul 7, 2025
BenHenning added a commit that referenced this pull request Jul 7, 2025
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Fixes RaspberryPiFoundation/blockly-keyboard-experimentation#563

### Proposed Changes

This introduces support in `FocusManager` to receive feedback on when an ephemerally focused element entirely loses focus (that is, neither it nor any of its descendants have focus).

This also introduces a behavior change for drop-down divs using the previously mentioned functionality to automatically close themselves when they lose focus for any reason (e.g. clicking outside of the div or tab navigating away from it).

Finally, and **importantly**, this adds a case where ephemeral focus does _not_ automatically return to the previously focused node: when focus is lost to the ephemerally focused element's tree and isn't instead put on another focused node.

### Reason for Changes

Ultimately, focus is probably the best proxy for cases when a drop-down div ought to no longer be open. However, tracking focus only within the scope of the drop-down div utility is rather difficult since a lot of the same problems that `FocusManager` handles also occur here (with regards to both descendants and outside elements receiving focus). It made more sense to expand `FocusManager`'s ephemeral focus support:
- It was easier to implement this `FocusManager` and in a way that's much more robust (since it's leveraging existing event handlers).
- Using `FocusManager` trivialized the solution for drop-down divs.
- There could be other use cases where custom ephemeral focus uses might benefit from knowing when they lose focus.

This new support is enabled by default for all drop-down divs, but can be disabled by callers if they wish to revert to the previous behavior of not auto-closing.

The change for whether to restore ephemeral focus was needed to fix a drawback that arises from the automatic returning of ephemeral focus introduced in this PR: when a user clicks out of an open drop-down menu it will restore focus back to the node that held focus prior to taking ephemeral focus (since it properly hides the drop-down div and restores focus). This creates awkward behavior issues for both mouse and keyboard users:
- For mouse: trying to open a drop-down outside of Blockly will automatically close the drop-down when the Blockly drop-down finishes closing (since focus is stolen back away from the thing the user clicked on).
- For keyboard: tab navigating out of Blockly tries to force focus back to Blockly.

**New in v2 of this PR**: Commit 0363d67 is the main one that prevents #9203 from being reintroduced by ensuring that widget div only clears its contents after ephemeral focus has returned. This was missed in the first audit since it wasn't clear that this line, in particular, can cause a div with focus to be removed and thus focus lost: https://github.com/google/blockly/blob/dfd565957b7652929ac07f4f7330390dfc459e94/core/widgetdiv.ts#L156

### Test Coverage

New tests have been added for both the drop-down div and `FocusManager` components, and have been verified as failing without the new behaviors in place.

There may be other edge cases worth testing for `FocusManager` in particular, but the tests introduced in this PR seem to cover the most important cases.

**New in v2 of this PR**: A test was added to validate that widget div now clears its contents only after ephemeral focus has returned to avoid the desyncing scenario that led to #9203. This test has been verified to fail without the fix. There are also a few new tests being added in the keyboard navigation plugin repository that also validate this behavior at a higher level (see RaspberryPiFoundation/blockly-keyboard-experimentation#649).

Demonstration of the new behavior:

[Screen recording 2025-07-01 6.28.37 PM.webm](https://github.com/user-attachments/assets/7af29fed-1ba1-4828-a6cd-65bb94509e72)

### Documentation

No new documentation changes seem needed beyond the code documentation updates.

### Additional Information

It's also possible to change the automatic restoration behavior to be conditional instead of always assuming focus shouldn't be reset if focus leaves the ephemeral element, but that's probably a better change if there's an actual user issue discovered with this approach.

This was originally introduced in #9175 but it was reverted in #9204 due to #9203.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: fix Fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ephemeral focus issue clicking outside injection div

4 participants