List view: Modify the shortcut to focus while open #45135
Conversation
alexstine
added
[Type] Enhancement
|
|
|
Size Change: +153 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
| // This only fires when the list view is open because of the conditional rendering. It is the same shortcut to open but that is defined as a global shortcut and only fires when the list view is closed. | ||
| useShortcut( 'core/edit-post/toggle-list-view', () => { | ||
| // If the list view has focus, we know it is safe to close. | ||
| if ( listViewHasFocus ) { |
There was a problem hiding this comment.
Instead of a new state value, can we instead rely on listViewRef.current.contains( document.activeElement ) here ?
There was a problem hiding this comment.
@youknowriad That looks better? Seems to work the same.
|
It works really well in my testing. It's nice how it takes advantage of the roving tab index, so you can select a block from list view, and then use the shortcut to move straight back to that block in List View. I spotted one issue, which is that if the List view sidebar close button is focused then shortcut doesn't close the sidebar, instead it moves focus into the list view treegrid. I think that's because the Changing this It adds more complexity to the code, but it may be the best option in terms of usability. |
|
@talldan How does a solution like that look? This will need a new E2E test to make sure none of this breaks but it seems to have gotten me around the bug. Thanks for reviewing. |
|
Okay, added a fairly complex E2E for this. It should catch mistakes but this also helped me discover one other bug. Selecting the already selected block in the list view will not trigger focus. This is likely related to Thanks. |
alexstine
added
[Tool] E2E Test Utils
| listViewRef.current.ownerDocument.activeElement | ||
| ) | ||
| listViewRef.current | ||
| .closest( '[role="region"]' ) |
There was a problem hiding this comment.
Was this change needed to make sure the "close" button is within this div?
Can we instead just move the listViewRef to the div in line 62 of this file? The goal is to avoid relying on something that is not rendered by this component itself
There was a problem hiding this comment.
@youknowriad I gave it a try and updated the E2E test. How does this look?
|
I'm noticing a small issue when the post is empty (no blocks at all). I load the post editor, I hit the shortcut, first it opens the list view but then it's stuck I can't close it anymore. |
|
I'll try to investigate this shortly. Going out of town for a while. Tried setting up Gutenberg local dev on my Windows machine, interested to see if maybe I can improve the experience if I happen to find bugs while I am away from my Windows and Mac working environments over the LAN. Not all of us are lucky enough to fire up Linux or have Mac, this should be interesting. |
|
@alexstine yeah, I have trouble working when I'm not on my regular setup as well. My codesandbox PR might help here, we'll have to see in the long run. |
|
@youknowriad I pushed a fix but that still leaves us with a problem. The Might be time to refactor this a bit and call useEffect to handle our focus situations in the sidebar. Hate to do it, but I need full visibility over the state. useEffect would be perfect for that since I can listen for state changes. Thoughts? |
|
Just removed the I need to refresh the branch later today and make sure the label changes have not broken anything. |
|
I think this is mostly good to go. It might be worth breaking up the E2E test a bit if others think that is necessary. If all looks good, I'll apply to other packages. I think I finally managed to fix all bugs. |
|
During the |
|
@talldan or @andrewserong Any ideas as to why the above test is failing? |
|
Would be nice to get a review on this. Need some assistance debugging the test, it is more than I know how to do at this point. CC @annezazu |
|
Thanks for the ping, apologies for the late reply, I must have missed the ping while catching up after AFK.
I walked through the failing test manually in the post editor and I think it's failing because of a change in this PR when the list view is opened. With this PR applied, it seems that the focus is always over the first block in the list view when it opens, rather than the block that is currently selected within the editor canvas. For example, if I create a post containing four blocks, with the fourth block selected, if I open the list view, then on Was that part of the changes in this PR intentional? |
|
@andrewserong That is not supposed to happen. Maybe I am not sending focus through the component properly? Can you spare a few moments to help me make this stable? So close. |
|
I took another look through the code, and I'm wondering if the reason the focus is going elsewhere is because the ref is now attached to the parent document overview container, which has tabbable elements within it, whereas previously the ref was attached to the container for the list view. So on trunk, I think that meant that However now that the ref is a level higher, I'm a little stretched for time this week (I'm off for a public holiday tomorrow), but I should have a little time on Friday to take another look. |
…view-modify-shortcut-focus
|
@andrewserong I could not reproduce the same. If I select the 2nd block in the list and open the list view, I land on the 2nd block in the list view. I added |
|
Flaky tests detected in 7945440. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4057808775
|
|
@alexstine I'm a bit stuck attempting to re-review this one, as in testing One thing that stands out to me after another dive into the code, is that over in the My hunch at the moment is that the focus behaviour in this PR means that the cell ref's focus isn't being called, so the focus never gets placed to the selected block in the list view, and so it doesn't receive an updated What does this all mean for next steps? I'm wondering if it would help if we looked into limiting the scope of what this PR deals with so that Now that we've passed feature freeze for 6.2, I should have a little more time to dig in and play around with some of these ideas. If you're happy for me to, I can experiment in this PR. Otherwise, I'm also happy to open up a separate PR to try that out, as I don't want to overwrite any of your good work here! Thanks again for your patience with all this, it'll be a really good enhancement to land. |
|
@andrewserong Can you remind me which browser E2E runs in? I think it is Chrome? I generally develop in Firefox so that could be it. Let me record a video tomorrow of what my experience is and maybe you can spot something that I just do not get visually. I think the reason this PR fell a little out of scope is because having duplicate focus everywhere just did not make much sense to me. I wanted to try to make it clear to anyone in the future exactly what managed focus when as allowing the At least that is my take... This whole solution really is not pretty from a code perspective. Thanks. |
The failing e2e test uses Puppeteer to run the tests, and Puppeteer is based on Chromium browser, so that very well could be a factor here. Also, I know what you mean about dealing with the managed focus, it's pretty tricky for folks to follow along when logic is spread across components, or is happening in different places. It took me quite a while to spot the I'll continue to have a play around with this also, and I'll let you know what I find! |
|
Update: it's pretty messy, but I've had a go at hacking on a change based on this PR over in #47901, specifically within list-view-sidebar.js. It's very much a throwaway PR, so feel free to disregard it, but for the moment I've been playing with the following:
In manual testing, that appears to resolve the issue I ran into, however I think the tests are still failing in a different way, so might not be a reliable solution. I mostly put up the code change just in case it sparks any ideas! I have limited time tomorrow to look into this further, so if not tomorrow, then I can continue digging in on Monday. |
…view-modify-shortcut-focus
|
@andrewserong I actually like the idea of doing less lookups so I removed that aspect from my PR. I still require a couple lookups but I have used more refs in my latest commit. I also made a video showing the difference between my PR and trunk. I think we'll likely have to update the E2E tests. I think I get what you are saying now. Thoughts? Thanks. video1197881057.mp4 |
There was a problem hiding this comment.
Thanks so much for adding the video @alexstine, it's super helpful hearing you talk through the behaviour of the feature, and it's also nice to hear your voice! You make a good point comparing how the feature works on trunk, and honing in on the changes of this PR.
After giving this PR a re-test, I think your latest refactor has fixed up the main difference between this branch and trunk, and the previously failing e2e test is now passing. While testing manually, it seems that this branch now has the same behaviour as trunk for me when initially opening the list view, either by clicking on the list view button or using the keyboard shortcut. So, I think the addition of the extra refs and calling find on listViewApplicationFocus seems to have fixed up any disparity there.
I think this PR is in a good place to land now. The feature as implemented is working correctly, the e2es are passing, and otherwise the existing behaviour when opening the list view appears to be the same between trunk and this PR. I imagine some of the inconsistency on trunk could be to do with when the cell ref within the list view sets focus. It seems like it might be occurring inconsistently over there, but that's an issue that could be investigated separately.
Great work again, this LGTM! ✨
There was a problem hiding this comment.
Thanks for persisting with this! It's working well for me on macOS with Chrome and Safari, although it doesn't work with VoiceOver, due to the keyboard shortcut on mac being ctrl+opt+o. That is an existing issue tracked by #11154, so definitely not a blocker for this PR.
I also checked that with the list view open, it's still possible to reach it from inside a block by Shift+Tabbing a couple of times (first Shift+Tab lands us in the block toolbar, second in the list view).
One thing that would be nice to look at in a follow-up: when moving focus from the canvas to the already open list view, as far as I can tell focus is always transferred to the block that was last focused in the list view. So, if I have a Paragraph in the list view, move focus to the canvas, and then press Enter to create a second Paragraph, if I then move focus back to the list view it lands on the first Paragraph. This is also existing behaviour: I can reproduce it on trunk when Shift+Tabbing into the open list view from the canvas.
|
@tellthemachines Good call out. I think we'll need to figure out how to change that in the I am ready to give this a try and see if anyone notices any issues. Over the next few days, I will try to get this going for the site editor. Thanks all. |
What?
Fixes #29466
This PR does the following.
Why?
See the related issue for more information. This allows the list view to remain visible for the sighted and still allows keyboard users to navigate back to the list view without closing and re-opening.
How?
It is not a pretty solution but it works. I left comments in the code to guide. The rest is magic.
Testing Instructions
Additional thoughts
useFocusReturnhook needs some refactoring or different usage for this. Since it attaches when the component mounts, it will not return focus to where the user expects if they have navigated around the list view for a while. I need some way to call the hook dynamically but my research is turning up nothing. Suggestions on how to fix this would be appreciated.It seems it would be better to give the sidebars it's own keyboard shortcuts. That would make abstraction of future functionality easier as well. For now, I think I'll leave it be. Being able to pass things from parent to child would likely help but we're not setup for it now.
Screenshots or screencast