fix(ngAria): clean up tabindex usage #12262

marcysutton · 2015-07-02T17:17:00Z

Do not put tabindex on natively interactive controls using ng-model or ng-click
Use a single nodeBlacklist to limit which elements receive support (not user-configurable)

marcysutton · 2015-07-02T17:18:43Z

src/ngAria/aria.js

@@ -126,6 +128,11 @@ function $AriaProvider() {
    };
  }

+  function isNodeOneOf(elem, nodeTypeArray) {


This function used to live inside of the ngClick override directive, below. This exposes it to the entire module.

gkalpak · 2015-07-02T19:38:25Z

Use a single nodeBlacklist to limit which elements receive support (not user-configurable)

I don't know why/if it should not be user-configurable, but putting it onto the service makes it user-configurable.

Not sure if it's for better or worse, but you could use the nodeBlacklist and the isNodeOneOf() directly (not through the service). Just mentioning it...

marcysutton · 2015-07-02T19:54:16Z

Good point...I swapped that around at the last minute and didn't think it through. Two things:

I actually think it makes sense to expose the element config, but I've been warned about this before by @caitp. Once it's in there, it's really hard to remove. see bug(ngAria): native controls fire a single click event #10766 (comment).
This is a dumb question (which may be moot after no. 1), but how can I expose those functions inside of ngAria without exposing them in the service or polluting the global scope?

patrickfox · 2015-07-03T04:42:01Z

tabindex="0" is used to make something user focusable/tabbable, and only elements that are intended to be actionable should be able to receive this. Does it make sense to allow a random DIV to have tabindex="0" without it having a specified interactive role? Would it make sense to allow tabindex="0" on non-blacklisted elements only when they have a role="{interactive role}"or at least throw out a console.warn?

Interactive roles are:

radio
checkbox
button
link
tab
menuitem
textbox
menuitemcheckbox
menuitemradio
spinbox
textbox
option

gkalpak · 2015-07-03T09:33:01Z

I actually think it makes sense to expose the element config, but I've been warned about this before by @caitp. Once it's in there, it's really hard to remove. see #10766 (comment).

It is indeed difficult/undesirable to refactor something, once it's been made public. I would refrain from it for now and re-consider if/when we have several users asking for it. Especially, since ngAria seems to be about to get some refactoring and internal things might be more volatile right now.
In any case, if we decide to make it public, we should do it as a configuration option on the provider (not on the service) to avoid inconsistencies.

[...] how can I expose those functions inside of ngAria without exposing them in the service or polluting the global scope?

All modules are wrapped in an IIFE, so you will not be polluting the global scope.

marcysutton · 2015-07-05T22:41:24Z

@patrickfox so glad to hear your take on it! I definitely don't think ngModel should add tabindex by itself, so I restricted it to use cases with roles...perhaps those use cases could be expanded upon. I like your idea about adding tabindex for interactive roles, and that's pretty much how it works already: if ngModel matches the scenarios we want to backfill with support, tabindex is added if not already present. Interaction events are still required to take accessibility the rest of the way–perhaps missing events would be a good opportunity to log a warning, like in https://github.com/rackt/react-a11y or https://github.com/angular/material. I can tell you already that people really like disabling warnings. :)

For ngClick on a non-blacklisted element without a role, it adds tabindex and role=button (if config allows). I will admit it's pretty heavy-handed, but it addresses one of the biggest abuses in Angular (ngClick on DIV elements). ngAria should handle common use cases before tackling intricate ARIA patterns like tabs, and that one is pretty straight-forward.

If we take a step back and look at what people expect from ngAria, it's to get accessibility for free: e.g. "I include this module, it makes my app accessible". Just including the module isn't enough, however–most of the time you also have to add a role to tell ngAria to "fix" something. This restricts it to certain use cases, but makes it less "automagical", since you still have to know which role to choose. At that point, how much do we gain by adding tabindex when you're already on the hook to manage your own roles? Should we even encourage people to use interactive roles like link when the native a tag does the same job, but better?

This last question I think about a lot when adding features to ngAria–I don't think an Angular module should encourage ARIA patterns like role=radiogroup when native radio inputs do the job so well, and you can style them. At some point I decided "fixing" those widgets with tabindex had diminishing returns. There is often too much interaction complexity for ngAria to anticipate everything, but we can consider common use cases, starting the list of interactive roles–many of which are already addressed.

marcysutton · 2015-07-05T22:50:04Z

I should add, I have another PR forthcoming to limit ARIA usage on native controls, since right now they introduce redundancy. For example, native inputs currently get aria-checked from ngModel, when they shouldn't. That work will come after this PR but I'm definitely keeping it in mind as we go through this discussion.

marcysutton · 2015-08-02T19:22:23Z

Okay, I finally got your feedback incorporated @gkalpak. I considered @patrickfox's points about interactive roles, but decided to keep the logic as-is. ngAria already checks for some of those roles under the hood for ngModel before adding tabindex (checkbox, radio, etc.)–it compares those roles against a nodeBlacklist including buttons, links, and form controls. Many custom ARIA patterns are simply too complex for ngAria to support, such as radiogroup and menuitem...so I left those out. Keep in mind the aria-checked refactor will be in a different PR, this is only concerning tabindex.

gkalpak · 2015-08-04T08:30:37Z

src/ngAria/aria.js

@@ -226,7 +235,7 @@ ngAriaModule.directive('ngShow', ['$aria', function($aria) {
          }
        },
        post: function(scope, elem, attr, ngModel) {
-          var needsTabIndex = shouldAttachAttr('tabindex', 'tabindex', elem);
+          var needsTabIndex = shouldAttachAttr('tabindex', 'tabindex', elem, nodeBlackList);


shouldAttachAttr() does not take nodeBlackList into account. I think ti should...
(Either take it into account or remove this 4th argument - to avoid confusion.)

gkalpak · 2015-08-04T08:39:31Z

It LGTM (with the exception of little comment regarding shouldAttachAttr()).

I am wondering if there should be a BREAKING CHANGE notice about not trying to tabindex radiogroup.

* Do not put tabindex on native controls using ng-model or ng-click * Uses a single nodeBlacklist to limit which elements receive support Closes angular#11500

marcysutton · 2015-08-04T18:55:52Z

@gkalpak I removed nodeBlackList from that shouldAttachAttr() call, since the directive is already checking for valid use cases before applying attributes.

The radiogroup reference was only in a test, there is no change to the code. So I don't think it's a breaking change.

gkalpak · 2015-08-05T10:47:55Z

@marcysutton, I don't think the ngModel directive checks for valid usecases before adding a tabindex.
AFAIKT, it only checks the "shape", which does not account for elements in the nodeBlackList.
WDYT ?

The radiogroup reference was only in a test, there is no change to the code.

Good point 😄 No breaking change then !

marcysutton · 2015-08-12T20:18:34Z

The check that it does is a bit weird, to be honest...the fact that it checks for type OR role is an artifact of the incorrect functionality this refactor is trying to fix. ngAria added ARIA attributes to native elements that didn't need them, i.e. <input>, and did so by checking for types. I'm thinking that type should just go away, since we are factoring the native input stuff out completely, but that is a pretty major breaking change.

It reminds me of the bugs I uncovered with aria-required in #11374 (comment): ngAria behaves differently when elements use type vs. role, the latter of which is what's actually valid and needed for custom controls.

Do you think we can kill the type checks and streamline ngAria to hinge off of role attributes and tagNames instead?

gkalpak · 2015-08-17T19:15:24Z

I am not sure how much of a breaking change it would be. Since this is on the 1.4.x milestone, we shouldn't introduce significantly different behaviour.

If you think it will not break stuff (much), I'd say go for it. Else, let's improve what can be "backwards compatibly" improved and defer bigger changes to 1.5.x.

marcysutton · 2015-08-25T17:04:38Z

Ok, I think it's worth keeping it as-is for now and swapping the type check for the nodeBlackList later, in 1.5x. Keeping in mind there is another PR coming for aria-checked that could warrant that change earlier....might as well let this work go forward.

gkalpak · 2015-08-27T10:37:07Z

@marcysutton, is this PR ready for final review/merging ?

marcysutton · 2015-09-03T16:54:54Z

@gkalpak yup! I think it should stay as-is until the next PR, which I can start as soon as this is merged.

gkalpak · 2015-09-04T12:08:54Z

LGTM

petebacondarwin · 2015-09-07T08:36:25Z

LGTM

googlebot added the cla: yes label Jul 2, 2015

marcysutton reviewed Jul 2, 2015
View reviewed changes

marcysutton force-pushed the ngaria-tabindex branch from c37c8a8 to 25769e8 Compare August 2, 2015 19:13

gkalpak reviewed Aug 4, 2015
View reviewed changes

marcysutton added 2 commits August 4, 2015 11:26

fix(ngAria): clean up tabindex usage

f129167

* Do not put tabindex on native controls using ng-model or ng-click * Uses a single nodeBlacklist to limit which elements receive support Closes angular#11500

doc(ngAria): Update accessibility guide

fd15b73

marcysutton force-pushed the ngaria-tabindex branch from 25769e8 to fd15b73 Compare August 4, 2015 18:53

Narretz added type: bug severity: broken expected use component: ngAria labels Aug 8, 2015

Narretz added this to the 1.4.x milestone Aug 8, 2015

marcysutton mentioned this pull request Aug 12, 2015

ngAria disables tabbing to a radio set when no default value selected #12492

Closed

marcysutton closed this in b8e3561 Sep 7, 2015

marcysutton deleted the ngaria-tabindex branch October 14, 2015 21:12

gkalpak mentioned this pull request Oct 15, 2015

fix(ngAria): remove tabindex from radio buttons #13095

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngAria): clean up tabindex usage #12262

fix(ngAria): clean up tabindex usage #12262

marcysutton commented Jul 2, 2015

marcysutton Jul 2, 2015

gkalpak commented Jul 2, 2015

marcysutton commented Jul 2, 2015

patrickfox commented Jul 3, 2015

gkalpak commented Jul 3, 2015

marcysutton commented Jul 5, 2015

marcysutton commented Jul 5, 2015

marcysutton commented Aug 2, 2015

gkalpak Aug 4, 2015

gkalpak commented Aug 4, 2015

marcysutton commented Aug 4, 2015

gkalpak commented Aug 5, 2015

marcysutton commented Aug 12, 2015

gkalpak commented Aug 17, 2015

marcysutton commented Aug 25, 2015

gkalpak commented Aug 27, 2015

marcysutton commented Sep 3, 2015

gkalpak commented Sep 4, 2015

petebacondarwin commented Sep 7, 2015

fix(ngAria): clean up tabindex usage #12262

fix(ngAria): clean up tabindex usage #12262

Conversation

marcysutton commented Jul 2, 2015

marcysutton Jul 2, 2015

Choose a reason for hiding this comment

gkalpak commented Jul 2, 2015

marcysutton commented Jul 2, 2015

patrickfox commented Jul 3, 2015

gkalpak commented Jul 3, 2015

marcysutton commented Jul 5, 2015

marcysutton commented Jul 5, 2015

marcysutton commented Aug 2, 2015

gkalpak Aug 4, 2015

Choose a reason for hiding this comment

gkalpak commented Aug 4, 2015

marcysutton commented Aug 4, 2015

gkalpak commented Aug 5, 2015

marcysutton commented Aug 12, 2015

gkalpak commented Aug 17, 2015

marcysutton commented Aug 25, 2015

gkalpak commented Aug 27, 2015

marcysutton commented Sep 3, 2015

gkalpak commented Sep 4, 2015

petebacondarwin commented Sep 7, 2015