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

docs(input): better accessibility in examples #11079

Closed
wants to merge 2 commits into from

Conversation

marcysutton
Copy link
Contributor

The input and ngModel docs examples have no labels or accessible error handling. They could do more to encourage accessibility best practices. This change does two things:

  1. Adds labels to each input.
  2. Wraps error messages with `role="alert" so they will announce in a screen reader.

Even though Angular 2.0 is coming soon, these docs will live on with the 1.0 project so it makes sense to show more accessible examples. (Better late than never.)

@gkalpak
Copy link
Member

gkalpak commented Feb 17, 2015

I wonder if it is a good idea to use the <label>...<input .../></label> form (see below) in order to avoid clattering the views (and avoid having to add id attributes to inputs that don't need it).

<label>
  Some label:
  <input type="text" name="input" ng-model="example.text" />
</label>

@marcysutton
Copy link
Contributor Author

@gkalpak because this is primarily for developer education, I opted for explicit labels on purpose. A common accessibility problem is to leave off the for="" and not properly link the <label> and the <input> elements (or to leave off <label> entirely). Showing how they are linked together does more for developer education, which has been sorely lacking in Angular in the past.

You can read about implicit vs. explicit labeling vs. aria-labelledby here: http://www.paciellogroup.com/blog/2011/07/html5-accessibility-chops-form-control-labeling/

@gkalpak
Copy link
Member

gkalpak commented Feb 17, 2015

Interestingly enough, although the author seems to suggest using for / id instead of wrapping, the test results seem to indicate that wrapping is more reliable than for / id ! Do I miss something ?

@marcysutton
Copy link
Contributor Author

The support differences look pretty minimal to me. This post is also from 2012, so take it with a grain of salt.

@zamith
Copy link

zamith commented Feb 27, 2015

👍

@pauljadam
Copy link

+1 please fix a11y in the docs it's very very bad, all the angularJS books have not a11y just like the docs, wrapped labels are fine in my testing, yes it's less code and will look more readable, any a11y we can get the better!

<label for="input">Single word:</label>
<input id="input" type="text" name="input" ng-model="example.text"
ng-pattern="example.word" required ng-trim="false">
<span role="alert">
Copy link

Choose a reason for hiding this comment

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

Does role=alert announce assertively/immediately? Is that the desired experience for a form validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I tested it, it matched the speed that the visual message would come up in context to what you're typing...it was especially useful for validations related to input length, so you'd know exactly when the error occurred.

Copy link

Choose a reason for hiding this comment

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

That's really cool! Good to know.

@ckundo
Copy link

ckundo commented Feb 27, 2015

Huge +1 in general. Great to see improved accessibility in the docs, and the role=alert pattern on form validations is super cool and a unique accessibility feature to Angular. The duplicate ids might be a blocker until resolved, but otherwise, I vote ship it 🚀!

@marcysutton
Copy link
Contributor Author

Thanks for the feedback! I've simplified a lot of the labeling based on comments in this PR.

@marcysutton
Copy link
Contributor Author

@ckundo @gkalpak I did a search for <input> and found a bunch more places that needed accessibility help, including stuff like ng-src in ng/directive/attrs.js, where the "good" example is missing an alt attribute:

The buggy way to write it:

 <img src="http://www.gravatar.com/avatar/{{hash}}"/>

The correct way to write it:

 <img ng-src="http://www.gravatar.com/avatar/{{hash}}"/>

Can you take another look if you get a chance? Thanks so much.


<label>Single word:
<input type="text" name="input" ng-model="example.text"
ng-pattern="example.word" required ng-trim="false">
Copy link

Choose a reason for hiding this comment

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

Alignment with the attributes above is not quite right.

@ckundo
Copy link

ckundo commented Mar 2, 2015

Is there some guideline around line length? 80 characters is a sane guideline, but I don't know if the project adheres to that. @gkalpak do you know?

@ckundo
Copy link

ckundo commented Mar 2, 2015

@marcysutton it occurred to me that the role=alert changes might be better off in a separate PR. Do you think that change can be isolated?

<hr>
<p ng-class="style">Using String Syntax</p>
<input type="text" ng-model="style" placeholder="Type: bold strike red">
<label><input type="text" ng-model="style" placeholder="Type: bold strike red">
Copy link

Choose a reason for hiding this comment

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

Is this missing a closing tag? </label>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed! Good catch.

@@ -144,7 +144,7 @@
*
* The correct way to write it:
* ```html
* <img ng-srcset="http://www.gravatar.com/avatar/{{hash}} 2x"/>
* <img ng-srcset="http://www.gravatar.com/avatar/{{hash}} 2x" alt="Description" />
Copy link
Member

Choose a reason for hiding this comment

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

I suggest the same change is made to the buggy way above.
Since this example is about src vs ngSrc, we don't want to distract the reader into thinking alt has anything to do with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. That makes sense to me.

Copy link

Choose a reason for hiding this comment

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

@gkalpak @marcysutton why not add the alt to both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Great suggestion.

@ckundo
Copy link

ckundo commented Mar 2, 2015

Besides the formatting fixes, I think the role=alert and a vs button concerns might be better in a separate PR, but that may be a little fussy. Also noted that the last file in the changes has new content, which was different. All the other changes should have no effect on the presentation of the page, so thought I'd raise it.

@marcysutton thanks a million for this work. Keep it up!

edit:
If the maintainers are ok accepting these all as one PR, by all means keep them together, it's less work to keep them together at this point.

@gkalpak
Copy link
Member

gkalpak commented Mar 2, 2015

It generally LGTM. Really great work ! 👍 @marcysutton

A couple of notes:

  1. Alignment is not super-correct. It would nice to have everything align/indented consistently of course, but I can leave without as well :)
  2. I prefer closing /> (with the /), but that seems to not be directly related to this PR (plus is a matter of personal preference anyway).
  3. Although I generally agree that button is preferable to a (when used as a button), as do look better in certain cases. Couldn't we go with the as (e.g. for X "buttons") and put a role attribute or something. Since many people do use as anyway, it would be good to raise awareness around "doing it right" (if you have to).

@marcysutton
Copy link
Contributor Author

Thanks again for your thorough review, this is awesome!

  1. Edited alignment to be more consistent.
  2. I did a find and replace for <br> in the files I had edited and changed any occurrences to <br/> since it was pretty easy.
  3. The reason I changed the <a>s to <button>s is because they all had empty hrefs and were essentially doing the work of buttons (ng-click only, no routing). Adding role=button wouldn't negate the need for an empty href to make it a valid element. It's just easier to make them buttons. :)

If it's okay to leave this all in one branch, it will save me from having to do the work of splitting it up and creating additional PRs. In the future I'll try to break it up though. I know it's a big one! Thanks again for all your help.

@ckundo
Copy link

ckundo commented Mar 2, 2015

@marcysutton thanks, LGTM! 👍

@Narretz Narretz added this to the Backlog milestone Mar 8, 2015
<input ng-model="name"> <br>
<textarea ng-model="html"></textarea> <br>
<input ng-model="name"> <br/>
<textarea ng-model="html"></textarea> <br/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These should have labels too. Oops! I'll fix them up later.

@marcysutton marcysutton deleted the input-docs branch April 14, 2015 18:29
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants