Use TabPanel in edit settings sidebar header (#13587) #16663

jffng · 2019-07-18T15:55:00Z

Description

This PR replaces the sidebar settings header with TabPanel from the components package.

No visual change, but a slight behavioral change in the keyboard navigation: only the active tab is placed in the tab order, and the user can navigate between tabs using the arrows.

This follows the WAI-ARIA recommendation on keyboard interaction for tab panel components:

Tab: When focus moves into the tab list, places focus on the active tab element . When the tab list contains the focus, moves focus to the next element in the page tab sequence outside the tablist, which is typically either the first focusable element inside the tab panel or the tab panel itself.

How has this been tested?

Tested in the playground and editor on:

macOS 10.14.5
Chrome 75.0.3770.100
Safari 12.1.1
Firefox 67.0.4

Types of changes

Addresses #13587

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

* Add styles to the component itself * Add aria-label prop to the TabButton * Replace markup with TabPanel component in edit settings sidebar header

jffng · 2019-07-18T21:58:35Z

Discovered two issues to address:

~~Rework some of the tests as the class selectors have changed.~~
~~Active tab class needs to change when the edit-post/block event fires~~

I ended up refactoring TabPanel with hooks to solve an issue where the initial state is set by a prop, but each re-render caused the component's active tab to lose focus.

Fix style border bug

ItsJonQ · 2019-07-23T13:16:05Z

packages/edit-post/src/components/sidebar/settings-header/index.js

+				onSelect={ onSelect }
+				controlledTabName={ selectedTabName }
+			>
+				{ () => { } }


@jffng Hallooo! This PR looks great! I was a little confused at this one (at first glance).

What do you think about adjusted the original TabPanel component's children() render to do something like...

children && children(selectedTab)

That way, you won't have to pass it in a children render function that returns null/empty.

P.S. I'm not sure what the current conventions are for handling this flow 😊. The flow I suggested feels little cleaner + less ambiguous

Thanks @ItsJonQ! I agree. We could also set children's default value to noop.

Aww yiss! noop it up! 🎉

Modify e2e tests to use arrow keys to switch between tabs and select using aria-label Allow settings header to control the active tab

mapk · 2019-07-24T22:45:40Z

Thanks @jffng for a fantastic PR, and thanks @ItsJonQ for the follow-up code review. I just tested this and love jumping around with the arrow keys. I did notice some keyboard nav weirdness though. When I was in the tabPanel, and "Document" was focused, hitting tab brought me to an in-between state where clicking Enter didn't do anything and nothing was focused. If I hit tab again, I was taken to the "Close" icon. Is this expected behavior?

I feel like that in-between tab position wants to rest on the "Block" tab, but isn't so it just leaves me hanging.

Would love an accessibility review here too!

gziolo · 2019-07-25T09:58:31Z

packages/components/src/tab-panel/README.md

@@ -133,9 +133,9 @@ The class to add to the active tab
 - Required: No
 - Default: `is-active`

-#### initialTabName
+#### controlledTabName


This is a breaking change and can't be landed as is. It needs to provide a way which ensures that this code is backward compatible with a solid deprecation strategy which informs developers that there is a new way to handle this prop.

Thanks for the review @gziolo, noted.

I added support for the initialTabName for backwards compatibility, as well deprecation notes in the README. However, I lack context as to the timing / version to officially deprecate this prop.

This is also assuming that having a prop to control the active tab from a parent component is a good idea / not an anti-pattern.

If we don't want to keep this param, we should rather remove it from docs and ensure that when used it works as close as possible as the new one.

gziolo · 2019-07-25T09:59:36Z

packages/components/src/tab-panel/style.scss

@@ -0,0 +1,46 @@
+.components-tab-panel__tabs {


@mapk are we fine with these styles to be the default look outside of the context of sidebar and WordPress in general?

jffng · 2019-07-25T14:17:44Z

When I was in the tabPanel, and "Document" was focused, hitting tab brought me to an in-between state where clicking Enter didn't do anything and nothing was focused. If I hit tab again, I was taken to the "Close" icon. Is this expected behavior?

No, nice catch @mapk. The component was rendering an empty container where the tab panel content would normally go (we're not actually using it in this case), and this empty container was receiving the focus in that in-between state. I pushed a fix that should resolve it, making the tab panel content optional.

Would love an accessibility review here too!

Yes!

gziolo · 2019-07-25T14:20:20Z

packages/edit-post/src/components/sidebar/settings-header/index.js

+		}
+	};
+
+	useEffect(


Did you consider adding onSelect to the objects passed to tabs instead? It might simplify the solution as you could leave the whole logic which reasons which tab is selected to TabPanel component.

I had considered it, but avoided that route as I was originally trying to minimize changes to TabPanel. I agree adding an onSelect method to the tab objects would simplify the solution. (We would still need to support the onSelect component handler for backwards compatibility)

The issue that led me to refactor TabPanel was the need to control the selected tab from the parent component without triggering a re-render of the entire component, causing the active tab to lose focus.

The issue that led me to refactor TabPanel was the need to control the selected tab from the parent component without triggering a re-render of the entire component, causing the active tab to lose focus.

Shouldn't the component ensure that there is never focus loss? 3rd party developers shouldn't care about it, it should be baked in into such component.

I will think about that, thank you!

I believe I was able to simplify the logic, please let me know if these changes address your feedback @gziolo, thanks.

Adjust test to three tab presses to navigate the block settings instead of four

gziolo

@jffng, the changes applied to PR look great. This is exactly what I would see as someone who consumes this component. Everything is handled behind the scenes and you don't need to worry about all the logic which handles navigation between tabs.

gziolo · 2019-07-29T12:52:55Z

packages/edit-post/src/components/sidebar/settings-header/index.js

-			</ul>
+			<TabPanel
+				tabs={ tabs }
+				controlledTabName={ sidebarName + '__panel-tab' }


Does it need to be explicitly set from outside or is it possible to append __panel-tab part internally in the TabPanel implementation?

gziolo · 2019-07-29T12:57:11Z

packages/edit-post/src/components/sidebar/settings-header/index.js

 		// translators: ARIA label for the Settings Sidebar tab, selected.
-		[ __( 'Block (selected)' ), 'is-active' ] :
+		__( 'Block (selected)' ) :


Maybe, the part which informs about the selected tab could be constructed inside the TabPanel from now on. It seems like we only append (selected) when a tab is selected.

Nice suggestion, this simplifies the settings header logic even further.

packages/edit-post/src/components/sidebar/settings-header/index.js

gziolo · 2019-07-31T11:45:12Z

@drw158 - how do you feel about the changes proposed in the context of the audit that you performed and documented in #16710?

As far as I see, TabPanel is used for the content of the tab in other libraries. I checked myself these two libs:

davewhitley · 2019-08-01T20:17:01Z

@gziolo I'm leaning towards either structure as shown in Reakit or Reach UI, especially if we rename our current Panel component.

<TabList {...tab} aria-label="My tabs">
    <Tab {...tab} stopId="tab1">
        Tab 1
    </Tab>
    <Tab {...tab} stopId="tab2" disabled>
        Tab 2
    </Tab>
    <Tab {...tab} stopId="tab3">
        Tab 3
    </Tab>
</TabList>
<TabPanel {...tab} stopId="tab1">
    Tab 1
</TabPanel>
<TabPanel {...tab} stopId="tab2">
    Tab 2
</TabPanel>
<TabPanel {...tab} stopId="tab3">
    Tab 3
</TabPanel>

annezazu · 2022-07-27T22:57:04Z

It seems the original issue was closed #13587 and this problem was addressed in another PR: #20872 Going to close this out as a result :) Feel free to re-open/ask for it to be if I am missing something.

Use TabPanel in edit settings sidebar header (WordPress#13587)

2f5414b

* Add styles to the component itself * Add aria-label prop to the TabButton * Replace markup with TabPanel component in edit settings sidebar header

jffng requested review from aduth, ajitbohra, chrisvanpatten, gziolo, jorgefilipecosta, talldan and youknowriad as code owners July 18, 2019 15:55

Toggle active states when block is selected

a9124b9

Fix style border bug

jffng requested review from nerrad and ntwb as code owners July 23, 2019 02:02

youknowriad requested review from jasmussen and mapk July 23, 2019 11:30

ItsJonQ reviewed Jul 23, 2019

View reviewed changes

Refactor TabPanel with useState and useEffect

bcf46a3

Modify e2e tests to use arrow keys to switch between tabs and select using aria-label Allow settings header to control the active tab

jffng force-pushed the try/sidebar-settings-tab-panel branch from b4542b8 to bcf46a3 Compare July 23, 2019 14:05

jffng mentioned this pull request Jul 23, 2019

WP components audit: TabPanel #16710

Closed

mapk added Needs Accessibility Feedback Need input from accessibility [Feature] UI Components Impacts or related to the UI component system labels Jul 24, 2019

gziolo requested changes Jul 25, 2019

View reviewed changes

gziolo added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jul 25, 2019

Backwards compatibility for initialTabName prop

cff31f0

gziolo reviewed Jul 25, 2019

View reviewed changes

Don't render the tab panel content container unless there's content

8733d7d

Adjust test to three tab presses to navigate the block settings instead of four

jffng force-pushed the try/sidebar-settings-tab-panel branch from 796cb66 to 8733d7d Compare July 25, 2019 16:03

Simplify settings header and tab panel logic

0686044

gziolo reviewed Jul 29, 2019

View reviewed changes

Differ aria-label selected text to TabPanel

84ab486

jffng force-pushed the try/sidebar-settings-tab-panel branch from 15ca57e to 84ab486 Compare July 29, 2019 18:00

gziolo removed the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jan 5, 2021

Base automatically changed from master to trunk March 1, 2021 15:42

annezazu closed this Jul 27, 2022

talldan mentioned this pull request Feb 1, 2023

Replace Post/Block tab switcher with TabPanel #43414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use TabPanel in edit settings sidebar header (#13587) #16663

Use TabPanel in edit settings sidebar header (#13587) #16663

jffng commented Jul 18, 2019

jffng commented Jul 18, 2019 •

edited

Loading

ItsJonQ Jul 23, 2019 •

edited

Loading

jffng Jul 23, 2019

ItsJonQ Jul 23, 2019

mapk commented Jul 24, 2019 •

edited

Loading

gziolo Jul 25, 2019

jffng Jul 25, 2019

gziolo Jul 25, 2019

gziolo Jul 25, 2019

jffng commented Jul 25, 2019

gziolo Jul 25, 2019

jffng Jul 25, 2019

gziolo Jul 25, 2019

jffng Jul 25, 2019

jffng Jul 26, 2019

gziolo left a comment

gziolo Jul 29, 2019

gziolo Jul 29, 2019

jffng Jul 29, 2019

gziolo commented Jul 31, 2019

davewhitley commented Aug 1, 2019 •

edited

Loading

annezazu commented Jul 27, 2022 •

edited

Loading

Use TabPanel in edit settings sidebar header (#13587) #16663

Use TabPanel in edit settings sidebar header (#13587) #16663

Conversation

jffng commented Jul 18, 2019

Description

How has this been tested?

Types of changes

Checklist:

jffng commented Jul 18, 2019 • edited Loading

ItsJonQ Jul 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapk commented Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jffng commented Jul 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Jul 31, 2019

davewhitley commented Aug 1, 2019 • edited Loading

annezazu commented Jul 27, 2022 • edited Loading

jffng commented Jul 18, 2019 •

edited

Loading

ItsJonQ Jul 23, 2019 •

edited

Loading

mapk commented Jul 24, 2019 •

edited

Loading

davewhitley commented Aug 1, 2019 •

edited

Loading

annezazu commented Jul 27, 2022 •

edited

Loading