chore: Add new tests for ephemeral focus #649
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
BenHenning
commented
Jul 7, 2025
Collaborator
Author
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Quick spot check self-review.
BenHenning
added a commit
to RaspberryPiFoundation/blockly
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.
Collaborator
Author
|
(Retriggering CI with an empty commit in order to validate these do pass correctly now that RaspberryPiFoundation/blockly#9213 is merged). |
RoboErikG
approved these changes
Jul 8, 2025
1 task
BenHenning
added a commit
to RaspberryPiFoundation/blockly
that referenced
this pull request
Jul 8, 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 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](https://github.com/user-attachments/assets/7c3c7c3c-224c-48f4-b4af-bde86feecfa8) ### 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.
Collaborator
Author
|
FYI I will be adding more tests to this (actually did locally, but one is flaky and need more investigation) before asking for a re-review. |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds 3 new tests for cases that were explicitly found as broken in RaspberryPiFoundation/blockly#9203 to accompany the fixes in RaspberryPiFoundation/blockly#9213.
These tests have been validated as failing without the fix from RaspberryPiFoundation/blockly#9213 in place.
These are being tested here instead of in core Blockly because they are more integration tests than testing specific components, and RaspberryPiFoundation/blockly#9213 includes a more targeted test for the specific regression that was fixed.
Some notes:
getFocusedFieldNamewas updated to properly account for a field not actually being focused for better error propagation in a failing test.