ngAria - incorrect "aria-checked" and "tabindex" applied to checkbox/radio

[DmitryEfimenko]

Hello angular team!

Overview of the Issue - incorrect attributes aria-checked and tabindex are applied to the input of type radio: when radio is selected, the following attributes are applied: aria-checked="false", tabindex="-1".
Motivation for or Use Case - radio button never gets selected when pressing Tab
Angular Version(s) - 1.3.1 for all modules
Browsers and Operating System - Chrome 38.0.2125.111 m. Haven't tested in other browsers, but pretty sure it'll be the same.
Reproduce the Error - see Plunker.
Related Issues - not found
Suggest a Fix - Switching attribute value to ng-value fixes the issue, but it is inconsistent through out the angular's logic - it was able to figure out that the radio should be selected with the regular value attribute, but it could not figure out correct accessibility attributes. It appears the issue has to do with the following lines of code

[pkozlowski-opensource]

@DmitryEfimenko I think I agree with you - it looks like pretty much broken atm... It looks like a deliberate choice, though, looking at this test:

angular.js/test/ngAria/ariaSpec.js

Lines 463 to 482 in 5b23bc9

	it('should set proper tabindex values for radiogroup', function() {
	compileInput('<div role="radiogroup">' +
	'<div role="radio" ng-model="val" value="one">1</div>' +
	'<div role="radio" ng-model="val" value="two">2</div>' +
	'</div>');
	var one = element.contents().eq(0);
	var two = element.contents().eq(1);
	scope.$apply("val = 'one'");
	expect(one.attr('tabindex')).toBe('0');
	expect(two.attr('tabindex')).toBe('-1');
	scope.$apply("val = 'two'");
	expect(one.attr('tabindex')).toBe('-1');
	expect(two.attr('tabindex')).toBe('0');
	dealoc(element);
	});
	});

@arbus @marcysutton could you guys please help me understand this test? The way I'm reading it is it disables keyboard navigation with tab on non-selected elements which is pretty non-intuitive. Or is the test backward?

[marcysutton]

That looks like a valid bug to me @DmitryEfimenko and @pkozlowski-opensource! ngAria is attempting to make custom controls accessible, and causing a bug on native inputs (I just saw that in your Plunker).

See this Plunker showing the custom implementation: http://plnkr.co/edit/AP1Ts8ArjeREksp77xb6?p=preview To create this, I started with the ngModel Checkbox Demo on the ngAria Developer Guide and simply swapped out the role from checkbox to radio and saw tabindex go from 0 to -1. This makes the radio button by itself inaccessible from the keyboard. However, there is more than one way to implement radio buttons.

On the Angular Material radio button, for example, we put tabIndex="0" on the <md-radio-group>, a wrapping element that groups radio buttons together and uses aria-activedescendant to communicate the selected radio button to assistive technology (it also has role="radiogroup"). It could be that ngAria is providing tabindex of -1 on radio buttons for this purpose, but without the rest of it it's an incomplete solution. ngAria could bind tabIndex="0" on radio buttons so that they are operable by themselves. Or we could just add a radio-group example to the Developer Guide and leave it as-is.

[pkozlowski-opensource]

@marcysutton thnx for your input - this starts to get interesting! Now, I must admit that my knowledge about aria-related topics is very limited (shame on me!) but it looks like there sth to fix here.

[marcysutton]

@pkozlowski-opensource I just updated my answer because I looked at the original Plunker in this issue: ngAria is totally causing a bug on native inputs. I'm working on a PR for #10012, so I can look into it.

[arbus]

@marcysutton @pkozlowski-opensource @DmitryEfimenko This looks like the bug is at https://github.com/angular/angular.js/blob/master/src/ngAria/aria.js#L231. In the original plnkr example, the $scope.myVal is 2 while the value of the attr.value is "2". So they don't compare and aria-checked is set to false.

We can verify this by just switching $scope.myVal to "2" like so: plnkr

An easy fix for this is to convert the newVal to a string before comparing to attr.value. @marcysutton should I open a new PR for this fix?

[marcysutton]

@arbus go for it! It looks like there is a bug with the existing implementation. Thank you!