Visualize Accessibility Issues #13428
Conversation
thomasneirynck
added
Feature:Visualizations
| @@ -85,11 +85,13 @@ class MetricPanelConfig extends Component { | |||
| <div | |||
| className={`kbnTabs__tab${selectedTab === 'data' && '-active' || ''}`} | |||
| onClick={() => this.switchTab('data')} | |||
| tabIndex="0" | |||
There was a problem hiding this comment.
The value is all lowercase tabindex.
There was a problem hiding this comment.
Forget this, I haven't noticed, this was a React component.
| Legend Position | ||
| </label> | ||
| <select | ||
| aria-labelledby="visualizeBasicSettingsLegendPosition" |
There was a problem hiding this comment.
In this case, I would prefer giving the select an id and the label a for attribute with that value.
That way all users would benefit, since the label is also outside the accessibility (i.e. in the regular DOM) linked to the select box, and you can e.g. click on the label to jump to the input (not working for select, but that's another topic).
| class="form-control" | ||
| ng-model="vis.params.legendPosition" | ||
| ng-options="position.value as position.text for position in vis.type.editorConfig.collections.legendPositions" | ||
| > | ||
| </select> | ||
| </div> | ||
| <div class="vis-option-item"> | ||
| <label> | ||
| <input type="checkbox" ng-model="vis.params.addTooltip"> | ||
| <label id="visualizeBasicSettingsShowTooltip"> |
There was a problem hiding this comment.
You shouldn't need this at all. Since the input is already inside the label they are correctly linked, and you wouldn't need to link them specially for screen readers.
| <label>Per Page</label> | ||
| <input type="number" ng-model="vis.params.perPage" class="form-control"> | ||
| <label id="datatableVisualizationPerPage">Per Page</label> | ||
| <input type="number" ng-model="vis.params.perPage" class="form-control" for="datatableVisualizationPerPage"> |
There was a problem hiding this comment.
id and for are interchanged. The label is for a specific input (specified by its id).
| <select ng-disabled="!vis.params.showTotal" | ||
| class="form-control" | ||
| ng-model="vis.params.totalFunc" | ||
| ng-options="x for x in totalAggregations"> | ||
| ng-options="x for x in totalAggregations" for="datatableVisualizationTotalFunction"> |
| @@ -1,7 +1,9 @@ | |||
| <!-- vis type specific options --> | |||
| <div class="form-group"> | |||
| <label>Map type</label> | |||
| <select name="agg" | |||
| <label id="coordinateMapOptionsMapType">Map type</label> | |||
There was a problem hiding this comment.
I would use <label for> here too instead of aria-labelledby
There was a problem hiding this comment.
Is there a reason, you left this with aria-labelledby instead of label for?
There was a problem hiding this comment.
my mistake, an oversight on my part, changed this now.
| @@ -5,6 +5,7 @@ | |||
| ng-class="{ 'timelion-interval-custom-compact': interval === 'other' }" | |||
| ng-model="otherInterval" | |||
| ><select | |||
| aria-labelledby="timelionInterval" | |||
There was a problem hiding this comment.
You could use for on the label instead. Which is imho also a little nicer in case of what the directive relies on. The aria-labelledby way the directive relies on being used in a context where a label with that given id exists. If you use for the directive just renders something with an id and the place where it is used utilize that id for its label.
Also I think this directive is used inside of timelion where there isn't a label currently. Since we still might want to use that directive in both places and make it accessible in both places, in this case we might be better adding an aria-label to it, that holds the label directly, without requiring the user of the directive to label it correctly.
|
thanks @timroes, pushed changes, 2nd look? |
Backport. Required a couple of manual edits
Follow up on #13226.
Closes #12896
Closes #12897
Closes #12898
Closes #12899