[Management] Improve accessibility within management #13364

chrisronline · 2017-08-07T14:10:17Z

Closes #12861
Closes #12862
Closes #12863
Closes #12865
Closes #12868
Closes #12870
Closes #11526

This PR addresses some low hanging fruit to improve accessibility within the management area.

timroes

Added some comments.

I moved the info icon in #13341. I moved it to the status page as suggested in the ticket, so I think we should also leave that PR open. But I guess we can merge yours and mine in the end without any problem.

timroes · 2017-08-07T14:28:16Z

src/core_plugins/kibana/public/management/app.html

-        <span ng-if="!sectionName" class="kuiLocalTab">
-          {{::section.display}}
-        </span>
+        <header ng-if="!sectionName" id="tabHeader" tabindex="0">


As discussed (unfortunately after you already did this) in #12861 I don't see a reason, why we would add an id and tabindex here. That element is not interactive at all, why would we need to focus it?
I think it might have been confusing, since that title also has a cursor: pointer and hover color when using the mouse (but still no interactive content).

I would really appreciate @aphelionz feedback on this, but I think we should remove id and tabindex here and most likely also remove the pointer cursor and hover color?

I agree about the cursor: pointer and hover color here if it's not interactive. However I can go either way on it being tabbable based on the idea that I expressed in the other issue - orienting the user on the page and audibly saying the name of the section.

I would then leave it like that. So just ignore that comment :-)

timroes · 2017-08-07T14:34:31Z

...ore_plugins/kibana/public/management/sections/indices/edit_index_pattern/field_controls.html

@@ -3,6 +3,7 @@
    data-test-subj="indexPatternFieldEditButton"
    ng-href="{{ kbnUrl.getRouteHref(field, 'edit') }}"
    aria-label="Edit"
+    tabindex="0"


a element with an href (or in this case ng-href) are automatically in taborder. This attribute isn't needed.

Okay good to know!

timroes · 2017-08-07T14:34:50Z

...ore_plugins/kibana/public/management/sections/indices/edit_index_pattern/field_controls.html

@@ -12,6 +13,7 @@
    ng-if="field.scripted"
    ng-click="remove(field)"
    class="kuiButton kuiButton--danger kuiButton--small"
+    tabindex="0"


buttons are automatically in taboder and this attribute can safely be removed.

timroes · 2017-08-07T14:43:00Z

src/core_plugins/kibana/public/management/sections/objects/_objects.html

@@ -73,6 +73,7 @@ <h1 class="kuiTitle">
              class="kuiToolBarSearchBox__input"
              type="text"
              placeholder="Search..."
+              role="search"


role=search should be on a parent diff, see my comments here

timroes · 2017-08-07T15:18:33Z

src/ui/public/paginated_table/paginated_table.html

@@ -15,6 +15,7 @@
            <kbn-info ng-if="col.info" info="{{ col.info }}" placement="top"></kbn-info>
            <i
              ng-if="col.sortable !== false"
+              tabindex="0"


tabindex needs to be on the element, that also allows the click (ng-click). Also this only makes it focusable, but not "keyboard accessible", that the user can activate the action by keyboard. You would need to add the kbnAccessibleClick directive too.

chrisronline · 2017-08-07T17:50:37Z

Resolved those issues @timroes. Appreciate the deep dive!

cjcenizal · 2017-08-07T19:19:45Z

src/core_plugins/kibana/public/management/app.html

-        <header ng-if="!sectionName" id="tabHeader" tabindex="0">
-          <h2 class="kuiLocalTab">
+        <header ng-if="!sectionName" id="tabHeader">
+          <h2>


We should keep the class kuiLocalTab here. I think @timroes's comment was about removing the id and tabindex attributes. But I think we may actually want to revert this commit until @aphelionz has had a chance to explain how these attributes affect accessibility, since he's the one who established this pattern originally.

Also, as a general rule of thumb, we should avoid removing classes that affect the UI unless we're confident that this is a good change. In this case, this is a bad change because it introduces inconsistency between the headers of the different apps. Feel free to ping members of the design team with questions about this kind of stuff in the future.

Thanks for the heads up @cjcenizal! I'll revert this back, or possibly introduce a new class that has the same styles sans the hover treatment.

Revert done. Will wait until we hear from @aphelionz!

This tab should have the .kuiLocalTab-isSelected class applied to it, which should do the trick.

That doesn't look right?

Ah, I know what's wrong. We need to add margin: 0 to the kuiLocalTab class in the UI Framework. The <h2> elements introduces a margin, and this component's design doesn't anticipate this kind of use case.

cjcenizal · 2017-08-07T19:23:35Z

Thanks for pinging me on this, @chrisronline! I'll wait until #13341 is merged and rebased onto this branch before I review, since that will simplify the diff.

cjcenizal · 2017-08-07T19:25:43Z

@timroes It looks like @aphelionz explained his reasoning in #12861 (comment):

I've found that some users find having a tabbable heading helpful just for orientation, though I can be persuaded either way on this. As long as we're consistent it should be fine.

Seems to make sense to me. Thoughts?

aphelionz

I think we might want to revisit the kuiLocalTab directive based on @timroes' feedback. There might be too much built in to one class declaration for a11y purposes

aphelionz · 2017-08-07T20:55:36Z

src/core_plugins/kibana/public/management/app.html

-        <span ng-if="!sectionName" class="kuiLocalTab">
-          {{::section.display}}
-        </span>
+        <header ng-if="!sectionName" id="tabHeader" tabindex="0">


I agree about the cursor: pointer and hover color here if it's not interactive. However I can go either way on it being tabbable based on the idea that I expressed in the other issue - orienting the user on the page and audibly saying the name of the section.

timroes · 2017-08-08T10:41:50Z

FYI: #13341 is merged. Could you maybe rebase to master again?

timroes · 2017-08-08T10:44:07Z

src/ui/public/field_editor/field_editor.html

    <input
      ng-model="editor.field.name"
+      id="scriptedFieldName00------------"


Btw, I forgot to add that comment yesterday: What has happened to this id? 🐱 ⌨️

That'd be my cat :) Thanks for pointing it out!

timroes · 2017-08-08T10:48:15Z

If you could still rebase it, fix the broken ID, I would be fine to give this a LGTM. I think we should move the discussion about kuiLocalTab into it's own issue, to not glut this PR.

…stly, we can probably make due without any class (using default h2 styles)

…s - honestly, we can probably make due without any class (using default h2 styles)" This reverts commit c54ea227889898baef85ee71b3350bbf69ef5d7c.

chrisronline · 2017-08-08T13:11:47Z

@timroes Sounds good. Should I remove all changes in regards to header, or leave what I have?

timroes · 2017-08-08T13:21:01Z

I would leave it like it is currently.

chrisronline · 2017-08-08T13:29:39Z

@timroes Great! PR has been rebased and ready for the last pass!

timroes · 2017-08-08T13:37:21Z

LGTM. I think we should think about the focus color/shadow in tables header, they look weird (in Chrome), but wouldn't consider this to be part of this PR.

cjcenizal

🎉 Fantastic! I had a couple minor comments.

To fully address #12863, we also need to make sure we address the different parts of the form that become available when you choose different "Format" values:

We also need to address the "Create Index Pattern" form for this issue, but we can do that as part of #13154. I left a comment on that PR.

cjcenizal · 2017-08-08T14:49:35Z

src/core_plugins/kibana/public/management/app.html

+          <h2 class="kuiLocalTab">
+            {{::section.display}}
+          </h2>
+        </header>


Minor nit, but I think the original issue had a slight error in that it referred to a "header", but really meant to refer to a "heading". So I think this markup should become this:

<h2 class="kuiLocalTab" ng-if="!sectionName" id="tabHeader" tabindex="0"> {{::section.display}} </h2>

cjcenizal · 2017-08-08T14:58:24Z

src/ui/public/paginated_table/paginated_table.html

@@ -10,6 +10,8 @@
          <th
            ng-repeat="col in columns"
            ng-click="paginatedTable.sortColumn($index)"
+            kbn-accessible-click
+            tabindex="0"


Awesome! @snide do you think you could fix the focus styles for these elements?

chrisronline · 2017-08-09T18:16:26Z

@cjcenizal Great call out! Fixed and ready for another pass!

aphelionz · 2017-08-09T18:18:35Z

src/ui/public/field_format_editor/pattern/pattern.html

@@ -1,5 +1,6 @@
 <div class="form-group">
  <input
+    id="{{ id }}"


Is there any requirement for a label here?

It corresponds to labels used outside of the directive: https://github.com/elastic/kibana/pull/13364/files#diff-cdc9adaaaf6e3096b1edb04fc611d045

Gotcha 👍

cjcenizal

Nice! Had one minor concern.

cjcenizal · 2017-08-09T20:13:00Z

src/ui/public/field_format_editor/pattern/pattern.js

+      // The id attribute is not always an angular expression but nearly always
+      // just a string passed in
+      $scope.id = attrs.id;
+      attrs.$observe('id', () => $scope.id = attrs.id);


I think this directive needs replace: true, or else the directive shows up as a DOM element with an id on it, which I think will take precedence over the input's id.

Also, I think you can define this a little more simply via the scope property:

scope: { id: '@', },

Did you already give that a shot?

You might also want to double check my assertion about the id in the browser.

You're absolutely right. Good call out

chrisronline · 2017-08-10T14:43:01Z

Fixed up @cjcenizal. Ready for another pass!

cjcenizal

LGTM!

* Use header/h tag for tab header, #12861 * Add tabindexes for tables, #12862 * Add for/id pairings, #12863 * Search role, #12868 * Use better descriptions for aria labels, #12865 * Add aria label, #12870 * Button and a tags are already in tab order, removing unnecessary change * The input field cannot contain the role="search" * The tabindex needs to be on the element which the click handler * Remove tabindex and remove the class applying the hover styles - honestly, we can probably make due without any class (using default h2 styles) * Revert "Remove tabindex and remove the class applying the hover styles - honestly, we can probably make due without any class (using default h2 styles)" This reverts commit c54ea227889898baef85ee71b3350bbf69ef5d7c. * Remove the extra code my cat added * Addressing hidden inputs from #12863 * Remove unnecessary header tag * Add clarifying comment * Prevent multiple ids

chrisronline · 2017-08-10T17:20:49Z

Backport

6.0: 4df4671
6.x: 511ffa2

chrisronline added Project:Accessibility :Management review v6.0.0 v6.1.0 v7.0.0 labels Aug 7, 2017

chrisronline self-assigned this Aug 7, 2017

chrisronline requested review from timroes and cjcenizal August 7, 2017 14:10

timroes suggested changes Aug 7, 2017

View reviewed changes

cjcenizal reviewed Aug 7, 2017

View reviewed changes

aphelionz reviewed Aug 7, 2017

View reviewed changes

timroes reviewed Aug 8, 2017

View reviewed changes

chrisronline added 12 commits August 8, 2017 09:10

Use header/h tag for tab header, elastic#12861

7430a3d

Add tabindexes for tables, elastic#12862

e7f6e36

Add for/id pairings, elastic#12863

14b8e23

Search role, elastic#12868

43a664b

Use better descriptions for aria labels, elastic#12865

e99955d

Add aria label, elastic#12870

21ea3ac

Button and a tags are already in tab order, removing unnecessary change

1fe2f2d

The input field cannot contain the role="search"

ad0575f

The tabindex needs to be on the element which the click handler

0bf778a

Remove tabindex and remove the class applying the hover styles - hone…

04f9f7f

…stly, we can probably make due without any class (using default h2 styles)

Revert "Remove tabindex and remove the class applying the hover style…

dbc6e38

…s - honestly, we can probably make due without any class (using default h2 styles)" This reverts commit c54ea227889898baef85ee71b3350bbf69ef5d7c.

Remove the extra code my cat added

124c3af

chrisronline force-pushed the management/accessibility branch from 428017d to 124c3af Compare August 8, 2017 13:10

timroes approved these changes Aug 8, 2017

View reviewed changes

cjcenizal suggested changes Aug 8, 2017

View reviewed changes

chrisronline mentioned this pull request Aug 9, 2017

[Accessibility] Management short-term issues #12207

Closed

9 tasks

chrisronline added 2 commits August 9, 2017 14:10

Addressing hidden inputs from elastic#12863

d031b93

Remove unnecessary header tag

fa8d64c

aphelionz reviewed Aug 9, 2017

View reviewed changes

Add clarifying comment

ca0cccd

cjcenizal suggested changes Aug 9, 2017

View reviewed changes

Prevent multiple ids

f6f82ab

cjcenizal approved these changes Aug 10, 2017

View reviewed changes

chrisronline merged commit d12aa4b into elastic:master Aug 10, 2017

chrisronline deleted the management/accessibility branch August 10, 2017 17:20

spalger mentioned this pull request Aug 17, 2017

Field format editor samples broken for date format #13542

Closed

jimgoodwin added the v6.0.0-rc1 label Sep 28, 2017

cjcenizal mentioned this pull request Oct 12, 2017

[UI Framework] Fix margin for KuiLocalTabs which are applied to typographic elements. #14451

Merged

[Management] Improve accessibility within management #13364

[Management] Improve accessibility within management #13364

Conversation

chrisronline commented Aug 7, 2017

timroes 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

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

chrisronline commented Aug 7, 2017 • edited Loading

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

cjcenizal Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Aug 7, 2017

cjcenizal commented Aug 7, 2017

aphelionz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes commented Aug 8, 2017

chrisronline commented Aug 8, 2017

timroes commented Aug 8, 2017

chrisronline commented Aug 8, 2017

timroes commented Aug 8, 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

chrisronline commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this 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

Choose a reason for hiding this comment

chrisronline commented Aug 10, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

chrisronline commented Aug 10, 2017

chrisronline commented Aug 7, 2017 •

edited

Loading

cjcenizal Aug 7, 2017 •

edited

Loading