Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix key handling after simulator is made full screen #10365

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

microbit-robert
Copy link
Contributor

@microbit-robert microbit-robert commented Jan 30, 2025

After triggering the full screen simulator, the "escape" key does nothing. After minimising the simulator using the relevant button, the "escape" key now triggers the full screen simulator.

The logic in the code appears to be inverted. The call to focus the simulator also seems dubious as it feels like the best thing to do is leave focus on the fullscreen toggle button in both the expand and collapse case, however, I'd welcome feedback on this.

This PR fixes the inverted logic in the code and removes the dubious focus call.

@riknoll
Copy link
Member

riknoll commented Jan 30, 2025

hmm... not sure about removing the focus stealing here. seems to me that the fullscreen simulator should be treated like a modal, since that's essentially what it is, and with modals we usually move focus to the content (or at least the container of the content). i think we should match that experience as closely as possible.

rest of the pr LGTM!

@riknoll riknoll self-assigned this Jan 30, 2025
@microbit-robert
Copy link
Contributor Author

hmm... not sure about removing the focus stealing here. seems to me that the fullscreen simulator should be treated like a modal, since that's essentially what it is, and with modals we usually move focus to the content (or at least the container of the content). i think we should match that experience as closely as possible.

rest of the pr LGTM!

That makes sense, thanks. I've pushed a commit to restore the focus call.

@riknoll riknoll merged commit aae3e27 into microsoft:master Jan 31, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants