Distraction Free: Avoid focus loss when enabling/disabling distraction free mode via the more menu #51627
Conversation
This file contains 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
talldan
added
[Type] Bug
10 tasks
talldan
commented
Jun 19, 2023
|
Size Change: +192 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 31a3c6d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5307656090
|
talldan
changed the title
…move block selection workaround for lost focus
|
This is great! I've been struggling with this issue but it didn't occur to me to not change the variations but instead change the animation props on distraction free mode updates. Because without this or re-rendering framer motion would continue to animate when exiting the mode. This is prefferable to re-rendering (off course) so 👏🏻 and 🎉 I have pushed a commit removing the conditional in the animation variants inside the header for the animation of the elements (site hub, document tools, publish tools).
This was when the focus was in the document title and no block was selected. There was a workaround for the focus loss implemented by me to select the 1st block - which is now not needed as the focus should remain in the dropdown. Fixed in 4e9a2a4 |
draganescu
approved these changes
Jun 19, 2023
There was a problem hiding this comment.
Excellent fix for a long standing issue, thank you @talldan
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…n free mode via the more menu (WordPress#51627) * Avoid header region unmounting when enabling distraction free mode via the more menu * remove the conditional in framer motion state for header elements, remove block selection workaround for lost focus --------- Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com>
priethor
added
[Focus] Accessibility (a11y)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
Improves the issue mentioned here - #41740 (comment)
There was a focus loss issue when entering distraction free mode via the editor options menu. This PR should resolve the issue in the majority of cases.
When testing I still noticed that very rarely a focus loss would occur, but I don't yet understand why, but I imagine there's a separate bug somewhere.
Why?
The focus loss was happening due to an issue in the
InterfaceSkeletoncomponent. When toggling distraction free mode, the element being rendered switches from adivto amotion.div. When switching components like this it can cause React to unmount and remount the DOM element.There was also some conditional rendering logic that might trip up the react reconciler.
How?
I've switch to always rendering a
motion.div. The advantage of this is that it's now possible to properly animate the entry and exit of distraction free mode.I've also removed some of the conditional rendering in favour of just outputting a single
NavigableRegionTesting Instructions for Keyboard
Expected: The menu should stay open and the menu item should retain focus.
Screenshots or screencast
Kapture.2023-06-19.at.11.58.53.mp4