Implement Tabs in editor settings
#55360
Conversation
chad1008
force-pushed
the
tabs-editor-settings
branch
from
4a26939 to
3e762bb
Compare
chad1008
force-pushed
the
tabs-editor-settings
branch
from
3e762bb to
f045279
Compare
|
Size Change: -495 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 5d20c8b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7148476902
|
chad1008
force-pushed
the
tabs-editor-settings
branch
from
f045279 to
c11a67f
Compare
There was a problem hiding this comment.
- While writing the above testing steps, I discovered the
Tabsimplementation is breaking the close button on the sidebar. Activating that button via mouse or keyboard triggers amaximum update deptherror from React. That button is provided by theComplementaryAreaHeadersubcomponent. I'm looking into why and how to address that.
Interesting. Sounds like the click handler there is triggering some sort of infinite loop.
- By default, Tabs should place the focus on the
tablistelement, and then navigate between individualtabswith arrow keys. Arrowing to a tab should activate it automatically. With this implementation, when tabbing over to the sidebar the focus lands directly on the firsttab(which semantically is abutton).
When looking at the Tabs storybook example, I can't see the tablist element receiving focus. Pressing tab seems to move focus directly to the selected tab.
You then use the arrow keys to move between tabs/buttons, but because the actual button is focused and not the
tablist, you need to explicitly activate the button with enter/return/space. I suspect this is being caused Tabs being nested inside aComplementaryAreabut need to look into it further.
I'm not sure I understand exactly what is going on. My expectation is that pressing arrow keys when the individual tab buttons are focused should move the tab selection (no need for tablist to be focused).
packages/edit-post/src/components/sidebar/settings-header/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-header/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-header/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
Yeah, you're right, sorry. I was misremembering. Focus does go to the buttons. Here's what the difference in keyboard navigation looks like, after looking more closely:
this PR:
So the focus is indeed going to the right place, but navigation and tab selection for keyboard users are a bit off when compared to expected |
|
I wonder if that's related to |
chad1008
force-pushed
the
tabs-editor-settings
branch
from
6aa72d6 to
6b72d41
Compare
I know when you and I looked at this together the other day, My test diff:diff --git a/packages/interface/src/components/complementary-area/index.js b/packages/interface/src/components/complementary-area/index.js
index 887c447d92..aa82f0e298 100644
--- a/packages/interface/src/components/complementary-area/index.js
+++ b/packages/interface/src/components/complementary-area/index.js
@@ -196,14 +196,7 @@ function ComplementaryArea( {
</ComplementaryAreaMoreMenuItem>
) }
{ isActive && (
- <ComplementaryAreaFill
- className={ classnames(
- 'interface-complementary-area',
- className
- ) }
- scope={ scope }
- id={ identifier.replace( '/', ':' ) }
- >
+ <div className={ 'interface-complementary-area' }>
<ComplementaryAreaHeader
className={ headerClassName }
closeLabel={ closeLabel }
@@ -244,7 +237,7 @@ function ComplementaryArea( {
) }
</ComplementaryAreaHeader>
<Panel className={ panelClassName }>{ children }</Panel>
- </ComplementaryAreaFill>
+ </div>
) }
</>
);
It messes up the UI placement, and the tab sequence is a bit awkward this way... but once things aren't being routed through a
So removing the I was talking this over with @brookewp yesterday as well, and she made an interesting observation. Keyboard navigation on this PR is behaving a little like the @ciampo, you shared the concern with me that it's possible some of Ariakit's internal store/context is what isn't making it through the @jsnajdr, I know you're pretty well-versed in Edit to add: I did try to recreate the problem in a simplified example as well, albeit unsuccessfully. There's a temporary Slot Fill Problem Example pushed to this PR that tries to replicate the issue, even on that story, complete with |
The first thing to check is what type of slot/fill your ComplementaryArea is. Is it the "bubblesVirtually" type, or the "classic" type? With "bubblesVirtually", a component is rendered in the React context of the Fill, although in DOM it's rendered under the Slot's elements. It doesn't see the Slot's React context. So, if the Slot is rendered as a child of some Ariakit component, the parent's Ariakit context is not visible to it. This is a common problem, and we've discussed this with @ciampo before, mainly in #51264 which introduced a new forwaredContext API. |
Thanks @jsnajdr! This particular |
There was a problem hiding this comment.
I took some time to look into this properly, and found out what is causing the issues highlighted above (spoiler alert — it's not Slot/Fill!)
Problem 1: keyboard interaction on tabs is broken
When I started looking at the symptoms I was particularly curious about the fact that, while keyboard interactions were not working, clicking individual tabs was working.
I then noticed that the Tab components have dedicated onClick handlers, which made me realize that the Tabs component is not being controlled correctly. The right way to control the component is via the onSelect callback on the Tabs component, while there's no need for those onClick handlers.
I tested those changes on my machine, and all interactions on the tabs started working again (click to expand)
diff --git a/packages/edit-post/src/components/sidebar/settings-header/index.js b/packages/edit-post/src/components/sidebar/settings-header/index.js
index 37045cb697..6c5644acdb 100644
--- a/packages/edit-post/src/components/sidebar/settings-header/index.js
+++ b/packages/edit-post/src/components/sidebar/settings-header/index.js
@@ -15,11 +15,6 @@ import { unlock } from '../../../lock-unlock';
const { Tabs } = unlock( componentsPrivateApis );
const SettingsHeader = ( { sidebarName } ) => {
- const { openGeneralSidebar } = useDispatch( editPostStore );
- const openDocumentSettings = () =>
- openGeneralSidebar( 'edit-post/document' );
- const openBlockSettings = () => openGeneralSidebar( 'edit-post/block' );
-
const { documentLabel, isTemplateMode } = useSelect( ( select ) => {
const postTypeLabel = select( editorStore ).getPostTypeLabel();
@@ -53,7 +48,6 @@ const SettingsHeader = ( { sidebarName } ) => {
<Tabs.TabList>
<Tabs.Tab
id={ 'edit-post/document' }
- onClick={ openDocumentSettings }
aria-label={
isTemplateMode ? templateAriaLabel : documentAriaLabel
}
@@ -65,7 +59,6 @@ const SettingsHeader = ( { sidebarName } ) => {
</Tabs.Tab>
<Tabs.Tab
id={ 'edit-post/block' }
- onClick={ openBlockSettings }
aria-label={ blockAriaLabel }
// translators: Data label for the Block Settings Sidebar tab.
data-label={ __( 'Block' ) }
diff --git a/packages/edit-post/src/components/sidebar/settings-sidebar/index.js b/packages/edit-post/src/components/sidebar/settings-sidebar/index.js
index e8681205b6..be654060a6 100644
--- a/packages/edit-post/src/components/sidebar/settings-sidebar/index.js
+++ b/packages/edit-post/src/components/sidebar/settings-sidebar/index.js
@@ -5,8 +5,8 @@ import {
BlockInspector,
store as blockEditorStore,
} from '@wordpress/block-editor';
-import { useSelect } from '@wordpress/data';
-import { Platform, useContext } from '@wordpress/element';
+import { useSelect, useDispatch } from '@wordpress/data';
+import { Platform, useContext, useCallback } from '@wordpress/element';
import { isRTL, __ } from '@wordpress/i18n';
import { drawerLeft, drawerRight } from '@wordpress/icons';
import { store as interfaceStore } from '@wordpress/interface';
@@ -73,6 +73,8 @@ const SettingsSidebar = () => {
[]
);
+ const { openGeneralSidebar } = useDispatch( editPostStore );
+
const Content = () => {
// Because `PluginSidebarEditPost` renders a `ComplementaryArea`, we
// need to forward the `Tabs` context so it can be passed through the
@@ -120,8 +122,17 @@ const SettingsSidebar = () => {
);
};
+ const onTabsSelect = useCallback(
+ ( newSelectedTabId ) => {
+ if ( !! newSelectedTabId ) {
+ openGeneralSidebar( newSelectedTabId );
+ }
+ },
+ [ openGeneralSidebar ]
+ );
+
return (
- <Tabs selectedTabId={ sidebarName }>
+ <Tabs selectedTabId={ sidebarName } onSelect={ onTabsSelect }>
<Content />
</Tabs>
);
Problem 2: closing the sidebar causes the editor to crash
When looking at this issue, I started looking at the console error, which suggested that the component rendering infinitely was the complementary area's Fill, which is where the tabs are rendered.
Looking at the changes that this PR introduces, one of the first suspects for causing those extra re-renders was the part, in the SettingsSidebar component, where we have to manually forward the Tabs.Context to the SettingsHeader component. I tried to comment the context provider out, and closing the sidebar started working again.
Now, that couldn't be the fix, but at least it was a big hint: the issue seems to be related to the Tabs component and its context. I then took a look at the Tabs component, and immediately noticed that the internal context value is not memorized, which means that a new object is going to be created every time the component re-renders. I wrapped the context value in a useMemo hook, and reloaded the editor.
Closing the sidebar still crashed, but the error message was different. Instead of happening in the Fill component, not the infinite rendering loop was happening directly in the Tabs component. I therefore started looking at the code inside the Tabs component, in particular at the three useLayoutEffect hooks. One of them, in particular, runs only when the component is used in controlled mode. I commented it out, and closing the sidebar started to work again!
So, what is happening? Basically, when the sidebar is closed, all the different tabs are removed from the page, which means that this line of code can't find a selected tab (since the array of items is empty). And therefore, this line of code runs, trying to reset the selected tab id to null. Depending on how the controlled Tabs implementation was coded (see how I solved problem 1), this may cause an infinite loop. Therefore, the solution that I found here was to pass selectedTabId={null} to Tabs when the sidebar is closed.
These are changes that I applied on my machine (click to expand)
diff --git a/packages/components/src/tabs/index.tsx b/packages/components/src/tabs/index.tsx
index 826c4e7c9d..5273e4c21f 100644
--- a/packages/components/src/tabs/index.tsx
+++ b/packages/components/src/tabs/index.tsx
@@ -8,7 +8,7 @@ import * as Ariakit from '@ariakit/react';
* WordPress dependencies
*/
import { useInstanceId } from '@wordpress/compose';
-import { useLayoutEffect, useRef } from '@wordpress/element';
+import { useLayoutEffect, useMemo, useRef } from '@wordpress/element';
/**
* Internal dependencies
@@ -154,8 +154,13 @@ function Tabs( {
setSelectedId,
] );
+ const contextValue = useMemo(
+ () => ( { store, instanceId } ),
+ [ store, instanceId ]
+ );
+
return (
- <TabsContext.Provider value={ { store, instanceId } }>
+ <TabsContext.Provider value={ contextValue }>
{ children }
</TabsContext.Provider>
);
diff --git a/packages/edit-post/src/components/sidebar/settings-sidebar/index.js b/packages/edit-post/src/components/sidebar/settings-sidebar/index.js
index be654060a6..89a756d258 100644
--- a/packages/edit-post/src/components/sidebar/settings-sidebar/index.js
+++ b/packages/edit-post/src/components/sidebar/settings-sidebar/index.js
@@ -39,8 +39,8 @@ const SIDEBAR_ACTIVE_BY_DEFAULT = Platform.select( {
} );
const SettingsSidebar = () => {
- const { sidebarName, keyboardShortcut, isTemplateMode } = useSelect(
- ( select ) => {
+ const { sidebarName, isSidebarOpen, keyboardShortcut, isTemplateMode } =
+ useSelect( ( select ) => {
// The settings sidebar is used by the edit-post/document and edit-post/block sidebars.
// sidebarName represents the sidebar that is active or that should be active when the SettingsSidebar toggle button is pressed.
// If one of the two sidebars is active the component will contain the content of that sidebar.
@@ -51,6 +51,7 @@ const SettingsSidebar = () => {
let sidebar = select( interfaceStore ).getActiveComplementaryArea(
editPostStore.name
);
+ const isOpen = sidebar !== null;
if (
! [ 'edit-post/document', 'edit-post/block' ].includes(
sidebar
@@ -66,12 +67,11 @@ const SettingsSidebar = () => {
).getShortcutRepresentation( 'core/edit-post/toggle-sidebar' );
return {
sidebarName: sidebar,
+ isSidebarOpen: isOpen,
keyboardShortcut: shortcut,
isTemplateMode: select( editPostStore ).isEditingTemplate(),
};
- },
- []
- );
+ }, [] );
const { openGeneralSidebar } = useDispatch( editPostStore );
@@ -132,7 +132,10 @@ const SettingsSidebar = () => {
);
return (
- <Tabs selectedTabId={ sidebarName } onSelect={ onTabsSelect }>
+ <Tabs
+ selectedTabId={ isSidebarOpen ? sidebarName : null }
+ onSelect={ onTabsSelect }
+ >
<Content />
</Tabs>
);
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-header/index.js
Outdated
Show resolved
Hide resolved
chad1008
force-pushed
the
tabs-editor-settings
branch
2 times, most recently
from
9ce52a0 to
d72b562
Compare
|
Thank you for debugging there @ciampo! I've updated the I was following a similar thread for the closed-sidebar-loop-of-infinite-doom, but you arrived at a speedier and more effective solution than I did, so I've applied those changes as well. I believe I've applied or replied to all open comments, but please let me know if I've missed anything. Oh, there are some things that will no longer work with this PR. Specifically, anything that relied on changes to |
|
rebased ✅ |
chad1008
force-pushed
the
tabs-editor-settings
branch
from
d72b562 to
27f696f
Compare
There was a problem hiding this comment.
Tabs seem to be working well!
I that there's a double border now showing:
That's because we're now wrapping each tab panel contents into a TabPanel component (where previously we had a react fragment, ie. no dom element). The extra div rendered by the TabPanel between the Panel and the PanelBody causes this rule not to apply anymore, which causes the top border of the first panel body to show.
The simplest solution (without involving style overrides) IMO could be to pass a className to PluginSidebarEditPost, through which we apply a margin-top: -1px style 🤞 We should add a comment to why that is necessary, though.
Apart from that, I think that this PR can be marked as ready for review. It would be also be great if @alexstine could take a look (I know he's been waiting for this part of the editor to have accessible tabs in a while!)
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
chad1008
added
[Type] Enhancement
chad1008
force-pushed
the
tabs-editor-settings
branch
from
c877b53 to
4d0c547
Compare
Yep! I actually just popped back on after dinner to wrap up (what should be) the last test failure. That now-defunct classname was the culprit in some other spots as well. Checks are running now 🤞 This PR has also been updated and rebased to account for the new controlled mode focus behavior, and the new |
|
Green is my new favorite color. |
|
I'm almost certain (hard to tell because of the margin error in our performance metrics) that this PR introduced a small performance regression in both "typing" and "block select" metric. Not saying we should revert but it would be good to understand why. https://www.codevitals.run/project/gutenberg |
Related: #52997
What?
Implement the new
Tabscomponent in the editor settings sidebarWhy?
Due to its more granular nature,
Tabsis better suited to more applications than its predecessor,TabPanel. Implementing it here will ensure it gets all of the accessibility and other benefits, and ongoing support, of the packaged component. This move also cuts down on the amount of custom CSS needed, as we're using a component with its own styles already built in that no longer need to be duplicated.How?
This PR modifies
Tabsslightly by exposing the context that the component uses to pass data to its subcomponents. The editor settings sidebar implements an internalComplimentaryArea, which in turn has an internalSlot/Fillthat we need to pass our context through.Now that we are exposing
Tabs.Contextwe can use the relevant provider to pass the context wherever it needs to go.Testing Instructions
Testing Instructions for Keyboard
Known Issues
Tabsimplementation is breaking the close button on the sidebar. Activating that button via mouse or keyboard triggers amaximum update deptherror from React. That button is provided by theComplementaryAreaHeadersubcomponent. I'm looking into why and how to address that.tablistelement, and then navigate between individualtabswith arrow keys. Arrowing to a tab should activate it automatically. With this implementation, when tabbing over to the sidebar the focus lands directly on the firsttab(which semantically is abutton). You then use the arrow keys to move between tabs/buttons, but because the actual button is focused and not thetablist, you need to explicitly activate the button with enter/return/space. I suspect this is being caused Tabs being nested inside aComplementaryAreabut need to look into it further.