-
Notifications
You must be signed in to change notification settings - Fork 879
docs(a11y-sweep): Apply baseline accessibility to the examples #1613
base: master
Are you sure you want to change the base?
Conversation
TOH also has a number of issues with keyboard operability. [Edit: I should say, later TOH examples.] |
<div> | ||
<label>name: </label> | ||
<input [(ngModel)]="hero.name" placeholder="name"> | ||
<label>name: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a project preference for nested inputs vs. for
/id
-based association?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No specific preference yet but I would prefer implicit labelling where possible as it removes the need to generate/bind unique IDs to controls in components. Although both options lead to valid accessible form controls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense, but I'd maybe like to see both techniques demonstrated at some point, simply because the explicit/non-nested labelling is pretty popular and also much easier to mess up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully agree, but don't think this is the place. I'm also working on an a11y cookbook that should see the light in the near future. That has a whole section on labelling, including the explicit labelling version. Here, however, the purpose is to get folks started with Angular2 so I think adding the extra stuff to generate the IDs right off the bat will distract from what the TOH docs are all about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I looked through the a11y cookbook PR a while back. Good stuff! Is it worth commenting on that PR now or are you making big changes?
I guess my concern with the implicit labelling is just that devs who're accustomed to doing explicit labelling incorrectly might learn something from seeing it done correctly here, whereas devs who are accustomed to doing implicit labelling are probably already doing it correctly. But I may be over-thinking it, and this is definitely an improvement in any case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah that one is outdated. Did that, then life happened, with some changes and edits. I just created a new one at #1625
Agree to a certain extent but what we don't want are devs copying and pasting it from TOH because they think that is what is needed to make Angular work
I think most devs looking here will be mostly concerned with getting their first app off the ground and we should not confuse them with extra code. So in the TOH examples I think we should look at the big wins that require little to no extra code. The rest will come later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think as long as it's accessible in some manner I'm OK. I can appreciate not wanting to confuse people but I also think it's important to position accessibility as a first-class concern and embed it throughout the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There I fully agree with you. This exercise is exactly about giving a11y that position but to find the balance where the lessons of the separate docs are not diluted. Stuff not added to to the examples due to complexity could eventually be added as comments/notes in the examples or docs.
@mattdistefano Thanks for the input! Agreed there are keyboard issues. Will be working through the TOH examples in the coming days with suggested changes. |
Sure thing. I was going to tackle some of this myself but have been too busy the last couple weeks. Let me know if you want some assistance, though. |
That sounds great. For one, please keep your eye on this PR and keep commenting. And just give me a day or three to get my thoughts in a row, then we see what makes sense. |
@@ -0,0 +1,9 @@ | |||
body, input[text], button { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it's present in the current TOH as well, but is the input[text]
selector doing anything? AFAIK this will target an input element with an attribute named 'text'. I'm guessing input[type="text"]
was the intent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm it is certainly not doing anything here. Result of copy&paste without looking. Already got it fixed locally, will push with next batch of changes. Not messing with it in the main css file though, as that is used by all the examples.
3cc9d89
to
8f918bb
Compare
Added Summary:
NOT FIXED
These would either conflict with the purpose of the example or make it overly complex. Recommendation to add a note on this later. |
8f918bb
to
9e885a4
Compare
Added Applied all changes from TOH-2 with same exclusions. |
9e885a4
to
95d782d
Compare
Added Applied all changes from TOH-2 with same exclusions. |
95d782d
to
f975e88
Compare
Added Applied all changes mentioned with TOH-2 where applicable in the new layout. Added similar fixes to the menu dashboard. Also similar exclusions. With the addition of no focus management. |
f975e88
to
bd3a864
Compare
Added Applied all changes mentioned with TOH-2 where applicable in the new layout. Added similar fixes to the menu dashboard. Also similar exclusions. Also did not add focus management and notifications to the screen reader for asynchronous content/actions As suggested before these could be added as notes/comments in the docs. Header levels on |
TOH application refactor complete. Based on this I would make the following recommendations. The following actions I recommend as MUST HAVE:
The examples must play reasonably well with screen readers. This means:
The following action I recommend as SHOULD HAVE: Further accessibility functions such as complex focus management and other Also implementation of full IMPORTANT: All these points are MUST HAVE in real world applications. Lets make a call on what is in and what is out. |
I like all of that. What I would personally do is: Add all those things that doesn't really change the game. Changing invalid For big game changers, say adding new events for keyboard, the What do you think (both @wardbell and @AlmeroSteyn) if we create a new kind of "section" (you know, a colored block) for a11y? A11y is a real thing, we should promote it everywhere and we could add this colored section (in green, I like green) with extra information. For example in this case, we can add a bit of text + snippet to say that we add a At least for the tutorial, we need to teach the best practices there. |
bd3a864
to
f591679
Compare
This stuff really needs to be merged! Every day people go to learn about Angular 2, they are learning anti-patterns. |
We agree! We had to postpone on our road to final. We're rev'ing up to review this, clean it up, and get it done. |
** DO NOT MERGE **
Starting with a11y sweep on the examples.
Background: The examples need to, where possible, comply with at least a minimum of accessibility features.
To kick off the sweep and discussion I have made a few subtle changes to the TOH-1 example app.
Violations corrected:
<dl>
.These kinds of accessibility issues can be fixed purely in HTML and CSS.