Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Conversation

btford
Copy link
Contributor

@btford btford commented Sep 18, 2014

I made some significant refactors to #8342, notably:

  • DRYing up the implementation
  • unDRYing up the specs so that the assertions/failures are more specific
  • Naming watch functions to make debugging 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 angular#5486 and angular#1600
@jeffbcross
Copy link
Contributor

This is awesome! LGTM

@btford
Copy link
Contributor Author

btford commented Sep 18, 2014

Landed as d1434c9.

@btford btford closed this Sep 18, 2014
@marcysutton
Copy link
Contributor

👍 👍 👍 Yay!

@tbosch
Copy link
Contributor

tbosch commented Sep 19, 2014

Hi Brian,
could you make the value of the service (e.g. watchExpr, ...) private, as those methods are very likely to change and I am not sure how reusable they are...? Just prefix everything with $$...

@btford
Copy link
Contributor Author

btford commented Sep 19, 2014

yep, on it.

return {
watchExpr: watchExpr,
ariaChecked: watchExpr('ngModel', 'ariaChecked'),
ariaDisabled: watchExpr('ngDisabled', 'ariaDisabled'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be removed from the service as only used by ngDisabled

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could aria-disabled be managed by ngDisabled itself?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could. Actually for most of these cases here and it would be even faster.
The idea was that we don't want to add more code to Angular core for people that don't need aria support.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who doesn't need ARIA support? :) I think many times people incorrectly assume their audience won't need it. If the framework can improve accessibility by default, it's a huge win. That said, it is important to not impact performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think by "don't need" tobias means "don't need angular to automatically do this." There are plenty of existing angular apps that have existing a11y strategies, for which this would be extra code weight.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants