Skip to content

Tab: fix a tabindex issue#38154

Closed
louismaximepiton wants to merge 5 commits intomainfrom
main-lmp-tab-fix
Closed

Tab: fix a tabindex issue#38154
louismaximepiton wants to merge 5 commits intomainfrom
main-lmp-tab-fix

Conversation

@louismaximepiton
Copy link
Copy Markdown
Member

@louismaximepiton louismaximepiton commented Mar 2, 2023

Description

Do the tabindex trick before checking whether the element has the role="tab" or not.

Motivation & Context

I couldn't spot any breaking change.

I tried to change the role of the tab component to make it a menubar but I went into the following issue following those steps:

  • Click on the active tab
  • Tab and Shift + Tab works well 🟢
  • , Tab and Shift + Tab works but not as expected 🟡
  • , , Tab and Shift + Tab doesn't work at all 🔴

Is there a need for role="tablist/tab" to make it work/accessible ? If yes, why the behavior still works if I stay on the tabs element ?

Here is the Codepen I used to make some tests:

https://codepen.io/louismaximepiton/pen/oNqOEWG

Project Name

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

Might be linked to #33079.

@GeoSot GeoSot requested a review from patrickhlauke March 3, 2023 15:21
Comment thread js/src/tab.js

toggle(SELECTOR_DROPDOWN_TOGGLE, CLASS_NAME_ACTIVE)
toggle(SELECTOR_DROPDOWN_MENU, CLASS_NAME_SHOW)
outerElem.setAttribute('aria-expanded', open)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this, will break dropdown functionality (either way it has to be removed in next major version)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oups, haven't seen that change, reverting it for sure !

@patrickhlauke
Copy link
Copy Markdown
Member

can you explain what you're actually trying to achieve here, @louismaximepiton ? what's the current problem, and how does your PR resolve that problem?

@louismaximepiton
Copy link
Copy Markdown
Member Author

Hi Patrick,

I'm trying to achieve a side navigation (heavily draft) using the tab js. It works well until I change the role from tablist to menubar and tab to menuitem. Then if I focus the tab element (left part of the example) and I play with arrows as mentionned in the description, I can't reaccess the tab element when I drop the focus on it.

@patrickhlauke
Copy link
Copy Markdown
Member

patrickhlauke commented Mar 6, 2023

I'm trying to achieve a side navigation (heavily draft) using the tab js

I'm not 100% convinced that particular UI widget is best suited to be represented as a tabbed interface, rather than a set of accordions/menus/megamenus ?

do you have a working "after" example that shows the behaviour or your specific case/example with your changes applied?

unless i'm mistaken, you're basically trying to hijack the tabs behaviour for non-tabs, and bumping against the guardrails that try to make sure this code only runs on a tabbed interface? looking over the code change, you'll achieve the effect visually, but the currently selected/expanded/live "menuitem"/"tab" also won't programmatically expose that it's expanded/open, which is doesn't sound good from an accessibility point of view...

@louismaximepiton
Copy link
Copy Markdown
Member Author

I'm not 100% convinced that particular UI widget is best suited to be represented as a tabbed interface, rather than a set of accordions/menus/megamenus ?

I was trying to achieve it only using the Bootstrap bundle but yeah it's far from ideal right now. Haven't thought about using the megamenus since those weren't in Bootstrap bundle but yeah I could try to implement it like that 🤔

do you have a working "after" example that shows the behaviour or your specific case/example with your changes applied?

Not yet but I could maybe give it a try.

unless i'm mistaken, you're basically trying to hijack the tabs behaviour for non-tabs, and bumping against the guardrails that try to make sure this code only runs on a tabbed interface? looking over the code change, you'll achieve the effect visually, but the currently selected/expanded/live "menuitem"/"tab" also won't programmatically expose that it's expanded/open, which is doesn't sound good from an accessibility point of view...

Yes you're right, I'm trying to hack a bit the component here and it doesn't update well the aria attributes.

The main issue for me here that it seems we allow people to use several ways to make a working tab:

const SELECTOR_TAB_PANEL = '.list-group, .nav, [role="tablist"]'
const SELECTOR_INNER = `.nav-link${NOT_SELECTOR_DROPDOWN_TOGGLE}, .list-group-item${NOT_SELECTOR_DROPDOWN_TOGGLE}, [role="tab"]${NOT_SELECTOR_DROPDOWN_TOGGLE}`

But only one way keep the behavior so I was a bit surprised.

I can see two ways to resolve this issue:

  • Changing/Removing the role should at least not impact the behavior : the way I do in this PR (or maybe a better version of what's done atm), but we leave folks the possibility to have a not well accessible tab element. IMO, could be done until v6.
  • Changing/Removing the role should impact the behavior : Change the selectors might be a better way to achieve this but it might be breaking for folks and maybe we should wait for v6 ?

@louismaximepiton
Copy link
Copy Markdown
Member Author

Closing this since it doesn't seem to land in anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants