fix: Don't auto-close flyout for new vars #9176
Draft
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
This isn't a complete solution, but is a decent start toward one. It also builds resilience into Flyout itself for recreation cases.
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The basics
The details
Resolves
Fixes #9106
Fixes #9122
Proposed Changes
This ensures
Flyoutdoesn't close when it shouldn't (i.e. when creating or renaming variables).Reason for Changes
The main issue is that the window prompt takes focus away from Blockly entirely, so ephemeral focus is now being used for variable creation and renaming to restore focus back to the original node.
However, that's not a complete solution since creating a variable causes the flyout to fully be laid out again. As a result, there's no trivial way to track and restore focus back to the element that had held focus previously (or to use the ephemerally-restored focus to even realize that focus should be restored). To make this work reasonably well, when the
Flyoutis re-laid out it will now check if it had focus before and, if it did, focus the first new element it creates after being relaid out. It's not a perfect solution, but it seems like a reasonable medium or long-term solution.Test Coverage
TODO: Tests need to be added yet.
Documentation
No new documentation changes should be needed.
Additional Information
Note that this PR is not production ready yet as it completely disables auto-flyout closing. The main issue here is that there's a moment either due to the prompt or due to elements with focus being deleted that eventually lead to the flyout's workspace being blurred and attempts to restore focus after that completely fail since it's been closed and the nodes no longer exist. It's unclear at the moment how to solve this problem.