Visualization accessibility issues #13226
Conversation
thomasneirynck
added
Feature:Visualizations
thomasneirynck
force-pushed
the
fix/access_add_search_role
branch
from
e60677f
to
7096dee
Compare
| </th> | ||
| </tr> | ||
|
|
||
| <tr ng-repeat="range in vis.params.colorsRange track by $index"> | ||
| <td> | ||
| <input | ||
| aria-labelledby="heatmap-custom-range-from" | ||
| ng-model="range.from" |
There was a problem hiding this comment.
please include a for attribute as well, i.e. for="heatmap-custom-range-from"
There was a problem hiding this comment.
aphelionz we seem to have different opinions here :D I just added to my review to remove it. According to html spec there is no for attribute on the input element. I only know it on the label element to reference the actual input element (but it doesn't accept multiple ids as a value. We could still consider adding it for the first range inputs, so the browser would jump to them clicking on "From" and "to"
| @@ -128,6 +129,7 @@ | |||
| </td> | |||
| <td> | |||
| <input | |||
| aria-labelledby="heatmap-custom-range-to" | |||
| ng-model="range.to" | |||
| type="number" | |||
There was a problem hiding this comment.
please include a for attribute as well, i.e. for="heatmap-custom-range-to"
|
@aphelionz could you take 2nd look? thx! |
| @@ -109,16 +109,18 @@ | |||
| <table class="vis-editor-agg-editor-ranges form-group" ng-show="vis.params.colorsRange.length"> | |||
| <tr> | |||
| <th> | |||
| <label>From</label> | |||
| <label id="heatmap-custom-range-from">From</label> | |||
There was a problem hiding this comment.
Our current html styleguide says, we should use camel case for ids - felt unintuitive for me, too :D
| </th> | ||
| <th colspan="2"> | ||
| <label>To</label> | ||
| <label id="heatmap-custom-range-to">To</label> |
| </th> | ||
| </tr> | ||
|
|
||
| <tr ng-repeat="range in vis.params.colorsRange track by $index"> | ||
| <td> | ||
| <input | ||
| aria-labelledby="heatmap-custom-range-from" | ||
| for="heatmap-custom-range-from" |
There was a problem hiding this comment.
The for attribute doesn't exist on input elements. It is for label elements to refer to the input they are labeling (which doesn't work in this case, since it can only reference one element). So we can just remove the for here.
| @@ -128,6 +130,8 @@ | |||
| </td> | |||
| <td> | |||
| <input | |||
| aria-labelledby="heatmap-custom-range-to" | |||
| for="heatmap-custom-range-to" | |||
| @@ -27,6 +27,7 @@ <h1 class="kuiTitle kuiVerticalRhythm kuiVerticalRhythm--medium"> | |||
| type="text" | |||
| placeholder="Search visualization types..." | |||
| ng-model="searchTerm" | |||
| role="search" | |||
There was a problem hiding this comment.
role=search is unfortunately a little bit tricky. If you put it on the input you will overwrite it's native role as a textfield, which will cause weird behavior in assistive technologies (like screen readers). Also you shouldn't put it to the form, since it would cause the same. Ususally it makes sense to put it on a div inside the form, that contains the input. In this case, this can be easily achieved, since we already have an encapsulated div (the one also commented with Search. Please put the role there.
There is an ongoing discussion in the ARIA standard, in several different issues... also see Where to put your search role.
| @@ -2,6 +2,7 @@ | |||
| <label>Select {{ groupName }} type</label> | |||
| <ul class="form-group list-group list-group-menu"> | |||
| <li | |||
| tabindex="0" | |||
There was a problem hiding this comment.
tabindex unfortunately also needs the id attribute on this element to work properly. We documented this in the styleguide
| @@ -11,6 +11,7 @@ | |||
| aria-label="{{:: 'Index pattern: ' + vis.indexPattern.title}}" | |||
| ng-if="vis.type.requiresSearch" | |||
| class="index-pattern" | |||
| tabindex="0" | |||
thomasneirynck
force-pushed
the
fix/access_add_search_role
branch
from
7c07708
to
e8f4347
Compare
|
rebased with master, and added @timroes feedback |
Closes