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.

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

[Accessibility] Improve visualization legends accessibility #14505

Merged
merged 12 commits into from
Oct 26, 2017
27 changes: 22 additions & 5 deletions src/ui/public/visualize/visualize_legend.html
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@
type="button"
ng-click="toggleLegend()"
class="kuiCollapseButton legend-collapse-button"
aria-label="Toggle legend"
Copy link
Contributor

Choose a reason for hiding this comment

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

VoiceOver doesn't announce this label because of this aria-hidden logic introduced in #13573. I think that the warning is still useful, but perhaps we should just remove aria-hidden="true" from the legend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I will remove it, but therefor I would like to add an aria-label introducing the legend as such. That would also partly solve the issue you mentioned below, regarding missing context for the user with just legend values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

aria-expanded="{{!!open}}"
aria-controls="{{::legendId}}"
>
<span class="kuiIcon {{getToggleLegendClasses()}}"></span>
</button>
<ul class="legend-ul" ng-show="open">
<ul class="legend-ul" ng-show="open" id="{{::legendId}}">

<li
ng-repeat="legendData in labels track by legendData.label"
Expand All @@ -18,6 +21,10 @@

<div class="legend-value-container">
<div
kbn-accessible-click
data-label="{{legendData.label}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this attribute do?

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the label doesn't indicate to the screen reader what will happen if the user clicks the button. Is there an elegant way to add an invisible label that says something like "Click for filtering options"? Maybe we can associate it with aria-describedby? Do you think this will be helpful?

Also, if the label is a value like "181", then the screen reader will just read this number aloud, which doesn't provide enough context for me to make sense of it. What if we added a noun to the end of this value? For example, if the aggregation was on "bytes", we could read aloud:

{{ `${legendData.label} + ${legendData.values.aggConfig.params.field.name}` }}

Which would cause the screen reader to announce "181 bytes". What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The attribute itself is required by the handlers in the visualization, that are doing the highlight and unhighlight to find the actual value. Not ideal imho, but that's the way it works atm.

I will add a description for what the label triggering does, that's a very good idea.

I think extending the label with the fieldname, won't always work as smooth as it did in your example :-) So I would rather add this to the description like: "Filtering bytes fields for this value"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it to the label instead, so you really can't miss it. I didn't add aria-expanded since that would require we can reference the legend options via aria-controls. But they use ng-if and not ng-show at the moment, so they are really removed from DOM and we cannot reference them when hidden. I also don't want to use ng-show in that place, since imho the options contain a significant amount of DOM nodes and having a dashboard with multiple visualizations on it, which several legend items, would render a significantly higher amount of DOM nodes with ng-show.

So I decided to not use aria-expanded in that case.

ng-focus="highlight($event)"
ng-blur="unhighlight($event)"
ng-click="showDetails = !showDetails"
Copy link
Contributor

Choose a reason for hiding this comment

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

Once the user opens these options, it's easy to get lost in the colorpicker. I know we don't want to spend time fixing this in this PR, but I think it would be helpful to let the user close the options by hitting ESC. This way if s/he gets lost in the color picker we've provided an easy way out. We just need to extract lines 22 through 75 into their own directive, and add a keypress handler there which sets showDetails to false. The challenge might be setting focus back on the button once the filtering options have been closed. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the color picker is not really accessible the way it is even after that PR. But I don't think just adding Escape would be enough. We still would need to think about color names, proper arrow navigation, etc... So I would rather replace this in the more or less near future, by using a standardized color picker from the UI framework instead and put the a11y effort into this. It would also come with the benefit, of being able to freely assign a color.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I hear you... I was just thinking of it as a temporary solution, since it's so easy to get stuck inside the color picker options and not know that the only way to get out of them is to backtrack all the way back to the button and then click it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

ng-class="showDetails ? 'legend-value-full' : 'legend-value-truncate'"
class="legend-value-title"
Expand All @@ -33,21 +40,31 @@
ng-show="canFilter(legendData)"
>
<button
class="kuiButton kuiButton--basic kuiButton--small" ng-click="filter(legendData, false)"
class="kuiButton kuiButton--basic kuiButton--small"
ng-click="filter(legendData, false)"
aria-label="Filter for value {{legendData.label}}"
>
<span class="kuiIcon fa-search-plus"></span>
</button>

<button
class="kuiButton kuiButton--basic kuiButton--small" ng-click="filter(legendData, true)"
class="kuiButton kuiButton--basic kuiButton--small"
ng-click="filter(legendData, true)"
aria-label="Filter out value {{legendData.label}}"
>
<span class="kuiIcon fa-search-minus"></span>
</button>
</div>

<div class="legend-value-color-picker">
<div class="legend-value-color-picker" role="listbox">
<span id="{{legendId}}ColorPickerDesc" class="kuiScreenReaderOnly">Set color for value {{legendData.label}}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we indent this?

<span
  id="{{legendId}}ColorPickerDesc"
  class="kuiScreenReaderOnly"
>
  Set color for value {{legendData.label}}
</span>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if I would say "no"? Yes of course we can :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Then you better have a good reason! :D

<i ng-repeat="choice in colors"
ng-click="setColor(legendData.label, choice)"
kbn-accessible-click
role="option"
aria-label="{{choice}}"
aria-describedby="{{legendId}}ColorPickerDesc"
aria-selected="{{choice === getColor(legendData.label)}}"
ng-click="setColor(legendData.label, choice, $event)"
ng-class="choice == getColor(legendData.label) ? 'fa-circle-o' : 'fa-circle'"
ng-style="{color: choice}" class="fa dot"
>
Expand Down
7 changes: 6 additions & 1 deletion src/ui/public/visualize/visualize_legend.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import html from 'ui/visualize/visualize_legend.html';
import { VislibLibDataProvider } from 'ui/vislib/lib/data';
import { FilterBarClickHandlerProvider } from 'ui/filter_bar/filter_bar_click_handler';
import { uiModules } from 'ui/modules';
import { htmlIdGenerator } from 'ui_framework/services';


uiModules.get('kibana')
Expand All @@ -16,6 +17,7 @@ uiModules.get('kibana')
link: function ($scope) {
const $state = getAppState();
const clickHandler = filterBarClickHandler($state);
$scope.legendId = htmlIdGenerator()('legend');
$scope.open = $scope.uiState.get('vis.legendOpen', true);

$scope.$watch('visData', function (data) {
Expand Down Expand Up @@ -48,13 +50,16 @@ uiModules.get('kibana')
handler.unHighlight.call(el, handler.el);
};

$scope.setColor = function (label, color) {
$scope.setColor = function (label, color, event) {
const colors = $scope.uiState.get('vis.colors') || {};
if (colors[label] === color) delete colors[label];
else colors[label] = color;
$scope.uiState.setSilent('vis.colors', null);
$scope.uiState.set('vis.colors', colors);
$scope.uiState.emit('colorChanged');
if (event) {
event.currentTarget.blur();
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why are we doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I was tinged with the slight idea, of not having the color button keeping the focus ring and it's then selected state. But honestly, we don't do this anywhere else, and again I want to replace this by the unified color picker in the future, so yeah I will throw it out for this PR.

}
refresh();
};

Expand Down