Skip to content

Conversation

@BenHenning
Copy link
Collaborator

@BenHenning BenHenning commented Jul 8, 2025

The basics

The details

Resolves

Fixes RaspberryPiFoundation/blockly-keyboard-experimentation#563

Proposed Changes

This expands the functionality introduced in #9213 to also include widget divs.

Reason for Changes

MakeCode makes use of widget div in several field editors, so the issues described in RaspberryPiFoundation/blockly-keyboard-experimentation#563 aren't fully mitigated with #9213 alone.

This PR essentially adds the same support for auto-closing as drop-down divs now have, and enables this functionality by default.

Note the drop-down div change: it was missed in #9123 that the API change for drop-down div's show function is actually API-breaking, so this updates that API to be properly backward compatible (and reverts one test change that makes use of it).

The FocusManager change actually corrects an implementation issue from #9123: not updating the tracked focus status before calling the callback can result in focus being inadvertently restored if the callback triggers returning focus due to a lost focus situation. This was wrong for drop-down divs, too, but it's harder to notice there because the dismissal of the drop-down div happens on a timer (which means there's sufficient time for FocusManager's state to correct prior to attempting to return from the ephemeral focus state).

Demonstration of fixed behavior (since the inline number editor uses a widget div):

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

Test Coverage

New widget div tests have been added to verify the new parameter and auto-close functionality.

The FocusManager test was updated to account for the new, and correct, behavior around the internal tracked ephemeral focus state.

Note that some tabindex state has been clarified and cleaned up in the test index page and FocusManager. It's fine (and preferable) for ephemeral-used elements to always be focusable rather than making them dynamically so (which avoids state bleed across test runs which was happening one of the new tests).

RaspberryPiFoundation/blockly-keyboard-experimentation#649 includes additional tests for validating widget behaviors.

Documentation

No new documentation should be needed here--the API documentation changes should be sufficient.

One documentation update was made in dropdowndiv.ts that corrects the documentation parameter ordering.

Additional Information

Nothing further to add.

@github-actions github-actions bot added the PR: fix Fixes a bug label Jul 8, 2025
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Jul 8, 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.

Self-checked and fixed a few things, and added a couple more tests.

@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Jul 8, 2025
@BenHenning BenHenning marked this pull request as ready for review July 8, 2025 21:14
@BenHenning BenHenning requested a review from a team as a code owner July 8, 2025 21:14
@BenHenning BenHenning requested a review from gonfunko July 8, 2025 21:14
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Jul 8, 2025
@BenHenning
Copy link
Collaborator Author

BenHenning commented Jul 8, 2025

FYI: I'm going to update RaspberryPiFoundation/blockly-keyboard-experimentation#649 to include tests for the field editor to integrate test the widget changes, and then I'm going to try and verify all samples that might be affected by both widget and drop-down div changes.

@microbit-matt-hillsdon
Copy link
Contributor

microbit-matt-hillsdon commented Jul 8, 2025

Did a quick test of this in MakeCode and it looks promising. Will test further tomorrow (don't block merge on my feedback).

Fields that use both widget + dropdown only close widget but I think that's probably on us to sort as it's the widget that takes the focus. It seems to do no real harm and we can likely address in MakeCode. You might have similar scenarios (maybe in other direction?) in samples.

@BenHenning
Copy link
Collaborator Author

Did a quick test of this in MakeCode and it looks promising. Will test further tomorrow (don't block merge on my feedback).

Fields that use both widget + dropdown only close widget but I think that's probably on us to sort as it's the widget that takes the focus. It seems to do no real harm and we can likely address in MakeCode.

Thanks for the quick check @microbit-matt-hillsdon!

Drop-downs + widgets have been historically tricky with ephemeral focus, and this definitely adds to that. It may be the case you simply would turn off the auto behavior for those situations, I suspect, and handle it manually (which isn't ideal since catching all the cases where focus is lost is annoying and precisely why FocusManager is doing it).

If there's an API change we can make to simplify this, please do let me know.

@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Jul 8, 2025
@BenHenning BenHenning merged commit bea183d into RaspberryPiFoundation:develop Jul 8, 2025
14 checks passed
@BenHenning BenHenning deleted the auto-close-widget-divs-on-lost-focus branch July 8, 2025 23:06
microbit-robert added a commit to microbit-matt-hillsdon/blockly that referenced this pull request Jul 9, 2025
BenHenning added a commit to BenHenning/blockly that referenced this pull request Jul 9, 2025
BenHenning added a commit that referenced this pull request Jul 9, 2025
* Revert "fix: Auto-close widget divs on lost focus (#9216)"

This reverts commit bea183d.

* Revert "fix: Auto close drop-down divs on lost focus (reapply) (#9213)"

This reverts commit 0e16b04.
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

3 participants