fix(ngAria): remove tabindex from radio buttons

[marcysutton]

There was some funky stuff going on when ngModel was set to null, which I fixed by adding a nodeBlackList check to the method setting tabindex.

The logic added to needsTabIndex should eventually be moved into the shouldAttachAttr function but it will require refactoring of other directives, so I decided to wait on that.

Closes #12492

[Narretz]

I think we should test that no element in the blacklist gets a tabindex, not only one. I think matsko's they helper might be good for this.

[gkalpak]

I believe this has been brought up before (#12262 (comment)). My understanding from that conversation was that it is a breaking change.

Is my understanding correct ? (If so a BC notice should added.)

On the minor side:

👍 for checking all elements in nodeBlackList.

I would also change the commit message to something like stop adding (instead of remove), because it gives the impression that something is being removed from the element.

[marcysutton]

@gkalpak I applied the nodeBlacklist to needsTabIndex as opposed to shouldAttachAttr, limiting it to only tabindex part of ngModel. Does this qualify as a breaking change?

@Narretz there was already a test ensuring tabindex didn't get added to those elements (https://github.com/marcysutton/angular.js/blob/ngaria-tabindex2/test/ngAria/ariaSpec.js#L619). What was missing was an ngModel test ensuring a null default value wouldn't trigger it. Should I expand on that, or does this cover it?

[Narretz]

@marcysutton Oh, I see. Although it's not only null, it happens with any value expect undefined when it's a radio.

I think we don't need this new test, we can make the original test better. The problem is that this test block https://github.com/angular/angular.js/blob/master/test/ngAria/ariaSpec.js#L619 behaves differently than tests that use compileElement, as in it doesn't digest after compilation. I've found this out when I was adding inputs with type checkbox and type radio to the list. Without a digest, the test fails only for type checkbox, with digest, it fails for checkbox and for radio.
I think since you said in #12492 (comment) that no native inputs should get tabindex, it makes sense to test for these types explicitly.

What do you think about replacing the above mentioned test with this one, and leaving the new test out?

    they('should not attach to native control $prop with ng-model', {
      'button': "<button ng-click='something'></button>",
      'a': "<a ng-href='#/something'>",
      'input[text]': "<input type='text' ng-model='val'>",
      'input[radio]': "<input type='radio' ng-model='val'>",
      'input[checkbox]': "<input type='checkbox' ng-model='val'>",
      'textarea': "<textarea ng-model='val'></textarea>",
      'select': "<select ng-model='val'></select>",
      'details': "<details ng-model='val'></details>"
    }, function(html) {
        compileElement(html);
        expect(element.attr('tabindex')).toBeUndefined();
    });

The they has the advantage that the element are compiled in isolation (doesn't really matter here), and that the expects is in the actual test block.

[marcysutton]

@Narretz I like it! I've updated the test with the one you provided, and it worked like a charm. Thank you.