Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Roles dynamically added with ngAria #10318

Closed
wants to merge 2 commits into from

Conversation

@marcysutton
Copy link
Contributor

@marcysutton marcysutton commented Dec 4, 2014

To communicate the purpose of custom inputs with a type of range, checkbox or radio, ngAria now dynamically adds roles to custom radio buttons, checkboxes and sliders.

It also adds a role of button to any element with ngClick that is not a button, anchor, input or textarea (such as a div). This feature comes on the heels of #10288, which binds the keypress event for ngClick. With these two changes together, users of assistive technologies will know what they are focused on and be able to operate controls with the keyboard (this is important because ngAria dynamically adds tabIndex="0" with ngClick). However, not all uses of ngClick are used for buttons–sometimes developers bind ng-click on the document. In this scenario, a role can be overridden by the developer if button does not match expected behavior.

Note: This change does not implement any kind of flag allowing the user to opt out of roles being set.

Closes #9254 and #10012.

@googlebot
Copy link

@googlebot googlebot commented Dec 4, 2014

CLAs look good, thanks!

@googlebot googlebot added the cla: yes label Dec 4, 2014
@marcysutton marcysutton force-pushed the marcysutton:ngaria-roles branch from b420186 to 90eb3a9 Dec 4, 2014
@gkalpak
Copy link
Member

@gkalpak gkalpak commented Dec 4, 2014

I know it is unrelated to this PR, but I was wondering (with regard to line 209):

((type || role) === 'checkbox' ...

What if the element has both a type and a role (and they are different) ?
Is it even a "legitimate" use-case ?

@marcysutton
Copy link
Contributor Author

@marcysutton marcysutton commented Dec 4, 2014

@gkalpak not for checkbox. <input type="range"> and role="slider" is a use case, and there may be others....but they'd have to be looked at on a case by case basis. I'll keep it in mind though.

@gkalpak
Copy link
Member

@gkalpak gkalpak commented Dec 5, 2014

@marcysutton: Aha, thx. So, if I interpret your answer correctly, in the "general" case each attribute (type, role) should be checked separately:

((type === 'xyz') || (role === 'xyz') || ...)

But for checkboxes and radios, type and role (if set) will be the same, so it is OK to write:

((type || role) === 'xyz')

(which will essentially ignore role if type is defined, but this is OK because role shouldn't be checkbox/radio unless type is checkbox/radio as well.)

Is my thinking correct ?

compileInput('<div ng-click="someFunction()"></div>');
expect(element.attr('role')).toBe('button');
});
it('should not add role="button" to anchor', function() {

This comment has been minimized.

@caitp

caitp Dec 5, 2014
Contributor

nit: 2 empty lines between each suite/spec/block

beforeEach(injectScopeAndCompiler);

it('should add missing role="button" to custom input', function() {
compileInput('<div ng-click="someFunction()"></div>');

This comment has been minimized.

@caitp

caitp Dec 5, 2014
Contributor

why are we calling this compileInput ? there's no input here =)

This comment has been minimized.

@marcysutton

marcysutton Dec 5, 2014
Author Contributor

Good point! This is fixed.

@caitp caitp added this to the 1.3.7 milestone Dec 5, 2014
@@ -319,6 +329,10 @@ ngAriaModule.directive('ngShow', ['$aria', function($aria) {
}
});
}

if (!elem.attr('role') && (elem[0].nodeName !== 'BUTTON') && (elem[0].nodeName !== 'A')) {

This comment has been minimized.

@caitp

caitp Dec 5, 2014
Contributor

I wonder if it would be more readable to have something like && !IsNodeOneOf(elem, "BUTTON", "A") or something. I don't think it's problematic like this, but if there was one extra condition added it would be really hard to read.

This comment has been minimized.

@marcysutton

marcysutton Dec 5, 2014
Author Contributor

Great point. That conditional is a little awkward in its current form.

@marcysutton marcysutton force-pushed the marcysutton:ngaria-roles branch from 90eb3a9 to 91776c2 Dec 5, 2014
@marcysutton
Copy link
Contributor Author

@marcysutton marcysutton commented Dec 5, 2014

@gkalpak there is no "generic" case. ngAria is checking for specific scenarios to add the right attributes. type || role applies correctly to each case where it is being used.

@gkalpak
Copy link
Member

@gkalpak gkalpak commented Dec 5, 2014

OK, thx ! I get it (I think :))

@caitp caitp modified the milestones: 1.3.7, 1.3.8 Dec 9, 2014
@pkozlowski-opensource pkozlowski-opensource modified the milestones: 1.3.8, 1.3.7 Dec 9, 2014
@petebacondarwin petebacondarwin modified the milestones: 1.3.7, 1.3.8 Dec 15, 2014
@btford btford modified the milestones: 1.3.8, 1.3.9 Dec 19, 2014
@lgalfaso lgalfaso removed this from the 1.4.0-beta.2 / 1.3.11 milestone Feb 5, 2015
@marcysutton marcysutton force-pushed the marcysutton:ngaria-roles branch 2 times, most recently from cb6e000 to 0cdd758 Feb 7, 2015
@@ -310,6 +320,9 @@ ngAriaModule.directive('ngShow', ['$aria', function($aria) {
return true;
}
}
if (!elem.attr('role') && !isNodeOneOf(elem, ['BUTTON', 'A'])) {

This comment has been minimized.

@dylanb

dylanb Feb 7, 2015

blacklist should include INPUT and TEXTAREA

This comment has been minimized.

@marcysutton

marcysutton Feb 7, 2015
Author Contributor

Good call. But do people put ng-click on those? shudder

This comment has been minimized.

@dylanb

dylanb Feb 7, 2015

input type=image, type=submit, type=button will all commonly have ng-click attributes applied

This comment has been minimized.

@marcysutton

marcysutton Feb 7, 2015
Author Contributor

Great point. I updated the blacklist and added it to the dynamic keypress binding so there would be more consistency.

@marcysutton marcysutton force-pushed the marcysutton:ngaria-roles branch from 0cdd758 to 9133e14 Feb 7, 2015
@marcysutton
Copy link
Contributor Author

@marcysutton marcysutton commented Feb 7, 2015

@caitp I rebased this branch with the recently merged ngAria changes. The code now uses a node blacklist to add roles and bind keypress events.

@marcysutton marcysutton force-pushed the marcysutton:ngaria-roles branch from 9133e14 to 708eacb Feb 7, 2015
@marcysutton
Copy link
Contributor Author

@marcysutton marcysutton commented Feb 16, 2015

@Narretz @petebacondarwin can you take a look at this PR for adding roles dynamically? It's the last ngAria change that would be great to have in the 1.4 release. Thank you!

});

it('should add missing role="checkbox" to custom input', function() {
scope.$apply('val = true');

This comment has been minimized.

@gkalpak

gkalpak Feb 16, 2015
Member

What is the purpose of this line ?

This comment has been minimized.

@marcysutton

marcysutton Feb 17, 2015
Author Contributor

It was unnecessary. Cleaned up!

Marcy Sutton added 2 commits Dec 4, 2014
Adds missing roles: slider, radio, checkbox
Closes #10012
Closes #9254
@marcysutton marcysutton force-pushed the marcysutton:ngaria-roles branch from 708eacb to 7fafdf9 Feb 17, 2015
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Feb 18, 2015

LGTM

@andrewaustin
Copy link
Contributor

@andrewaustin andrewaustin commented Feb 24, 2015

Would love to see this in 1.3.x. We have some tags that are clickable and are currently having to do some extra work with ng-keypress to make them fully accessible. Having a blacklist rather than the current div & li whitelist would be extremely useful.

@ginader
Copy link

@ginader ginader commented Feb 27, 2015

👍

petebacondarwin added a commit that referenced this pull request Mar 2, 2015
Closes #9254
Closes #10318
petebacondarwin added a commit that referenced this pull request Mar 2, 2015
This change adds the missing roles: `slider`, `radio`, `checkbox`

Closes #10012
Closes #10318
petebacondarwin added a commit that referenced this pull request Mar 2, 2015
Closes #9254
Closes #10318
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Mar 2, 2015

Landed in master and cherry picked to 1.3.x for good measure.

@marcysutton
Copy link
Contributor Author

@marcysutton marcysutton commented Mar 2, 2015

Woohoo! Thanks @petebacondarwin! 🎉

hansmaad pushed a commit to hansmaad/angular.js that referenced this pull request Mar 10, 2015
This change adds the missing roles: `slider`, `radio`, `checkbox`

Closes angular#10012
Closes angular#10318
hansmaad pushed a commit to hansmaad/angular.js that referenced this pull request Mar 10, 2015
@marcysutton marcysutton deleted the marcysutton:ngaria-roles branch Apr 11, 2015
netman92 added a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
This change adds the missing roles: `slider`, `radio`, `checkbox`

Closes angular#10012
Closes angular#10318
netman92 added a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.