Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Visualize Accessibility Issues #13428

Merged
merged 6 commits into from
Aug 10, 2017
Merged

Visualize Accessibility Issues #13428

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0b1d3a8
add for/id pairing
thomasneirynck Aug 9, 2017
70ca2fc
link from/to labels
thomasneirynck Aug 9, 2017
68c40b0
add more pairings
thomasneirynck Aug 9, 2017
3965ac7
ensure tsvb tabs are tabbable
thomasneirynck Aug 9, 2017
09671a1
feedback fixes
thomasneirynck Aug 9, 2017
9a48a44
review feedback
thomasneirynck Aug 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/core_plugins/kbn_vislib_vis_types/public/controls/gauge_options.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,16 +97,17 @@
<table class="vis-editor-agg-editor-ranges form-group" ng-show="vis.params.gauge.colorsRange.length">
<tr>
<th>
<label>From</label>
<label id="gaugeOptionsCustomRangeFrom">From</label>
</th>
<th colspan="2">
<label>To</label>
<label id="gaugeOptionsCustomRangeTo">To</label>
</th>
</tr>

<tr ng-repeat="range in vis.params.gauge.colorsRange track by $index">
<td>
<input
aria-labelledby="gaugeOptionsCustomRangeFrom"
ng-model="range.from"
type="number"
class="form-control"
Expand All @@ -117,6 +118,7 @@
</td>
<td>
<input
aria-labelledby="gaugeOptionsCustomRangeTo"
ng-model="range.to"
type="number"
class="form-control"
Expand Down
7 changes: 4 additions & 3 deletions src/core_plugins/kbn_vislib_vis_types/public/controls/vislib_basic_options.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
<div>
<div class="vis-option-item">
<label>
<label id="visualizeBasicSettingsLegendPosition">
Legend Position
</label>
<select
aria-labelledby="visualizeBasicSettingsLegendPosition"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<input type="checkbox" ng-model="vis.params.addTooltip" aria-labelledby="visualizeBasicSettingsShowTooltip">
Show Tooltip
</label>
</div>
Expand Down
2 changes: 2 additions & 0 deletions src/core_plugins/metrics/public/components/panel_config/metric.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,13 @@ class MetricPanelConfig extends Component {
<div
className={`kbnTabs__tab${selectedTab === 'data' && '-active' || ''}`}
onClick={() => this.switchTab('data')}
tabIndex="0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value is all lowercase tabindex.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forget this, I haven't noticed, this was a React component.

>Data
</div>
<div
className={`kbnTabs__tab${selectedTab === 'options' && '-active' || ''}`}
onClick={() => this.switchTab('options')}
tabIndex="0"
>Panel Options
</div>
</div>
Expand Down
3 changes: 3 additions & 0 deletions src/core_plugins/metrics/public/components/panel_config/timeseries.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,16 +146,19 @@ class TimeseriesPanelConfig extends Component {
<div
className={`kbnTabs__tab${selectedTab === 'data' && '-active' || ''}`}
onClick={() => this.switchTab('data')}
tabIndex="0"
>Data
</div>
<div
className={`kbnTabs__tab${selectedTab === 'options' && '-active' || ''}`}
onClick={() => this.switchTab('options')}
tabIndex="0"
>Panel Options
</div>
<div
className={`kbnTabs__tab${selectedTab === 'annotations' && '-active' || ''}`}
onClick={() => this.switchTab('annotations')}
tabIndex="0"
>Annotations
</div>
</div>
Expand Down
2 changes: 1 addition & 1 deletion src/core_plugins/metrics/public/components/vis_picker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ function VisPickerItem(props) {
<div className={iconClassName}>
<i className={`fa ${icon}`} />
</div>
<div className={labelClassName}>
<div className={labelClassName} tabIndex="0">
{ label }
</div>
</div>
Expand Down
8 changes: 4 additions & 4 deletions src/core_plugins/table_vis/public/table_vis_params.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<div class="form-group">
<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">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id and for are interchanged. The label is for a specific input (specified by its id).

</div>

<div class="checkbox">
Expand Down Expand Up @@ -32,10 +32,10 @@
</div>

<div>
<label>Total function</label>
<label id="datatableVisualizationTotalFunction">Total function</label>
<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">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id <-> for

</select>
</div>
6 changes: 4 additions & 2 deletions src/core_plugins/tile_map/public/editors/tile_map.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use <label for> here too instead of aria-labelledby

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason, you left this with aria-labelledby instead of label for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my mistake, an oversight on my part, changed this now.

<select
aria-labelledby="coordinateMapOptionsMapType"
name="agg"
class="form-control"
ng-model="vis.params.mapType"
ng-init="vis.params.mapType || vis.type.editorConfig.collections.mapTypes[0]"
Expand Down
1 change: 1 addition & 0 deletions src/core_plugins/timelion/public/directives/timelion_interval/timelion_interval.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
ng-class="{ 'timelion-interval-custom-compact': interval === 'other' }"
ng-model="otherInterval"
><select
aria-labelledby="timelionInterval"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

class="kuiSelect timelion-interval-presets"
ng-options="intervalOption for intervalOption in intervalOptions"
ng-class="{ 'timelion-interval-presets-compact': interval === 'other'}"
Expand Down
2 changes: 1 addition & 1 deletion src/core_plugins/timelion/public/vis/timelion_vis_params.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<div>
<div class="form-group">
<label>Interval</label>
<label id="timelionInterval">Interval</label>
<div class="form-group">
<timelion-interval model="vis.params.interval"></timelion-interval>
</div>
Expand Down