Components: Tabs: Improve Controlled Mode Focus Handling #57696
Conversation
|
Before reviewing in details the proposed solution, I actually wonder if adding this quite complex logic to the component is a good idea. When reviewing A different approach would be to assume that the consumer of the component is in charge of implementing any specific custom behavior themselves, corroborated by Andrew's advice about
I'm not sure yet exactly where we should draw the line, but it's definitely something that we should at least consider before moving forward. cc @mirka |
|
On the one hand I like the idea of handling this within the component, so the different implementations (I can think of three at the moment) similar to this one don't need to be maintained separately by the editor... but I do see your point about making the component more and more complex. Thinking about it, I suppose another package could write a reusable helper to handle this logic internally if need be. If we do go that route, it might make sense to remove the previously added focus logic as well. That way the component doesn't manipulate focus at all, and it's entirely in the hands of the consumer. |
Yeah, while we wait to hear Lena's opinion, maybe you could look into exploring this option? |
Good call. From what I understood, the proposed changes seem generically useful enough to live inside this component. And the added code complexity doesn't immediately look disproportionate to the value that it adds. Someone could persuade me otherwise on either of these points, but that's my current impression. Also I'm not sure we can shift this responsibility down to consumers without them coupling their code with Ariakit internals ( So I'm ok with this logic staying in the component, at least with the information we have now. (Not saying we'll never discover complications in the future though, however low-risk it seems now 😅) |
|
That's fair enough. I honestly wanted to take a step back and hear opinions from folks who have been less directly involved in the @chad1008 , would you be able to add some unit tests (that would fail on |
chad1008
force-pushed
the
tabs-improve-controlled-focus-sync
branch
3 times, most recently
from
2832b29
to
8c086f2
Compare
ciampo
added
[Type] Bug
chad1008
force-pushed
the
tabs-improve-controlled-focus-sync
branch
from
3e0f078
to
1c3a994
Compare
chad1008
force-pushed
the
tabs-improve-controlled-focus-sync
branch
from
260eae5
to
908f0f7
Compare
|
feeling ready to merge this one, but in the interest of thoroughness on the a11y front, ccing @andrewhayward or @alexstine in case y'all have any thoughts 😄 |
Apologies, I missed this PR.
I'm happy to defer to @alexstine on this one, who has a lot more real world experience with these things, but as a general rule, we should avoid moving focus unless the user takes an action that might require it. So while I can see where you're coming from with this, it doesn't quite feel right. Personally I'd recommend the following:
Does that make sense? |
@andrewhayward It does! thank you for organizing into those bullets, that was very helpful. Reading through that, I start to wonder if the best path forward for this PR might actually be:
so we don't mess with focus, but we minimize any additional confusion. |
|
@alexstine / @andrewhayward - not an urgent ping, but wondering if either of you had any final insights on approach here. The more I consider Andrew's feedback above, the more I think moving away from changing focus at all, and just preserving expected arrow key navigation makes the most sense, but I'd definitely prefer to defer to the experts on this one 😄 |
This could be a good compromise, although I'm curious to hear other folks' opinions too. |
chad1008
force-pushed
the
tabs-improve-controlled-focus-sync
branch
from
5df31aa
to
32d0066
Compare
|
Thanks @ciampo. In the interest of expediency, i've updated the PR to no longer manipulate focus and instead only manage the arrow key navigation, and requested some fresh reviews. This change will cause some unit tests to fail, I'll get them updated once we've finalized our path forward. |
chad1008
force-pushed
the
tabs-improve-controlled-focus-sync
branch
from
32d0066
to
a46c253
Compare
|
After some additional brainstorming with @ciampo and @mirka we're going to move forward with this PR's approach. Ultimately, this makes the component less invasive/opinionated by reversing the previous decisions to manipulate focus. It also addresses the bug with arrow key navigation in controlled mode. These both feel like wins! For future context, if the decision is ever made to restore the focus management, one of the commits on this PR will serve as a good record of what was briefly in place. I've updated the impacted unit and e2e tests. On the unit tests, I've done away with the previously added @ciampo / @mirka, would you mind taking a one last look at the test changes before I push this through? Thanks!! |
|
Size Change: +318 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Thanks for your continued work on this, @chad1008! Maybe I'm misunderstanding the intent of the final implementation, but what I'm seeing seems to go against the testing instructions above. current.tab.behaviour.movWhen focus is not on the active tab, focus is not moved when the active tab changes. But when focus is on the active tab when it changes, the focus moves with it. This is counter to points 4 and 10 in your Storybook instructions. |
…state to track the active ID
…ack into the tablist
|
@andrewhayward , from your screencast it seems like you're testing a stale version of this PR? @chad1008 and I paired a bit on this PR, and we came up with an updated version of the hook which seems a lot more solid, since it reads the I pushed a few commits:
|
|
I'm going to call myself out of this final round of review, as I'm the co-author of these changes. @andrewhayward / @mirka / @chad1008 , if would be great if we could have one final round of review and smoke testing, so that we can unblock the remaining tabs PRs |
…e active element
Huh... weird, I'm sure I was up to date. Anyway, verified that I have the latest changes, and it is now working as intended/expected. |
|
Thanks so much everyone! |
This PR is meant to initially serve as a proof of concept and place for discussion. I've included a possible path forward but I'm primarily interested in feedback on what the best path forward is. I put the changes together because I wanted to test their viability for own purposes, and figured I may as well include them in case a working example was helpful.
What?
Limitcontrolled-modeTabscomponent focus updates related to selection changes to cases when the tab selected at the time of the change had focus.Remove the automatic focus syncing behavior while adding a new check to ensure arrow key navigation is preserved.
Why?
Currently, the
Tabscomponent is set to make sure that in controlled mode, if a tab has browser focus and the selected tab changes, the newly selected tab gets focus. This was first discussed when introducing the newTabscomponent into the editor settings, and introduced in #56658. In that initial PR, we intentionally skip the focus updates whenselectOnMoveisfalse, to avoid moving focus on a user who is actively navigating across the available tabs.In a separate but similar PR, we came to the conclusion that manual selection was a better option for the tabs in question. This means the update mentioned above will no longer apply, (
selectOnMovewill befalse) and the selected tab can easily become out of sync with the currently focused tab.I had two thoughts on how to address this:
selectOnMovebetruefor keeping focus and selection in syncselectOnMovecondition, but limit the actual focus update to cases where the tab that was selected at that time of the change actually had focus.This PR experiments with the second option as a bit of a compromise. @andrewhayward's advice in the discussion linked above was to err on the side of not manipulating focus more than necessary. With the approach on this PR, if you've placed focus on the currently selected tab and the selection changes, focus will update you'll ultimately end up with focus still in alignment with the displayed content. On the other hand, if you've explicitly moved focus away from the selected tab the focus will not be changed. This way if you're actively navigating across tabs when a change happens, you're much less likely to find your focus bouncing around unexpectedly.Adjusting based on feedback (see comments below), this PR removes the previously introduced focus management. This means that if the currently selected tab is focused and that selection is changed by the controlling component, focus will not be moved to the newly selected tab.
Note: This also means that in the editor settings sidebars (both post and site editors) tabbing through the available blocks into the sidebar can cause the Block tab to be focused while the Document tab is actually selected. If necessary, we can look into adding a helper to the editor itself to avoid this situation. That way we address that use case without making the component itself overly opinionated.
How?
When deciding whether or not to update focus, we check the id of the previously selected tab. If that's the same tab that has focus (we haven't shifted focus yet, so it's still going to be wherever the user last placed it) we know the selected tab was focused before the change was made. In that case we shift the focus to the new tab.Regardless of any other factors, we no longer change browser focus.
We do, however, have to manage the component's internal focus. Ariakit tracks and manages focus internally via an
activeIdvalue. When the selected tab is changed, thatactiveIdalso changes to match the selection.If DOM focus is on a different tab when this happens, the arrow keys will base their actions off of the new
activeId, not the current DOM focus. This can lead to awkward navigation. To avoid that, when the selected tab is changed and focus was not already on the focused tab, we update theactiveIdto once again match the currently select tab in the DOM. This ensures that arrow key behavior remains consistent.Testing Instructions
In the editor:
Apply the following testing diff
This adds
focusOnMove={false}to this implementation, which I'll most likely be doing for real in a separate PR, but it's the most convenient place to test these changes.In Storybook
Apply the following test diff
This adds
selectOnMove={false}and a delay to the Controlled Mode story, so you can move focus to test the the effect of the tab changing on you.Tabscomponent and the dropdown menu. Then press Tab to focus the currently selected tab (Tab 3)