vitamin-web icon indicating copy to clipboard operation
vitamin-web copied to clipboard

refactor(@vtmn/svelte, @vtmn/vue, @vtmn/react): rework tabs a11y

Open thibault-mahe opened this issue 2 years ago • 2 comments

Changes description

Following rework on demo on #1233 (refactor(@vtmn/css): tabs demo showcase + rework a11y), some updates and fixes should be made on Svelte, React and Vue tabs integration.

  • Components should observe a similar behavior as the demo, in particular regarding script for accessibility conformity.
  • Add missing a11y attributes: aria, tabindex, ...

Context

  • The tabs component wasn't accessible, now you can move across the different tabs with the left and right arrow keys. The tabs focus on the next element after the nav.

Checklist

  • [x] Make sure you are requesting to pull a topic/feature/bugfix branch. Please, don't request directly from your main!
  • [x] Check commits & PR names matches our requested structure. It must follow the https://www.conventionalcommits.org pattern.
  • [x] Check your code additions will fail neither code linting checks.
  • [x] I have reviewed the submitted code.
  • [x] I have tested on related showcases.
  • [x] If it includes design changes, please ask for a review with a core team designer.

Does this introduce a breaking change?

Yes :

  • replace a with button, remove the href attribute.

thibault-mahe avatar Sep 13 '22 12:09 thibault-mahe

Hi @GaspardMathon @lauthieb @7studio @romuleald @Tlahey ,

I keep you updated on this PR. Fell free to answer, review or even ignore this message ;)

I pushed a new commit to challenge the current conception of our tabs. I started with the React integration.

As a reminder, here is what were missing :

  • each tab item should be linked to a corresponding tab panel ;
  • users should be able to tab from a tab item to the corresponding tabpanel, and use arrow keys to navigate through tab items ;
  • missing a11y attributes should be fixed.

Unfortunately, all the fixes has resulted in a totally and completely breaking change :/ :

  • user has now to provide tab panels ;
  • accordingly, the tabs structure has changed since the items and the panels have to share some states variable now ;
  • to keep the same strategy, major logic is hidden inside component, but user still have to provide some configuration. In addition to the already existing VtmnTabsItem, user has now to use 3 new components inside Tabs : VtmnTabsItems, VtmnTabsPanels, VtmnTabsPanel (see below)

Regarding the integration of the component for React, here is some elements:

  • The main idea was to keep the initial strategy, which was like the compound component pattern: leave to the user the rendering of the tabs navigation and tab panels, the easiest way possible, and hide some internal logic inside the component. I'm not sure if it is the best pattern for this case (we could just have a tab component with a content prop, waiting for an array or an object...), but once again the main idea was to keep current implementation logic.
  • Hence, I kept this idea by defining 2 « areas », meaning a component VtmnTabsItems and a component VtmnTabsPanels, instead of a single children for items. Both of these « areas » have their own children than the user can fill out, usually with the VtmnTabsItem and a new VtmnTabsPanel See:
<VtmnTabs>
    <VtmnTabsItems>
        <VtmnTabsItem>Tab Item 1</VtmnTabsItem>
        <VtmnTabsItem>Tab Item 2</VtmnTabsItem>
        <VtmnTabsItem>Tab Item 3</VtmnTabsItem>
    </VtmnTabsItems>
    <VtmnTabsPanels>
        <VtmnTabsPanel>Tab Panel 1</VtmnTabsPanel>
        <VtmnTabsPanel>Tab Panel 2</VtmnTabsPanel>
        <VtmnTabsPanel>Tab Panel 3</VtmnTabsPanel>
    </VtmnTabsPanels>
</VtmnTabs>
  • This way all the logic is kept inside component, out of reach for the user. Tabs component shares global state (currentTabIndex, …) to both Items area (VtmnTabsItems) and panels area (VtmnTabsPanels). Each of these zone manages their own local state: for instance VtmnTabsItems manages the arrow navigation and gives its items some props such as 'aria-controls' according to their index ; VtmnTabsPanels gives its panels some props such as 'aria-labelledby' according to their index. Finally VtmnTabsItem and VtmnTabsPanel are mainly UI
  • Regarding this implementation,

Now, 2 question remain:

  • What do you guys think about keeping the current compound component pattern? I know that it is not the best implementation for React right now (for instance it could be a good idea to use the context API for sharing some state elements here). How do you feel for the Svelte and Vue implementation?
  • Because of all the breaking changes, I think that we should wait for the complete rework of the whole component on Vitamin V1. This would allow us to provide all user with better accessibility and the best experience. What do you think?

Thanks :)

thibault-mahe avatar Sep 14 '22 13:09 thibault-mahe

Thanks for the news @thibault-mahe. I agree with all the things you've said. I think that we should let this PR in draft and we will rework the component for the next major version.

We had a similar strategy with the vtmn-text-input : a lot of improvements could be made but were breaking change so we raised an issue not to forget to update this component for the next version.

GaspardMathon avatar Sep 15 '22 08:09 GaspardMathon

Sorry to disturb this PR with an old subject but you work too fast.

After our first conversation about the markup of this component (<nav> was deleted) I had some doubts about the <ul role="tablist"><li><button role="tab"> too.

According to this thread on Twiiter: https://twitter.com/7studio/status/1565425932923437056 with role="tablist", the <ul> element isn't a list anymore and the <li> blocks a correct restitution with its role listitem.

The right markup would be: <ul role="tablist"><li role="none"><button role="tabt"> or simplier <div role="tablist"><button role="tab">

I hope it helps to improve your component.

7studio avatar Sep 28 '22 19:09 7studio

Hi @7studio, thanks for your suggestion. Indeed I didn't know that the role tablist removes the list quality of the ul. The thread you shared is really informative on the subject. I agree with the simpler <div role="tablist"><button role="tab">, we'll make a fix accordingly. Thanks!

thibault-mahe avatar Sep 29 '22 09:09 thibault-mahe

Thanks @7studio, I've made an update in consequence ;)

GaspardMathon avatar Sep 29 '22 15:09 GaspardMathon