-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(tabs): Improve accessibility - FRONT-3661 #2576
Changes from 6 commits
f525523
356129e
831108c
174ba59
84d0e09
5f85f19
ef1e8e8
1a7d935
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,8 @@ | |
{% set _css_class = 'ecl-tabs' %} | ||
{% set _extra_attributes = [ | ||
'data-ecl-tabs="true"', | ||
'data-ecl-auto-init="Tabs"']|join(' ') | ||
'data-ecl-auto-init="Tabs"', | ||
'role="tablist"']|join(' ') | ||
%} | ||
|
||
{# Internal logic - Process properties #} | ||
|
@@ -62,24 +63,35 @@ | |
<nav class="{{ _css_class }}"{{ _extra_attributes|raw }}> | ||
<div class="ecl-tabs__container"> | ||
<ul class="ecl-tabs__list"> | ||
{% for _item in _items %} | ||
{% if _item.path is not empty %} | ||
<li class="ecl-tabs__item"> | ||
{% set _item_extra_classes = 'ecl-tabs__link' %} | ||
{% if _item.is_current is defined and _item.is_current is not empty %} | ||
{% set _item_extra_classes = _item_extra_classes ~ ' ecl-tabs__link--active' %} | ||
{% for _item in _items %} | ||
{% if _item.path is not empty %} | ||
<li class="ecl-tabs__item"> | ||
{% set _item_extra_classes = 'ecl-tabs__link' %} | ||
{% set _item_extra_attributes = [{ name: 'role', value: 'tab' }] %} | ||
{% if loop.index != 1 %} | ||
{% set _item_extra_attributes = _item_extra_attributes|merge([{ name: 'tabindex', value: '-1' }]) %} | ||
{% endif %} | ||
{% if _item.is_current is defined and _item.is_current is not empty %} | ||
{% set _item_extra_classes = _item_extra_classes ~ ' ecl-tabs__link--active' %} | ||
{% set _item_extra_attributes = _item_extra_attributes|merge([{ name: 'aria-selected', value: 'true' }]) %} | ||
{% else %} | ||
{% set _item_extra_attributes = _item_extra_attributes|merge([{ name: 'aria-selected', value: 'false' }]) %} | ||
{% endif %} | ||
{% if _menu is not empty %} | ||
{% set extra_attributes = extra_attributes|merge([{ name: 'data-ecl-has-menu' }]) %} | ||
{% endif %} | ||
{% include '@ecl/link/link.html.twig' with { | ||
link: { | ||
label: _item.label, | ||
path: _item.path, | ||
}, | ||
extra_classes: _item_extra_classes, | ||
extra_attributes: _item_extra_attributes, | ||
} only %} | ||
</li> | ||
{% endif %} | ||
{% include '@ecl/link/link.html.twig' with { | ||
link: { | ||
label: _item.label, | ||
path: _item.path, | ||
}, | ||
extra_classes: _item_extra_classes, | ||
} only %} | ||
</li> | ||
{% endif %} | ||
{% endfor %} | ||
<li class="ecl-tabs__item ecl-tabs__item--more"> | ||
{% endfor %} | ||
<li class="ecl-tabs__item ecl-tabs__item--more"> | ||
{% include '@ecl/button/button.html.twig' with { | ||
label: _more_label, | ||
variant: 'secondary', | ||
|
@@ -92,6 +104,7 @@ | |
transform: 'rotate-180' | ||
}, | ||
extra_classes: 'ecl-tabs__toggle', | ||
extra_attributes: [{ name: 'tabindex', value: '-1' }], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i understand that we are introducing navigation through keyboard arrows, but why disabling the standard navigation via Tab..? It feels really weird to access the first tab with a Tab and then being moved out of the tabs typing again Tab. This way the user really have to know that the only way of navigating our tabs would be using arrows. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is basically the request, to only be able to navigate with arrows and thus be able to go to the content more quickly without having to tab all tabs. https://www.w3.org/WAI/ARIA/apg/example-index/tabs/tabs-manual.html There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't understand, really.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can discuss this on the next weekly if you want ;) |
||
} only %} | ||
</li> | ||
</ul> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see that we normally define our internal variable for attributes as a string that gets populated by looping into the extra_attributes param, and it's ok since it's going to be rendered as a string. But when we have "internal" attributes to set, it would feel natural to merge these values into the extra_attributes array and get the _extra_attributes string as the result of the internal and external attributes processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, I properly understand the issue here, we are doing same in expandable and news-ticker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not an issue, just that we are requesting extra attributes as objects with name and value for then processing them to generate a string. if we are about to create internal attributes i would merge these with the extra_attributes array in order to process them in one place consistently. I know that we have other use cases like this, but that doesn't make it right. it was a suggestion, if you like it like this, keep it that way.