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

feat(ngAria): New module to make a11y easier #8342

Closed
wants to merge 1 commit into
base: master
from

Conversation

@arbus
Contributor

arbus commented Jul 25, 2014

Adds various aria attributes to the built in directives.
This module currently hooks into ng-show/hide, input, textarea
button as a basic level of support for a11y. I am using this as a
base for adding more tags into the mix for form direction flow,
making ng-repeat updates atomic but the tags here are the most
basic ones.

Closes #5486 and #1600

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins Jul 25, 2014

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#8342)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

mary-poppins commented Jul 25, 2014

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#8342)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@arbus arbus added cla: yes and removed cla: no labels Jul 25, 2014

@IgorMinar IgorMinar added this to the 1.3.0 milestone Jul 25, 2014

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Jul 25, 2014

Member

Interesting. The current implementation is not ideal because it requires on reading the DOM state during digest (which is super slow), so we can't merge this as is. In many cases we could instead use $attrs.$observe to intercept attribute changes instead of watching them (that won't work for class attribute though because we use classList if available).

@naomiblack can you or someone else review this for correctness from the a11y perspective?

Member

IgorMinar commented Jul 25, 2014

Interesting. The current implementation is not ideal because it requires on reading the DOM state during digest (which is super slow), so we can't merge this as is. In many cases we could instead use $attrs.$observe to intercept attribute changes instead of watching them (that won't work for class attribute though because we use classList if available).

@naomiblack can you or someone else review this for correctness from the a11y perspective?

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Jul 25, 2014

Contributor

I think we're better off having a separate repo/module for this so that ARIA support doesn't block the 1.3 release, and so that we can iterate on and release better ARIA support independently of Angular's release cycle. I do think this is important enough to be maintained by angular-core.

(from #5486 (comment) )

@IgorMinar is it alright if I start a new repo for this? Or do we want to get this into 1.3?

nvm just saw this was triaged

Contributor

btford commented Jul 25, 2014

I think we're better off having a separate repo/module for this so that ARIA support doesn't block the 1.3 release, and so that we can iterate on and release better ARIA support independently of Angular's release cycle. I do think this is important enough to be maintained by angular-core.

(from #5486 (comment) )

@IgorMinar is it alright if I start a new repo for this? Or do we want to get this into 1.3?

nvm just saw this was triaged

@arbus

This comment has been minimized.

Show comment
Hide comment
@arbus

arbus Jul 26, 2014

Contributor

@IgorMinar Code is now changed to use $attrs.$observe where possible

Contributor

arbus commented Jul 26, 2014

@IgorMinar Code is now changed to use $attrs.$observe where possible

Show outdated Hide outdated src/ngAria/aria.js

@btford btford removed the gh: PR label Aug 20, 2014

Show outdated Hide outdated src/ngAria/aria.js
@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Aug 27, 2014

Contributor

@marcysutton do you think this is good to go?

Contributor

btford commented Aug 27, 2014

@marcysutton do you think this is good to go?

@btford btford assigned btford and unassigned naomiblack Aug 27, 2014

Show outdated Hide outdated src/ngAria/aria.js
Show outdated Hide outdated src/ngAria/aria.js
@marcysutton

This comment has been minimized.

Show comment
Hide comment
@marcysutton

marcysutton Aug 27, 2014

Member

@btford I have a few questions about the directive to try and understand the scope and intention. It definitely makes sense to add support for tried and tested patterns, such as ngShow, ngHide, and native form inputs. I want to make sure we aren't thinking too small for the likely use cases, including non-semantic and custom elements.

Member

marcysutton commented Aug 27, 2014

@btford I have a few questions about the directive to try and understand the scope and intention. It definitely makes sense to add support for tried and tested patterns, such as ngShow, ngHide, and native form inputs. I want to make sure we aren't thinking too small for the likely use cases, including non-semantic and custom elements.

Show outdated Hide outdated test/ngAria/ariaSpec.js
Show outdated Hide outdated src/ngAria/aria.js
@marcysutton

This comment has been minimized.

Show comment
Hide comment
@marcysutton

marcysutton Sep 2, 2014

Member

Ok, I verified that the latest update works on radio button inputs. I'm still curious, though, if we can extend this to non-semantic elements like <div role="checkbox"> since they're so common in apps these days. Although the role is typically forgotten along with aria-checked. For something as commonly named as ngAria I'd hope it would cover a likely use case: developers creating custom elements or directives needing ARIA attributes to support assistive technologies.

@btford we could merge this to keep things moving forward and then identify ways to improve accessibility of custom elements later, if that sounds good to you.

Member

marcysutton commented Sep 2, 2014

Ok, I verified that the latest update works on radio button inputs. I'm still curious, though, if we can extend this to non-semantic elements like <div role="checkbox"> since they're so common in apps these days. Although the role is typically forgotten along with aria-checked. For something as commonly named as ngAria I'd hope it would cover a likely use case: developers creating custom elements or directives needing ARIA attributes to support assistive technologies.

@btford we could merge this to keep things moving forward and then identify ways to improve accessibility of custom elements later, if that sounds good to you.

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Sep 2, 2014

Contributor

@marcysutton how would you envision that to work? An element with role="checkbox" would automatically add/remove aria-checked?

Contributor

btford commented Sep 2, 2014

@marcysutton how would you envision that to work? An element with role="checkbox" would automatically add/remove aria-checked?

@marcysutton

This comment has been minimized.

Show comment
Hide comment
@marcysutton

marcysutton Sep 2, 2014

Member

@btford exactly. If a role is encountered, automatically adding ARIA attributes (if they aren't already included) would be a great way for the framework to do some heavy lifting.

I will make a list of use cases, but here are a few:

role="radio" and aria-checked
role="checkbox" and aria-checked
role="tab" and aria-selected
role="progressbar" and aria-valuenow, aria-valuemin, aria-valuemax
role="slider" and aria-valuenow, aria-valuemin, aria-valuemax

Less common, but could easily be added:

role="menuitemradio" and aria-checked
role="menuitemcheckbox" and aria-checked
role="textbox" and aria-multiline="true"

Some of these patterns could be good opportunities to float up warnings to developers, as well. For example: if role="radio" is used without a parent role of radiogroup, or if role="tab" is not inside of an element with role="tablist", a warning could be added directing developers to accessibility information. Brian, I know we talked about this as a potential idea for Angular. My first question would be: should ARIA warnings like these be added to the ngAria module or something else?

Member

marcysutton commented Sep 2, 2014

@btford exactly. If a role is encountered, automatically adding ARIA attributes (if they aren't already included) would be a great way for the framework to do some heavy lifting.

I will make a list of use cases, but here are a few:

role="radio" and aria-checked
role="checkbox" and aria-checked
role="tab" and aria-selected
role="progressbar" and aria-valuenow, aria-valuemin, aria-valuemax
role="slider" and aria-valuenow, aria-valuemin, aria-valuemax

Less common, but could easily be added:

role="menuitemradio" and aria-checked
role="menuitemcheckbox" and aria-checked
role="textbox" and aria-multiline="true"

Some of these patterns could be good opportunities to float up warnings to developers, as well. For example: if role="radio" is used without a parent role of radiogroup, or if role="tab" is not inside of an element with role="tablist", a warning could be added directing developers to accessibility information. Brian, I know we talked about this as a potential idea for Angular. My first question would be: should ARIA warnings like these be added to the ngAria module or something else?

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Sep 2, 2014

Contributor

Right. The issue with custom elements is that we don't always know what bindings their state is based on.

For cases where we can log warnings at runtime without any extra work, I think it's appropriate to implement them as a part of this module.

For cases where there's a bit of extra work, we should consider a module for angular-hint.

Contributor

btford commented Sep 2, 2014

Right. The issue with custom elements is that we don't always know what bindings their state is based on.

For cases where we can log warnings at runtime without any extra work, I think it's appropriate to implement them as a part of this module.

For cases where there's a bit of extra work, we should consider a module for angular-hint.

@arbus

This comment has been minimized.

Show comment
Hide comment
@arbus

arbus Sep 2, 2014

Contributor

@marcysutton How about we just look for a checked attribute for custom elements with role="radio" and role="checkbox"?

I would also like to suggest adding aria-atomic to your list for ng-repeat. I noticed that when searching a large list rendered through any filter and the filter changes, the entire list is read back as opposed to only the changes.

Contributor

arbus commented Sep 2, 2014

@marcysutton How about we just look for a checked attribute for custom elements with role="radio" and role="checkbox"?

I would also like to suggest adding aria-atomic to your list for ng-repeat. I noticed that when searching a large list rendered through any filter and the filter changes, the entire list is read back as opposed to only the changes.

@marcysutton

This comment has been minimized.

Show comment
Hide comment
@marcysutton

marcysutton Sep 4, 2014

Member

@arbus that sounds like a good start!

aria-atomic on ng-repeat also sounds like a good idea. I don't have extensive experience with that property, is there any downside? Will only announcing part of a list make it hard to discern the context?

Member

marcysutton commented Sep 4, 2014

@arbus that sounds like a good start!

aria-atomic on ng-repeat also sounds like a good idea. I don't have extensive experience with that property, is there any downside? Will only announcing part of a list make it hard to discern the context?

@arbus

This comment has been minimized.

Show comment
Hide comment
@arbus

arbus Sep 5, 2014

Contributor

@marcysutton After some more research, looks like aria-atomic needs to be paired with aria-live regions to work correctly. I am currently experimenting with live regions first as there does not really seem to be a one size fit all solution to this.

The following has been added on your suggestion:
role="radio" and aria-checked
role="checkbox" and aria-checked
role="progressbar" and aria-valuenow, aria-valuemin, aria-valuemax
<input type="range"> and aria-valuenow, aria-valuemin, aria-valuemax
role="slider" and aria-valuenow, aria-valuemin, aria-valuemax
role="textbox" and aria-multiline="true"
role="menuitemradio" and aria-checked
role="menuitemcheckbox" and aria-checked

This one,(role="tab" and aria-selected), however needs for us to track clicks on it and does not take into account any logic that may be implemented to disable(step tabs on a wizard etc) so I think we may need to skip it as providing no aria information may be better than providing the wrong aria information

Contributor

arbus commented Sep 5, 2014

@marcysutton After some more research, looks like aria-atomic needs to be paired with aria-live regions to work correctly. I am currently experimenting with live regions first as there does not really seem to be a one size fit all solution to this.

The following has been added on your suggestion:
role="radio" and aria-checked
role="checkbox" and aria-checked
role="progressbar" and aria-valuenow, aria-valuemin, aria-valuemax
<input type="range"> and aria-valuenow, aria-valuemin, aria-valuemax
role="slider" and aria-valuenow, aria-valuemin, aria-valuemax
role="textbox" and aria-multiline="true"
role="menuitemradio" and aria-checked
role="menuitemcheckbox" and aria-checked

This one,(role="tab" and aria-selected), however needs for us to track clicks on it and does not take into account any logic that may be implemented to disable(step tabs on a wizard etc) so I think we may need to skip it as providing no aria information may be better than providing the wrong aria information

@marcysutton

This comment has been minimized.

Show comment
Hide comment
@marcysutton

marcysutton Sep 5, 2014

Member

@arbus, tabs are definitely tricky to get right. I'd agree that omitting ARIA for tabs is a good idea at this point.

I also don't think live regions make sense to add across the board (but I could be proven otherwise). Whether polling updates would be useful to AT users really depends on the context. I'd also want to make sure there aren't any performance issues before introducing them to Angular core.

I am experimenting with demos to make sure the use cases check out. One edit we should consider making now, however, is to check whether an ARIA attribute already exists before injecting it. We're doing that on AngularJS Material to make sure we don't stomp on top of the work by folks who know what they're doing. Any thoughts on that?

Member

marcysutton commented Sep 5, 2014

@arbus, tabs are definitely tricky to get right. I'd agree that omitting ARIA for tabs is a good idea at this point.

I also don't think live regions make sense to add across the board (but I could be proven otherwise). Whether polling updates would be useful to AT users really depends on the context. I'd also want to make sure there aren't any performance issues before introducing them to Angular core.

I am experimenting with demos to make sure the use cases check out. One edit we should consider making now, however, is to check whether an ARIA attribute already exists before injecting it. We're doing that on AngularJS Material to make sure we don't stomp on top of the work by folks who know what they're doing. Any thoughts on that?

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Sep 5, 2014

Contributor

I'd also want to make sure there aren't any performance issues before introducing them to Angular core.

+1; we should probably add a benchmark: https://github.com/angular/angular.js/tree/master/benchmarks

One edit we should consider making now, however, is to check whether an ARIA attribute already exists before injecting it. We're doing that on AngularJS Material to make sure we don't stomp on top of the work by folks who know what they're doing. Any thoughts on that?

+1; I think this makes sense.

Contributor

btford commented Sep 5, 2014

I'd also want to make sure there aren't any performance issues before introducing them to Angular core.

+1; we should probably add a benchmark: https://github.com/angular/angular.js/tree/master/benchmarks

One edit we should consider making now, however, is to check whether an ARIA attribute already exists before injecting it. We're doing that on AngularJS Material to make sure we don't stomp on top of the work by folks who know what they're doing. Any thoughts on that?

+1; I think this makes sense.

Show outdated Hide outdated test/ngAria/ariaSpec.js
@marcysutton

This comment has been minimized.

Show comment
Hide comment
@marcysutton

marcysutton Sep 5, 2014

Member

Ok, cool. Some more comments in the module would help maintainability, as well.

Member

marcysutton commented Sep 5, 2014

Ok, cool. Some more comments in the module would help maintainability, as well.

@arbus

This comment has been minimized.

Show comment
Hide comment
@arbus

arbus Sep 6, 2014

Contributor

@marcysutton existing aria tags won't be overwritten if they already exist now.

@btford I am looking at how to write benchmarks now. Is there any particular part of the code that you want to focus on?

Contributor

arbus commented Sep 6, 2014

@marcysutton existing aria tags won't be overwritten if they already exist now.

@btford I am looking at how to write benchmarks now. Is there any particular part of the code that you want to focus on?

@stevefaulkner

This comment has been minimized.

Show comment
Hide comment
@stevefaulkner

stevefaulkner Sep 6, 2014

It is great to see angular improving its accessibility for AT users. It's unclear, from my reading of the code, if the keyboard behaviours associated with the roles being added are also being built-in. For example, if a role=button is being used on a <div> it is imperative that the control is included in the focus order and can be activated using both the enter and space keys. If this is not the case then the addition of a role can degrade the user experience.

stevefaulkner commented Sep 6, 2014

It is great to see angular improving its accessibility for AT users. It's unclear, from my reading of the code, if the keyboard behaviours associated with the roles being added are also being built-in. For example, if a role=button is being used on a <div> it is imperative that the control is included in the focus order and can be activated using both the enter and space keys. If this is not the case then the addition of a role can degrade the user experience.

@marcysutton

This comment has been minimized.

Show comment
Hide comment
@marcysutton

marcysutton Sep 6, 2014

Member

@stevefaulkner that is such a good point! We definitely need to factor in tabIndex for the module to be useful. Thanks for the heads up.

Member

marcysutton commented Sep 6, 2014

@stevefaulkner that is such a good point! We definitely need to factor in tabIndex for the module to be useful. Thanks for the heads up.

@stevefaulkner

This comment has been minimized.

Show comment
Hide comment
@stevefaulkner

stevefaulkner Sep 6, 2014

The ARIA design patterns documents required role/state/properties and keyboard interaction for a heap of custom controls.

stevefaulkner commented Sep 6, 2014

The ARIA design patterns documents required role/state/properties and keyboard interaction for a heap of custom controls.

@marcysutton

This comment has been minimized.

Show comment
Hide comment
@marcysutton

marcysutton Sep 10, 2014

Member

@arbus this module should also handle tabIndex. There are some obvious places, such as role="button" and role="checkbox". The more challenging contexts will come from items like role="radio" where the tabIndex="0" is expected on the parent radiogroup role. How do you want to work this in?

Member

marcysutton commented Sep 10, 2014

@arbus this module should also handle tabIndex. There are some obvious places, such as role="button" and role="checkbox". The more challenging contexts will come from items like role="radio" where the tabIndex="0" is expected on the parent radiogroup role. How do you want to work this in?

@arbus

This comment has been minimized.

Show comment
Hide comment
@arbus

arbus Sep 11, 2014

Contributor

@btford @ThomasBurleson All the digest calls have been changed to use $apply. Also, use of ng-init has been removed.

@marcysutton, I just added tabindex for role="checkbox", role="menuitemcheckbox" role="button" and ngClick. Do you think we should add tabindex to ngDblclick as well?

As for the radiogroup problem, it looks like we can just set a tabindex=0 on the selected element tabindex=-1 on the other elements. This is the same approach shown in http://oaa-accessibility.org/example/28/ as well.

Also, do you think we should consider adding in a role="application" on ng-app?

Contributor

arbus commented Sep 11, 2014

@btford @ThomasBurleson All the digest calls have been changed to use $apply. Also, use of ng-init has been removed.

@marcysutton, I just added tabindex for role="checkbox", role="menuitemcheckbox" role="button" and ngClick. Do you think we should add tabindex to ngDblclick as well?

As for the radiogroup problem, it looks like we can just set a tabindex=0 on the selected element tabindex=-1 on the other elements. This is the same approach shown in http://oaa-accessibility.org/example/28/ as well.

Also, do you think we should consider adding in a role="application" on ng-app?

@stevefaulkner

This comment has been minimized.

Show comment
Hide comment
@stevefaulkner

stevefaulkner Sep 11, 2014

@arbus

Also, do you think we should consider adding in a role="application" on ng-app?

Suggest extreme caution with use of role=application

stevefaulkner commented Sep 11, 2014

@arbus

Also, do you think we should consider adding in a role="application" on ng-app?

Suggest extreme caution with use of role=application

@arbus

This comment has been minimized.

Show comment
Hide comment
@arbus

arbus Sep 11, 2014

Contributor

@stevefaulkner ah got it. Then we shouldn't add that as a default.

Contributor

arbus commented Sep 11, 2014

@stevefaulkner ah got it. Then we shouldn't add that as a default.

@marcysutton

This comment has been minimized.

Show comment
Hide comment
@marcysutton

marcysutton Sep 11, 2014

Member

@arbus DEFINITELY not on the role="application"; it would cause more harm than good. Thanks @stevefaulkner for your input.

For tabIndex, I think it does make sense to add for ngDblClick. Nice catch! Sounds good for radio buttons.

Member

marcysutton commented Sep 11, 2014

@arbus DEFINITELY not on the role="application"; it would cause more harm than good. Thanks @stevefaulkner for your input.

For tabIndex, I think it does make sense to add for ngDblClick. Nice catch! Sounds good for radio buttons.

Subra
feat(ngAria): New module to make a11y easier
Adds various aria attributes to the built in directives.
This module currently hooks into ng-show/hide, input, textarea
button as a basic level of support for a11y. I am using this as a
base for adding more tags into the mix for form direction flow,
making ng-repeat updates atomic but the tags here are the most
basic ones.

Closes #5486 and #1600
@arbus

This comment has been minimized.

Show comment
Hide comment
@arbus

arbus Sep 11, 2014

Contributor

@marcysutton tabindex has been added to ngDblClick now

Contributor

arbus commented Sep 11, 2014

@marcysutton tabindex has been added to ngDblClick now

@ThomasBurleson

This comment has been minimized.

Show comment
Hide comment
@ThomasBurleson

ThomasBurleson Sep 11, 2014

@arbus - ngAria is really great work!
@btford - angular-hint is very cool. Perhaps this should also be used in Angular Material ?

ThomasBurleson commented Sep 11, 2014

@arbus - ngAria is really great work!
@btford - angular-hint is very cool. Perhaps this should also be used in Angular Material ?

@marcysutton

This comment has been minimized.

Show comment
Hide comment
@marcysutton

marcysutton Sep 16, 2014

Member

@arbus @btford I thought of another thing that needs addressing: keyboard events on custom controls. For example, if an element has ng-click on it, we should probably fire ng-keypress unless the developer opts out. Does this make sense to go in the ngAria module?

Member

marcysutton commented Sep 16, 2014

@arbus @btford I thought of another thing that needs addressing: keyboard events on custom controls. For example, if an element has ng-click on it, we should probably fire ng-keypress unless the developer opts out. Does this make sense to go in the ngAria module?

@arbus

This comment has been minimized.

Show comment
Hide comment
@arbus

arbus Sep 17, 2014

Contributor

@marcysutton So you mean if a person tabs into an item with ng-click on it, we should listen for say a space bar keypress and fire the ng-click?

Contributor

arbus commented Sep 17, 2014

@marcysutton So you mean if a person tabs into an item with ng-click on it, we should listen for say a space bar keypress and fire the ng-click?

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Sep 18, 2014

Contributor

@arbus @marcysutton I paired with @tbosch on this yesterday. Made some changes, but I think it's ready to land. I squashed my changes into @arbus's commit, and created a new PR at #9157. I'd be happy to keep iterating on it once it's in master, but we need to get this in soon for 1.3.

Going forward, we can add new features to this module, but keep them disabled by default so they aren't breaking.

Contributor

btford commented Sep 18, 2014

@arbus @marcysutton I paired with @tbosch on this yesterday. Made some changes, but I think it's ready to land. I squashed my changes into @arbus's commit, and created a new PR at #9157. I'd be happy to keep iterating on it once it's in master, but we need to get this in soon for 1.3.

Going forward, we can add new features to this module, but keep them disabled by default so they aren't breaking.

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Sep 18, 2014

Contributor

I landed this as d1434c9 with some changes! 💃

@marcysutton @arbus @ThomasBurleson @stevefaulkner thanks so much for your input! If there are more things we should add, can we start threads in new issues to discuss building upon what we have here?

Contributor

btford commented Sep 18, 2014

I landed this as d1434c9 with some changes! 💃

@marcysutton @arbus @ThomasBurleson @stevefaulkner thanks so much for your input! If there are more things we should add, can we start threads in new issues to discuss building upon what we have here?

@btford btford closed this Sep 18, 2014

@david-driscoll

This comment has been minimized.

Show comment
Hide comment
@david-driscoll

david-driscoll Sep 18, 2014

Would this override something like ngTouch's touch supported ng-click?

david-driscoll commented Sep 18, 2014

Would this override something like ngTouch's touch supported ng-click?

@btford

This comment has been minimized.

Show comment
Hide comment
@btford
Contributor

btford commented Sep 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment