Tabs: consider simplifying edge case handling

[ciampo]

The private Tabs component has a bunch of custom logic that handles a few edge cases:

1. Initial tab selection:
   • if a defaultTabId is specified, Tabs will select an initial tab only if there is a tab matching defaultTabId. If there isn't a matching tab, Tabs won't have an initially selected tab. If a tab with a matching id is added lazily, Tabs will select it.
   • the initial tab is also used as a fallback when the currently selected tab can't be found anymore. If the initial tab also can't be found, the Tabs component won't have any selected tab;
   • This bunch of logic only applied in uncontrolled mode — the idea being that a consumer controlling the component is in charge of handling these edge cases;
2. The currently selected tab becomes disabled:
   • the main assumption is that a disabled tab cannot stay as the selected tab;
   • In controlled mode, Tabs assumes that the consumer of the component is in charge of the situation, and therefore it un-selects the previously active, now disabled tab;
   • In uncontrolled more, if the currently selected tab becomes disabled, Tabs falls back to the defaultTabId if possible. Otherwise, it selects the first enabled tab (if there is one).
3. In controlled mode, Tabs will reset the active tab id (ie. no tabs are active) if a tab associated to the currently selected ID can't be found
4. If there is no active tab, fallback to place focus on the first enabled tab, so there is always an active element
5. It keeps the active id in sync with the currently focused tabs

This custom logic was originally added after observing how the legacy TabPanel component worked, in an effort to cover all edge cases found when using the component in the editor.

As the ariakit library updates and we iterate on the component I am considering removing most (if not all) of this custom logic:

• All of this custom logic is hard to maintain, and it can sometimes cause new unwanted edge cases or unexpected regressions (like during the latest ariakit update);
• The difference between controlled vs uncontrolled behaviour is one more layer of complexity;
  -ariakit's latest changes should make sure that a composite widget (like tabs) is always somewhat tabbable, even if the active/selected ID doesn't match an existing DOM element;
• In general, I think we should delegate the handling of such special cases to the consumer of the component, and keep Tabs focused on providing good tab widgets fundamentals;

My instinct is to remove as much of this code as possible (ideally all of it):

• review whether the current way of handling these edge cases makes sense at all, or is instead a bad practice;
• move edge case handling directly where Tabs is consumed, if necessary;

@WordPress/gutenberg-components , I'd like to hear your thoughts on this proposal

[ciampo]

Some more thoughts:

• behavior #1 feels arbitrary. I guess that this could be delegated to the consumer of the component, if they even find themselves in this scenario;
• I would challenge the idea behind behaviour #2 : why can't a selected tab that becomes disabled stay selected? If that's problematic, I guess it's another situation in which the consumer of the component could handle the edge case;
• behaviour #3 and #4 are potentially addressed in ariakit with this update; if the selectedid gets out of sync, could we also assume that the responsibility should fall on the consumer of the component?
• behaviour #5 looks like the most reasonable, almost a bug fix. We'd need to look into it and understand when / why it was needed, and if we still need it.

[tyxla]

Thanks for the details 👍

I agree with your proposal 👍 It's worth auditing the reasons behind introducing all of these custom edge cases, and if there's no clear good reason, we should just clean them up and go with the defaults.

Any arguments to support not going that way?

[ciampo]

It's worth auditing the reasons behind introducing all of these custom edge cases

I'll do my best, but it may be tricky since these behaviors track back to the legacy TabPanel component and may be the result of several iterations over the years.

I'll do my best, but I'm also tempted to remove all the hooks and see what e2e tests break + smoke test the tabs in the editor.

[DaniGuardiola]

@ciampo fully support this and your proposed approach in the last comment. I think when/if the need for those behaviors pops up again after the clean-up, when should probably take the route of reporting to ariakit first as bugs or feature proposals.

[ciampo]

I opened #66097 to remove the custom logic.

• behaviour #5 looks like the most reasonable, almost a bug fix. We'd need to look into it and understand when / why it was needed, and if we still need it.

Opened ariakit/ariakit#4213 to bring this behavior to the attention of ariakit authors

[ciampo]

Update: With #66097 merged, the only bit of custom logic left is related to the conversation happening in ariakit/ariakit#4213