Fix secondary menu item accessibility #5701
Conversation
| <ul class="umb-tabs-nav"> | ||
| <li class="umb-tab" ng-repeat="tab in vm.tabs | limitTo: vm.maxTabs" data-element="tab-{{tab.alias}}" ng-class="{'umb-tab--active': tab.active, 'umb-tab--error': tabHasError}" val-tab> | ||
| <ul role="tablist" class="umb-tabs-nav"> | ||
| <li class="umb-tab" role="tab" aria-selected="true" aria-controls="panel-1" id="tab-1" tabindex="0" ng-repeat="tab in vm.tabs | limitTo: vm.maxTabs" data-element="tab-{{tab.alias}}" ng-class="{'umb-tab--active': tab.active, 'umb-tab--error': tabHasError}" val-tab> |
There was a problem hiding this comment.
@tiffy74 it seems to be invalid, where all tabs get the id tab-1. You could change it to id="tab-{{$index + 1}}" and maybe tabindex="{{$index}}".
There was a problem hiding this comment.
Thanks for the response
Actually I had intended it to be simpler - I just forgot to remove the additional elements from my own testing code. The id="tab-1" was meant to be removed.
It should be:
<li class="umb-tab" role="tab" aria-selected="true" tabindex="0" ng-repeat="tab in vm.tabs | limitTo: vm.maxTabs" data-element="tab-{{tab.alias}}" ng-class="{'umb-tab--active': tab.active, 'umb-tab--error': tabHasError}" val-tab> <a ng-href="" ng-click="vm.clickTab($event, tab)">{{ tab.label }}</a> </li>
There was a problem hiding this comment.
Have now updated the branch and removed the id and aria-controls elements - tested and works as expected.
Removed id and aria-controls - tested and secondary menu tabbing functions correctly
|
Hi @tiffy74 - before I review this, the title of this PR is a bit ambiguous as to whether the work on this PR has completed or not? I see there's a merge conflict but that's fine, something I can resolve easily when reviewing 👍 So let me know if you're still working on this PR or if it's ready for review. |
|
Hiya @nul800sebastiaan It is ready for review. Think the merge conflict is the package-lock file so as you say can be resolved easily. Ive removed the items that @bjarnef was highlighting and so should work fine now Thanks Tiff |
|
Thanks @tiffy74 ! I've reverted the package-lock and am happy with the proposed change. Thanks very much! 🎖 |
nul800sebastiaan
changed the title
Prerequisites
If there's an existing issue for this PR then this fixes Issue 5277 - no.95 and no.100
Description
Added role=tablist to ul of dashboard menu and added role=tab to li items of dashboard menu. This means they will now not be skipped by navigation