-
Notifications
You must be signed in to change notification settings - Fork 35
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
Conversation
@@ -33,7 +33,8 @@ | |||
{% set _css_class = 'ecl-tabs' %} | |||
{% set _extra_attributes = [ |
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.
break; | ||
|
||
default: | ||
break; |
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.
never seen a break on a default case for a switch, what would that be doing..? :)
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.
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 don't absolutely agree with the user who wrote that comment, default is always executed at the end of the switch, breaking after it is completely irrelevant, i'd say logically wrong, for how i see it. But again, fine if you like it this way.
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.
removed ;)
} | ||
|
||
if (flag) { | ||
e.stopPropagation(); |
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 about what we would prevent here, but ok, just that for how the logic is it seems you could set the variable to true and then in the default case set it to false, not a big difference but less repetitions..
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.
Sure you are right, I have removed this prevents condition 👍
@@ -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 comment
The 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.
Also because the standard navigation would be working decently in most of the viewports and only when some tabs get hidden it would be really problematic.
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.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
We can discuss this on the next weekly if you want ;)
@@ -59,27 +60,38 @@ | |||
{# Print the result #} | |||
|
|||
{% if _items is not empty and _items is iterable %} | |||
<nav class="{{ _css_class }}"{{ _extra_attributes|raw }}> | |||
<div class="{{ _css_class }}"{{ _extra_attributes|raw }}> |
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 the reason why of this change, but i guess there is.. :)
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.
Was following markup from w3: https://www.w3.org/WAI/ARIA/apg/example-index/tabs/tabs-manual.html but I guess we can keep <nav>
} else { | ||
this.moveFocusToTab(this.tabsKey[index + 1]); | ||
} | ||
} |
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 don't see the new listeners being destroyed together with the others in the destroy method..
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.
done
flag = true; | ||
break; | ||
|
||
case 'ArrowRight': |
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 thnk this navigation has a minor flaw, when you reach the "collapsed" tabs you would maybe expect to be able to browse the items also using arrowDown and arrowUp, no..? At least this is what i instinctively tried to do when i first tested this.
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 was first trying doing this, but it's more simple now the dropdown opens automatically if you try to go to the next tabs (collapse). Otherwise, you need to tab open the dropdown navigate with up and down arrows and tab again to come back to previous tabs. this was agreed with COMM.
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 was only sugggesting to "add" the possibility of browsing the dropdown also with up and down arrows, nothing wrong with the fact that it opens and close using left and right arrows. Actually what i would find correct would be to focus on the "more" element and open the dropdown for then using arrow up and down inside of it.
/** | ||
* @param {HTMLElement} currentTab tab element | ||
*/ | ||
moveFocusToPreviousTab(currentTab) { |
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.
Do we really need separated callbacks foran operation like moving focus between the tags..? It's not only because of the almost duplicated code, but it's about the logic, i feel that we could handle this in a single function, but it's just a suggestion and a preference..
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.
Merged the 2 functions
this.dropdown.classList.remove('ecl-tabs__dropdown--show'); | ||
} | ||
currentTab.focus(); | ||
return this; |
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.
Why returning the instance here..?
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.
removed 👍
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.
A few comments, minor fixes and a few questions, but the navigation with arrows works fine, just that i don't understand why preventing users from using Tabs as well, it creates the impression of a "bug" and you need to discover yourself the alternative way of navigating between the different tabs.
No description provided.