Reset focus after route navigation in Site Editor #37314
Conversation
kevin940726
added
[Type] Bug
|
Size Change: +216 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
kevin940726
force-pushed
the
fix/edit-site-focus-management
branch
from
db5941f
to
1b50ad1
Compare
| */ | ||
| import { store as editSiteStore } from '../../store'; | ||
|
|
||
| export default function EditorActions() { |
There was a problem hiding this comment.
This component is extracted from the original <Editor> component inside <InterfaceSkeleton actions={...}>.
| useTitle( isReady && __( 'Editor (beta)' ) ); | ||
|
|
||
| useURLQueryController(); |
There was a problem hiding this comment.
<URLQueryController> doesn't return anything and it should always run, we can just re-write it into a hook so that we don't have to worry about where to put it.
| import NavigationSidebar from '../navigation-sidebar'; | ||
| import { store as editSiteStore } from '../../store'; | ||
|
|
||
| function Layout( { |
There was a problem hiding this comment.
This component extracts the common parts of two screens (especially <InterfaceSkeleton> and <NavigationSidebar>) so that they can both share the same parents.
| @@ -74,22 +40,18 @@ export default function List() { | |||
| : undefined; | |||
|
|
|||
| return ( | |||
| <InterfaceSkeleton | |||
| className={ classnames( 'edit-site-list', { | |||
| 'is-navigation-open': isNavigationOpen, | |||
There was a problem hiding this comment.
I just found out that we can use body.is-navigation-sidebar-open for this.
| // Don't focus for the initial page redirection. | ||
| // TODO: This can be removed once #36873 is resolved. | ||
| if ( expectRedirectionRef.current ) { | ||
| expectRedirectionRef.current = false; | ||
| if ( history.action === Action.Replace ) { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Hopefully we can remove all these once #36873 is resolved and make this hook easier to understand.
| import getIsListPage from '../../utils/get-is-list-page'; | ||
|
|
||
| export default function EditSiteApp( { reboot } ) { | ||
| return ( | ||
| <SlotFillProvider> | ||
| <UnsavedChangesWarning /> |
There was a problem hiding this comment.
<UnsavedChangesWarning> is moved to <Layout>.
kevin940726
requested review from
noisysocks,
talldan,
Mamaduka and
alexstine
|
@kevin940726 This isn't going to work in it's current state.
This isn't good UX to move focus at this point because the menu is open and users are interacting with the menu. useEffect firing on every URL change is a major drawback but to be completely honest, I wouldn't know how else to do this. In my PR, I only switch focus back when the menu is closed but as you pointed out, this has it's own drawbacks. This is super difficult to figure out because if the menu triggered some dialog on close, focus may not be placed in the dialog and instead on the menu trigger. However, your solution makes focus happen too often on every page URL update. Even if the user is interacting with the menu, focus is still placed on the trigger. Would be cool if we could somehow combine our solutions that way useEffect only fires if the menu is closed and URL change occurs. Thoughts? Please let me know if this doesn't make sense. Happy to try explaining another way. Thanks. |
|
@kevin940726 I think I may have come up with a suitable solution. I don't have write access to wordpress/gutenberg, but I attached a diff to my comment. The only bug I found with this approach is it tries to focus the canvas/frame first. It looks like there's already focus management happening somewhere and it is based off which code runs first. Would be nice to be able to track that down. Please let me know if I need to explain further. Thanks. |
|
Small fix, but still trying to track down where the focus is happening after template/template part item is selected. |
|
@alexstine Thanks for the test! I was also wondering about this, but I tested some websites out there and they try to refocus even if clicking on the nav link (For instance, Reach UI and Gatsby). (I just found out Reakit is doing what you suggested though). Fortunately, I don't think the fix is that complicated, I just pushed it to d60ce2a. Hopefully it's more robust and match the semantics of what you mean here. Let me know if there's any gotchas! 🙇♂️ |
|
@kevin940726 Just gave it a test and yes, it is much simpler than my code. I was trying to pass the state from the store and you simply used native JS functions. It seems to be working well but that weird focus bug is still happening. After I select a template or template part, it focuses the canvas editor and then focuses the Toggle navigation button. It must be because useEffect is slower than another focus event or something. Any ideas on how to track down this other focus event? Other than that, this is good from A11Y point of view. Thanks. |
Hmm 🤔 , I didn't experience that. I'll take a look! |
|
@kevin940726 You want to get back to working on this? I know my solution was a bit of a hack so anyway I can help move this forward might be a good thing for the future. |
|
@alexstine Yeah sure! I have some other things I'm doing right now, but I'd like to come back to this maybe next week. I'll rebase the PR and ask for another round of reviews. I'll also ask around to see if this is still relevant too (if there's a design revamp planned ahead for instance). |
kevin940726
force-pushed
the
fix/edit-site-focus-management
branch
from
d60ce2a
to
191b2af
Compare
|
@alexstine I just rebased this, I think this is ready for another round of reviews. |
kevin940726
force-pushed
the
fix/edit-site-focus-management
branch
from
d9fffa4
to
2157324
Compare
|
@kevin940726 The focus is certainly working now as I would expect it to. Nice job. The only thing I still don't like is the page title seems to never update anymore. Why is it that when I select a template part, it doesn't change the page title? It remains as Editor (beta). I would expect it to read "Editing template part: Header (Dark, large)" just like the first heading on the page says. Is that out of scope for this PR or can you work on it? Seems like it was working at one time but stopped. Thanks. |
This seems like a bug! Not sure if it's related to this PR, I'll take a look! Thanks for spotting it! |
|
@kevin940726 Is there anything from my side blocking this PR? The issue I started discussing above can likely be split out and fixed in another PR. |
Sorry, the issue was bigger than I thought and I didn't have time to look deeper into it at the time. I think this is worth a revisit but I don't have much capacity for it recently. If anyone wants to take it over or start a new one with a different approach, feel free to go for it! |
|
I think this one can be closed out now. Would likely be a lot of work to bring it back to life. |
Description
Part of #36597. Alternative to #37265.
Continue from #36488, this PR refactors a large part of the original PR to optimize the render times needed when switching to another route.
This PR will reset the focus to the navigation toggle button on route change. In order to prevent it from focusing on the button on initial page load, we create a flag
isInitialPageLoadRefand set it to false after the first render.gutenberg/packages/edit-site/src/components/routes/use-reset-focus-on-route-change.js
Lines 13 to 20 in bdb289a
However, the button will remount whenever we switch from list page to the editor or the other way around. It causes
isInitialPageLoadRefto be reset totrueafter remount, thus skipping the focus.We could do some weird hack to make it work with minimal efforts, but a more robust fix will be to prevent the button from remounting in the first place. The button only remounts because we change the "parent" of it after switching to a different route. For instance, switching
from
<List><NavigationToggle /></List>to<Editor><NavigationToggle /></Editor>.It's called "Reparenting" in React, changing the parent of an element will inevitably remount itself. There are only a few options we could do to bypass that. The basic ideas are the same though, don't re-parent the element.
The approach this PR takes is to use a
renderLayoutfunction to render the route layout rather than using yet another component. The advantage of render function here is that it doesn't count as a parent, since there's no component being rendered. The disadvantage is that we can't use hooks inside the render function, so we'll have to pass the state from its parent.First, we define the
renderLayoutfunction on the Route entry component:gutenberg/packages/edit-site/src/components/list/index.js
Lines 20 to 55 in bdb289a
Then, we just have to call the render function with all the required states in the parent (
<App>):gutenberg/packages/edit-site/src/components/app/index.js
Lines 46 to 49 in bdb289a
Since that
<Layout>will never re-parent, neither will<InterfaceSkeleton>and<NavigationToggle>underneath it, they will not remount on route change anymore. The fix for focus reset then become trivial and predictable.Note that this refactor doesn't just benefit this focus fix, but also improves the overall performance of the editor, since that it doesn't have to do unnecessary remounts anymore.
This is a big change though, so if we can't get enough testing, I agree to postpone it after 5.9. We can fix focus management in another smaller PR (like #37265).
How has this been tested?
tt1-blockstheme or any other block-based themeTypes of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).