Accessible sort buttons #12217

aphelionz · 2017-06-07T02:43:37Z

Will Resolve #11853

Here's my rationale. I know that the directive code is likely inadequate, so I'm happy to get feedback there. Otherwise this should work as expected.

Changing the i to a button to make it clickable without a javascript handler
Adding the usual id and tabindex
Making the screen reader say "Sort time ascending" when clicking the button will cause it to sort it ascending

To test, tab to the time field in the discover interface and hit the space bar to sort.

aphelionz · 2017-06-07T02:44:00Z

@cjcenizal Also please feel free to add any other appropriate reviewers to this.

cjcenizal

Nice! I think there are a couple things we need to account for before we can merge, but we're close!

cjcenizal · 2017-06-07T19:22:02Z

src/ui/public/doc_table/components/table_header.html

+      ng-click="cycleSortOrder(indexPattern.timeFieldName)"
+      tooltip="Sort by time"
+    ></button>
+    </span>


Nice solution! A few lines below, I think we also need to update this markup similarly:

<span class="table-header-name"> {{name | shortDots}} <i ng-class="headerClass(name)" ng-click="cycleSortOrder(name)" tooltip="{{tooltip(name)}}" tooltip-append-to-body="1"></i> </span>

I think this is what drives the appearance of the sort button for a non-time columns (e.g. geo.srcdest). To show this column in the table header, just expand open a row and click this icon:

This was addressed.

cjcenizal · 2017-06-07T19:29:11Z

src/ui/public/doc_table/components/table_header.js

+
+      $scope.sortText = $scope.sortOrder[1] === 'asc'
+        ? 'Button: Sort time descending'
+        : 'Button: Sort time ascending';


Great idea! But I think we'll need to extract this out into its own scope method, and tweak it to use a dynamic column name. Something like this (haven't tested that this works):

<button aria-label="{{ getAriaLabelForColumn('time') }}">

$scope.getAriaLabelForColumn = function getAriaLabelForColumn(name) { const [currentColumnName, currentDirection = 'asc'] = $scope.sortOrder; if(name === currentColumnName && currentDirection === 'asc') { return `Button: Sort ${name} ascending`; } return `Button: Sort ${name} descending`; }

cjcenizal · 2017-06-07T19:31:26Z

src/ui/public/styles/base.less

@@ -213,6 +213,9 @@ kbn-table, .kbn-table {

    .fa {
      font-size: 1.1em;
+      border: none;
+      background: none;
+      padding: 0;


Let's create a new class called .docTableHeaderButton in doc_table/doc_table.less and move these styles there, just so they're a little more organized together and semantically specific.

aphelionz · 2017-06-08T18:22:30Z

Jenkins, please test

cjcenizal · 2017-06-08T19:36:57Z

@aphelionz you just need to have some variation of the words "jenkins test this", I think

cjcenizal · 2017-06-08T19:37:16Z

Though I am impressed by how polite to jenkins you always are!

cjcenizal

⚡️ One super minor request then we're good to go.

cjcenizal · 2017-06-08T20:21:24Z

src/ui/public/doc_table/components/table_header.js


        if (!sortOrder || column !== sortOrder[0]) return defaultClass;
-        return ['fa', sortOrder[1] === 'asc' ? 'fa-sort-up' : 'fa-sort-down'];
+        return ['fa', 'docTableHeaderButton', sortOrder[1] === 'asc' ? 'fa-sort-up' : 'fa-sort-down'];


Because docTableHeaderButton is going to be applied in every case, we can actually remove it from this logic (and go back to what was here originally), and use it directly in the template instead by assigning it with the class attribute:

<span>Time <button id="docTableHeaderFieldSort{{indexPattern.timeFieldName}}" tabindex="0" aria-label="{{ getAriaLabelForColumn(indexPattern.timeFieldName) }}" class="docTableHeaderButton" ng-class="headerClass(indexPattern.timeFieldName)" role="button" ng-click="cycleSortOrder(indexPattern.timeFieldName)" tooltip="Sort by time" ></button> </span> <button id="docTableHeaderFieldSort{{name}}" tabindex="0" aria-label="{{ getAriaLabelForColumn(name) }}" class="docTableHeaderButton" ng-class="headerClass(name)" ng-click="cycleSortOrder(name)" tooltip="{{tooltip(name)}}" tooltip-append-to-body="1" ></button>

Bargs · 2017-06-12T20:23:41Z

Hmmm the columns that aren't currently being sorted on seem to only be tabbable if you are hovering over them with your mouse. I thought maybe display:none was being used but that doesn't seem to be the case. Not sure what's going on exactly.

aphelionz · 2017-06-12T21:19:18Z

@Bargs @cjcenizal Yeah, we might need to loop in design to see what they want to do here. I'll play with it a bit more. I think I can get them to be visually hidden but still tabbable.

cjcenizal · 2017-06-12T23:05:19Z

@Bargs Good catch!

@aphelionz We'll just need to replace lines 203 to 229 of src/ui/public/styles/base.less with these styles:

kbn-table, .kbn-table {
  font-size: @font-size-small;

  th {
    white-space: nowrap;
    padding-right: 10px;

    .table-header-move, .table-header-sortchange {
      opacity: 0;

      &:focus {
        opacity: 1;
      }
    }

    .fa {
      font-size: 1.1em;
    }

  }

  th:hover {
    .table-header-move, .table-header-sortchange {
      opacity: 1;
    }
  }
}

Switching from visibility to opacity makes them tabbable and then we can reveal them when they have focus.

I think we also just need to change the <i> elements to <button> elements in src/ui/public/doc_table/components/table_header.html to make them tabbable, and then move some classes around to reveal them on focus too. That file would become:

<tr>
  <th width="1%"></th>
  <th ng-if="indexPattern.timeFieldName" data-test-subj="docTableHeaderField">
    <span>Time <button
      id="docTableHeaderFieldSort{{indexPattern.timeFieldName}}"
      tabindex="0"
      aria-label="{{ getAriaLabelForColumn(indexPattern.timeFieldName) }}"
      class="docTableHeaderButton"
      ng-class="headerClass(indexPattern.timeFieldName)"
      role="button"
      ng-click="cycleSortOrder(indexPattern.timeFieldName)"
      tooltip="Sort by time"
    ></button>
    </span>
  </th>
  <th ng-repeat="name in columns" data-test-subj="docTableHeaderField">
    <span class="table-header-name">
      {{name | shortDots}}
      <button
        id="docTableHeaderFieldSort{{name}}"
        tabindex="0"
        aria-label="{{ getAriaLabelForColumn(name) }}"
        class="docTableHeaderButton"
        ng-class="headerClass(name)"
        ng-click="cycleSortOrder(name)"
        tooltip="{{tooltip(name)}}"
        tooltip-append-to-body="1"
      ></button>
    </span>
    <button
      class="fa fa-remove table-header-move"
      ng-click="onRemoveColumn(name)"
      ng-if="canRemoveColumn(name)"
      tooltip-append-to-body="1"
      tooltip="Remove column"
    ></button>
    <button
      class="fa fa-angle-double-left table-header-move"
      ng-click="moveColumnLeft(name)"
      ng-if="canMoveColumnLeft(name)"
      tooltip-append-to-body="1"
      tooltip="Move column to the left"
    ></button>
    <button
      class="fa fa-angle-double-right table-header-move"
      ng-click="moveColumnRight(name)"
      ng-if="canMoveColumnRight(name)"
      tooltip-append-to-body="1"
      tooltip="Move column to the right"
    ></button>
  </th>
</tr>

You'll just need to add a few styles to .table-header-move class to eliminate the default button appearance and maybe remove the padding.

This will expand the scope of this PR slightly, but code above is already written, and we can close out #12230! 🎯

aphelionz · 2017-06-20T03:15:15Z

@cjcenizal @Bargs This has been updated based on the feedback. Let me know!

Bargs

LGTM!

cjcenizal

A couple last things and then let's merge this sucker!

cjcenizal · 2017-06-20T16:15:53Z

src/ui/public/doc_table/components/table_header.html

    </span>
+    <button


Can we add an aria-label for this button? This should work:

aria-label="Remove {{name}} column"

cjcenizal · 2017-06-20T16:15:59Z

src/ui/public/doc_table/components/table_header.html

+      tooltip-append-to-body="1"
+      tooltip="Remove column"
+    ></button>
+    <button


And this one:

aria-label="Move {{name}} column to the left"

cjcenizal · 2017-06-20T16:16:06Z

src/ui/public/doc_table/components/table_header.html

+      tooltip-append-to-body="1"
+      tooltip="Move column to the left"
+    ></button>
+    <button


This guy too:

aria-label="Move {{name}} column to the right"

aphelionz · 2017-06-20T16:47:24Z

@cjcenizal done!

cjcenizal

Caught a little typo. :)

cjcenizal · 2017-06-20T17:20:46Z

src/ui/public/doc_table/components/table_header.html

+      ng-if="canMoveColumnRight(name)"
+      tooltip-append-to-body="1"
+      tooltip="Move column to the right"
+      aria-label="Move {{name}} column to the left"


This needs to be "Move {{name}} column to the right".

aphelionz · 2017-06-20T17:59:17Z

@cjcenizal fixed :)

cjcenizal

LGTM!

cjcenizal · 2017-06-20T19:48:26Z

Thanks @aphelionz.

First pass at creating accessible sort buttons

d05e8ee

aphelionz requested a review from cjcenizal June 7, 2017 02:43

aphelionz changed the title ~~First pass at creating accessible sort buttons~~ Accessible sort buttons Jun 7, 2017

aphelionz added Project:Accessibility release_note:enhancement labels Jun 7, 2017

cjcenizal requested a review from Bargs June 7, 2017 18:57

cjcenizal added Feature:Discover Discover Application :Discovery v6.0.0 v6.0.0-beta1 labels Jun 7, 2017

cjcenizal suggested changes Jun 7, 2017

View reviewed changes

aphelionz added 2 commits June 8, 2017 00:09

A better scope function

9c6acbf

Applying to other elements, fixing CSS

e78300b

cjcenizal suggested changes Jun 8, 2017

View reviewed changes

Better class implementation

27e6afc

Updating buttons to use opacity

8327d76

Bargs approved these changes Jun 20, 2017

View reviewed changes

cjcenizal suggested changes Jun 20, 2017

View reviewed changes

Adding aria-labels

fe46d06

cjcenizal suggested changes Jun 20, 2017

View reviewed changes

Fixing typo

84bb7e4

cjcenizal approved these changes Jun 20, 2017

View reviewed changes

cjcenizal merged commit 22a70b7 into elastic:master Jun 20, 2017

cjcenizal mentioned this pull request Jun 20, 2017

Accessibility: Discover table column buttons aren't keyboard accessible #12230

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accessible sort buttons #12217

Accessible sort buttons #12217

aphelionz commented Jun 7, 2017

aphelionz commented Jun 7, 2017

cjcenizal left a comment

cjcenizal Jun 7, 2017

aphelionz Jun 8, 2017

cjcenizal Jun 7, 2017

cjcenizal Jun 7, 2017

aphelionz commented Jun 8, 2017

cjcenizal commented Jun 8, 2017

cjcenizal commented Jun 8, 2017

cjcenizal left a comment •

edited

Loading

cjcenizal Jun 8, 2017

Bargs commented Jun 12, 2017

aphelionz commented Jun 12, 2017

cjcenizal commented Jun 12, 2017

aphelionz commented Jun 20, 2017

Bargs left a comment

cjcenizal left a comment

cjcenizal Jun 20, 2017

cjcenizal Jun 20, 2017

cjcenizal Jun 20, 2017 •

edited

Loading

aphelionz commented Jun 20, 2017

cjcenizal left a comment

cjcenizal Jun 20, 2017

aphelionz commented Jun 20, 2017

cjcenizal left a comment

cjcenizal commented Jun 20, 2017

Accessible sort buttons #12217

Accessible sort buttons #12217

Conversation

aphelionz commented Jun 7, 2017

aphelionz commented Jun 7, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aphelionz commented Jun 8, 2017

cjcenizal commented Jun 8, 2017

cjcenizal commented Jun 8, 2017

cjcenizal left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bargs commented Jun 12, 2017

aphelionz commented Jun 12, 2017

cjcenizal commented Jun 12, 2017

aphelionz commented Jun 20, 2017

Bargs left a comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

aphelionz commented Jun 20, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aphelionz commented Jun 20, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal commented Jun 20, 2017

cjcenizal left a comment •

edited

Loading

cjcenizal Jun 20, 2017 •

edited

Loading