Implement Tabs in site-editor settings
#56959
Conversation
|
Size Change: -251 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 07f355d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7278433086
|
chad1008
force-pushed
the
tabs-site-editor-settings
branch
2 times, most recently
from
4e9165d to
530c9b1
Compare
chad1008
requested review from
kevin940726,
Mamaduka,
ntwb,
nerrad and
ajitbohra
as code owners
chad1008
changed the title
Tabs in site-editor settingsTabs in site-editor settings
chad1008
force-pushed
the
tabs-site-editor-settings
branch
from
5a43b86 to
3d9c1dc
Compare
|
I see neither this PR or #55360 have the 'Accessibility' label or the 'Needs Accessibility feedback' label. It would be greatly appreciated that any change that is relevant for accessibility gets marked with those labels. Also, pinging some accessibility specialists for feedback would be appreciated. I would also like to note that I don't see any related GitHub issue for this PR nor for #55360. The contributing guidelines for repository management of this project state that, quoting:
Allowing broader discussion on important, impactful changes is essential in an open source project to make sure we deliver the best possible user experience. Also, I'm not sure not following the contributing guidelines is the best way to encourage collaboration between teams and contributors. That said, @joedolson @alexstine @andrewhayward are we OK with changing the buttons to switch the settings panel to an ARIA tabs interface? I would like to remind everyone that not using ARIA tabs here was a deliberate choice made a few years ago (in 2018). See for example here: #8079 (comment) The point is that the main advantage of ARIA tabs is that they reduce the amount of tab stops to navigate the user interface with the keyboard. That makes perfectly sense when there are many tabs. Personally, I doubt it brings in any advantage when there's only two tabs. That's just my personal oopinion though and I'd like to hear some thoughts from other accessibility specialists. To be clear: I do realize an ARIA tabs interface is 'more standard' but I still doubt it's a better user experience in this specific case. In any case, if we go with an ARIA tabs interface, I'm not sure that in this specific case the activation of the panels should be automatic. There are two patterns: Automatic activation and Manual activation. I think manual activation would be more appropriate here. Note that In the post editor these buttons have already been changed to ARIA tabs in #55360 |
@andrewhayward is an accessibility specialist and was indeed pinged on this issue (and other related ones), although we're more than happy to ping a wider group in the future.
There is a tracking issue related to the
Related to whether it's a good idea to use aria tabs, my personal opinion is it's better to provides a more standard, predictable experience across the editor. As a user, it's very confusing that separate tabs-like interfaces behave differently.
Using the same component also allows us to reduce code repetition across many places in the editor, resulting in smaller bundle sizes, less code maintenance, and fewer bugs.
I'll let folks discuss and agree on the best decision here — in any case, that specific behaviour can be controlled via the |
|
I think it's fine to use the ARIA tabs pattern, it adds consistency even if it is only a couple tabs. I've wanted this UI change for a while now and very happy with the results. I think it's also fine to change tabs based on automatic selection alone. The only area I'd ever push back on that is if focus is moved. If focus moves on tab selection, it must use manual activation. Thanks. |
|
On the whole, I think we should probably bias towards a consistent interface. While this is indeed not a scenario as is where the tab interface offers a lot of benefit, if we are using tabs in other, similar interfaces, then it's beneficial for those interfaces to behave the same way. That creates a lower cognitive burden for users, since they can operate patterns the same way when they occur. Though the exception may offer a minor benefit in this particular situation, I'm not sure that specific use-case benefit should outweigh consistency in UX. Regarding automatic vs. manual activation: I prefer manual activation, but would be OK with automatic activation. Automatic activation is just a bit less predictable. |
chad1008
force-pushed
the
tabs-site-editor-settings
branch
from
7aed117 to
07f355d
Compare
|
@afercia, thanks for the feedback on the Accessibility labels, that's something I can pay more attention to in the future. @alexstine and @joedolson thank you both also for jumping in with your thoughts. It sounds like the most undecided point is auto vs manual selection. I took another look at the guidelines:
Because latency when displaying the tab contents isn't really a concern here, and we aren't facing unexpected focus changes like the ones Alex mentioned, I'm inclined to lean towards auto activation. Does anyone have a strong objection to that approach? |
My only concern is that the Tags, Categories, Featured image panels render with some delay on first render but that would happen also with manual activation. Generally, I'd prefer manual activation. Regardless, I'd think the most important thing is to establish a pattern to be used across the editor. Having some tabs that work with manual activation and other tabs that work with auto activation isn't probably the best user experience. I'd prefer they all either use manual or auto activation, everywhere in the UI.
There is a tracking issue related to the @ciampo I'm not sure that's relevant. That issue is related to refactoring existing tabs interfaces with the new implementation. Instead, this PR introduces a new tabs interface where it was explicitly decided to not use it, This PR changes an important accessibility feature and changes decisione that were made previously. I'm perfectly fine with changes for a better user experience but such changes must be discussed broadly and following this project guidelines is the best way to do it. It's also a requirement for all contributors. Thanks. |
|
Quick question: for consistency,
|
Good point @afercia! These button based tab-like patterns are easier to miss than the existing |
Sorry @ciampo, somehow I missed this comment until just now. That's an interesting idea. My biggest hesitation with it is that we'd need to implement matching behavior for similar sidebar patterns. Specifically those that were previously using the same list-of- If I can put something that works well more universally into |
chad1008
force-pushed
the
tabs-site-editor-settings
branch
2 times, most recently
from
42d6218 to
6aa6f3f
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core SVNIf you're a Core Committer, use this list when committing to GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Now that #57696 is merged, this PR should be ready for another round of review. The updates to I've updated the PR description and testing instructions with the relevant details! |
There was a problem hiding this comment.
For some reason I'm not able to select tabs with arrow keys — the selection always stays on the same tab.
The cause seems to be the changes introduced by #57696 — for some reason, the focusedElement's ID is not yet updated when the activeId from ariakit updates.
I managed to make this work by wrapping the contents of that hook inside a requestAnimationFrame, which seems to give the browser enough time to apply the change of focus
Click to show diff
diff --git a/packages/components/src/tabs/index.tsx b/packages/components/src/tabs/index.tsx
index e8e9bff76b..4573f7a696 100644
--- a/packages/components/src/tabs/index.tsx
+++ b/packages/components/src/tabs/index.tsx
@@ -164,23 +164,25 @@ function Tabs( {
return;
}
- const focusedElement =
- items?.[ 0 ]?.element?.ownerDocument.activeElement;
-
- if (
- ! focusedElement ||
- ! items.some( ( item ) => focusedElement === item.element )
- ) {
- return; // Return early if no tabs are focused.
- }
+ requestAnimationFrame( () => {
+ const focusedElement =
+ items?.[ 0 ]?.element?.ownerDocument.activeElement;
+
+ if (
+ ! focusedElement ||
+ ! items.some( ( item ) => focusedElement === item.element )
+ ) {
+ return; // Return early if no tabs are focused.
+ }
- // If, after ariakit re-computes the active tab, that tab doesn't match
- // the currently focused tab, then we force an update to ariakit to avoid
- // any mismatches, especially when navigating to previous/next tab with
- // arrow keys.
- if ( activeId !== focusedElement.id ) {
- setActiveId( focusedElement.id );
- }
+ // If, after ariakit re-computes the active tab, that tab doesn't match
+ // the currently focused tab, then we force an update to ariakit to avoid
+ // any mismatches, especially when navigating to previous/next tab with
+ // arrow keys.
+ if ( activeId !== focusedElement.id ) {
+ setActiveId( focusedElement.id );
+ }
+ } );
}, [ activeId, isControlled, items, setActiveId ] );
const contextValue = useMemo(
Unless you can think of any other reasons why this implementation in the site editor does not like every other similar implementation (post editor, widgets), we should probably add that change in a separate PR and come back here after
chad1008
force-pushed
the
tabs-site-editor-settings
branch
from
6aa6f3f to
5247bf0
Compare
|
Feedback addressed! Thanks as always @ciampo |
chad1008
force-pushed
the
tabs-site-editor-settings
branch
from
5247bf0 to
65a8221
Compare
What?
Replace existing list-based tabs the new
Tabscomponent in the site editor settings sidebarWhy?
The current implementation was necessary because our legacy
TabPanelcomponent was not granular enough to be applied in this sidebar, due in large part to the internals of the theComplementaryAreathe sidebar uses.How?
Using the new, more granular sub-components of
Tabs, we can render the appropriate parts of the UI viaComplementaryArea'sslot/fill, and pass theTabscontext to those components as needed.This implementation is very similar to the one used previously in #55360. The biggest difference is that in this implementation, in addition to making sure the editor sidebar is active, we also need to confirm that that the canvas itself is in edit mode. When the editor canvas switches from
edittoview, all of the tabs are removed from the DOM (much like when the sidebar itself gets closed). When this happens, the component can't find the currently selected tab (which is tracked ininterfaceStore) which causes an infinite loop as the component and the store argue with each other. Checking the canvas state before setting the current tab makes sure that loop doesn't happen.One important note on this specific
Tabsimplementation is that in the editor sidebars a race condition exists where if no tab is selected, and then the Tab key is used to cycle through all of the existing blocks, the next press of Tab will move focus to the settings sidebar.When that happens, two events fire independently of each other in this order:
interfaceStoredetects that focus is no longer on a block. The sidebar is automatically switched over the the Post tab.The net result from a user perspective is that pressing Tab that last time focuses one tab wile selecting the other. This feels really confusing, so we're adding a check to prevent that from happening.
Testing Instructions
Testing Instructions for Keyboard
tablistand focuses the next element instead (the close button)