Add TabPanel to document overview replacing fake tabs #50199
Conversation
… parent refs to wrapping content div.
alexstine
added
[Type] Enhancement
There was a problem hiding this comment.
I am going to need some help with styling I am sure. Probably do not need all these divs since the TabPanel adds some of them as well. I am also expecting failing E2Es from the long set of tests I wrote around this. I will try to fix them up once reported back.
| orientation = 'horizontal', | ||
| activeClass = 'is-active', | ||
| onSelect, | ||
| itemRef, |
There was a problem hiding this comment.
Not sure why TS is not forcing me to type this. also not sure how to type it. I would expect this value to take either a single ref or merged refs from useMergedRefs hook.
| @@ -190,12 +196,14 @@ export function TabPanel( { | |||
| role="tabpanel" | |||
| id={ `${ selectedId }-view` } | |||
| className="components-tab-panel__tab-content" | |||
| ref={ itemRef || undefined } | |||
There was a problem hiding this comment.
This should never be required, only complementary. Useful for managing focus in rendered children.
There was a problem hiding this comment.
As context, the components crew is working on a new version of the TabPanel component that would support these kinds of features in a more ergonomic API. So ideally, I'd like to make this fix to the Document Overview without adding new APIs to the current TabPanel component (i.e. the itemRef prop).
Would it be possible to handle the item ref concerns directly in the consumer (.edit-post-editor__list-view-container) without sending it through the TabPanel component?
There was a problem hiding this comment.
I do not think so, not for this case specifically. I need a way to always make sure I know where my ref is attached for focus reasons. I hate to do it but trying to guess focus is not going to work for this. I could try to refactor it but it will likely not be pretty.
There was a problem hiding this comment.
Just to be clear, I mean something like this. It seems to work but I may not be picking up on all the focus management concerns — are there any requirements that this doesn't cover?
There was a problem hiding this comment.
Let me give it a test later and see. Thanks for the quick commit @mirka. If it works, it will indeed be simpler than what I was trying to do. I kept getting a rules of hooks violation but I didn't think about using useMergeRefs() earlier.
| ref={ useMergeRefs( [ | ||
| <TabPanel | ||
| ref={ tabPanelRef } | ||
| selectOnMove={ false } |
There was a problem hiding this comment.
Manual activation due to focus handling with the list view.
|
Size Change: -63 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice work @alexstine, the keyboard navigation is testing really well for me with this PR 👍
The change introduces some visual issues that will need to be addressed, as you mention. I think most of them should be do-able via making adjustments to the components-tab-panel__tabs class. I've hacked around locally, and managed to get the header looking pretty similar to trunk via something like the following (except for the close button):
.components-tab-panel__tabs {
border-bottom: $border-width solid $gray-300;
box-sizing: border-box;
display: flex;
width: 100%;
padding-right: $grid-unit-70;
.components-button {
width: 50%;
}
}We'd then need to get the close button to be positioned absolutely over that header, a little bit like how the Style Book does it here and here.
In the above snippet, we should hopefully be able to replace .components-button with the classnames for each of the tabs. But I'm wondering if it means if we'll also want to pass down a custom classname for the NavigableMenu so that we don't depend on .components-tab-panel__tabs. At the moment TabPanel allows us to provide a classname either at the wrapper level or for the individual tabs, so we might need an extra prop for this additional classname, so that we can avoid depending on the internal tabs classname, if it's necessary. But overall, this seems quite do-able to me!
I've got a few other tasks I'm working on at the moment, but I'm happy to help out with this PR in the next week or two if no-one else beats me to it.
I'll just CC @mirka and @ciampo in case they have feedback about how to override styles for the header / tablist of the TabPanel.
|
Thanks for the quick review @andrewserong . Just like the other PR we worked on, feel free to push styling fixes. Hopefully, this week, I can dedicate some time to working on the failing E2E tests. |
|
Thanks for confirming, Alex! I should have a bit of time to jump in to try pushing styling fixes next week. |
alexstine
added
the
[Focus] Accessibility (a11y)
There was a problem hiding this comment.
I've just been exploring some styling fixes to get this to more closely match trunk visually. In the process, I believe we can remove the edit-post-editor__document-overview-panel-header wrapper div for the button, and instead position the button using position: absolute.
One thing I wasn't sure of, though, is that removing that wrapper div for the button will also remove the headerFocusReturnRef. Do you know if that's still needed @alexstine? If it isn't needed, then I think getting the button to play nicely should be fairly straightforward, and I can push a commit on Monday to get the styling most of the way there.
The other thing I'm looking into is that switching to the TabPanel appears to cause the vertical scrollbar not to appear for the list view, and instead the content overflows, creating a bigger scrollbar for the whole page. I'll dig into that a little more next week, too.
|
@andrewserong Unfortunately, that ref is still required due to some type of bug that causes focus loss on list view close. All this needs to be refactored at some point. |
Thanks for confirming @alexstine! I think I've managed to get the styles to mostly look the same for the TabPanel as on There's still an outstanding issue with the vertical scrollbar for the list view. I should hopefully have a bit of time this week to continue digging into that. |
|
Flaky tests detected in 362e0e9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5096587535
|
|
I'm not familiar with the code nor the test, I can take a look but @Mamaduka might be able to spot the error faster than me 😉. |
|
I think there're regressions in Document Overview behavior that causes e2e test failure. I spotted two.
|
|
@Mamaduka Which file is number 2 occuring in? E2E logs can be very hard for me to read. :( For number 1, I do not know how to fix it. I think it's the same bug that @andrewserong and I discovered at the beginning of this project. The list view and canvas are not in sync completely. |
|
@alexstine, the file is |
packages/edit-post/src/components/secondary-sidebar/list-view-sidebar.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the updates @alexstine and for fixing up the e2e tests! 👍
I've just given this a re-test, and this is still testing well for me across browsers, and the code change LGTM.
When you open the list view currently selected block doesn't receive a focus. Instead, the first item in the list gets the focus. This isn't the cause of the failure, but we should fix it.
Good catch! This behaviour appears to be the same as an on trunk as far as I can tell. In my testing, the selected block receives focus if it is at the root of the document. For nested blocks, I think what's happening is that if you select a nested block, when you open the list view, the list isn't yet expanded, and so focus is directed to the first item in the list. Then, very soon after the list view is opened, the useEffect in useListViewExpandSelectedItem fires, which expands the parent nodes in the tree, so the selected block becomes visible. However, it seems that happens after the focus logic. A fix could potentially be for the expand selected item logic to including focusing behaviour, too — however, if a user is navigating the editor canvas with the list view sidebar open, then they likely won't be wanting focus to shift over to the list view unexpectedly, so there might be some nuance to getting the logic right.
It'd be a good thing to fix up, but from the sounds of it, I think that's probably outside the scope of this PR as the switch to the TabPanel appears to be unrelated as far as I can tell. Could be a good follow-up to dig into.
This PR is in a good shape to land to me! Nice work @alexstine 🎉
|
Yeah, based on that description, I think there is not an easy fix. The focus management has to be contained to the sidebar based on how the tab rendering works and how I am detecting focus in the sidebar. The focus code checks for the first tabbable and that attribute changes in the If focus is on inner block and parent is collapsed, expand and then render the proper The other less complicated option might be using the I guess this is all to say, I'd be happy to work on a follow-up but I do not see an easy way forward. This bug I bet was introduced when collapse by default went into trunk and the lack of E2E tests meant we never caught it. Now it comes up from time to time when I make these PRs. :) If no further feedback, I'll merge this one by end of day tomorrow. Thanks. |
|
Addressing the selected block focus issue in a separate PR makes sense. I've one more piece of feedback that might be outside this change. I wanted to highlight the difference between navigating the "Editor settings" and "Document Overview" region tabs. I'm using CTRL + ` to switch between regions, and then each step represents Tab navigation. Editor settings
Document Overview
I know that "Editor settings" currently don't use the |
Exactly, the Editor Settings tabs are implemented incorrectly at the moment, including semantics and keyboard navigation. Tracked in #43414. |
|
I expected as much, but thanks for the clarification, @mirka! |
|
Yep, my hope is to see that changed soon. I might even attempt a PR if someone wants to come behind me and cleanup styles. What blocked us for a while is the fact that the |
* Add TabPanel to document overview sidebar. * TabPanel: Changed to support ref forwarding. Added prop for attaching parent refs to wrapping content div. * Reviewer feedback addressed. * Fix E2E tests. * Update styles to more closely reflect the look of the tabs in trunk * Restore scrollbar behaviour * Add changelog entry for components package * Better comments. * Fix E2E. * Add back useState for cleaner code. * Fix list view prop. Changelog update. * Fix tab panel focus. Still need to use focus.tabbable.find(). * Fix changelog format. --------- Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
What?
Adds the
TabPanelto the document overview and removes the fake tab buttons. Makes some adjustments to theTabPanelnecessary for this to work.Why?
Better accessibility/UX.
How?
See above.
Testing Instructions
TabPanelinstead of buttons.Testing Instructions for Keyboard
The
TabPanelis perfectly accessible via the keyboard. Access it by pressing tab, left/right arrow keys will cycle options, and enter to activate.Screenshots or screencast