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

feat: add WCAG levels #1270

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

aguscha333
Copy link

@aguscha333 aguscha333 commented May 9, 2021

Closes #1209

Summary

This PR adds the WCAG level to each checklist item so it's easier to know what checks to prioritize when trying to make a site accessible.

Notes / Questions

  • Do you think that the placement of the new information is correct / the best way to display it?
    I tried putting it in the part of the checklist item that is always visible but there there was a concern with responsiveness and the full Level: AAA was taking too much space.
  • @ericwbailey you mentioned in the issue that some checklist items might represent multiple success criterions. At least from what I saw each checklist item has only one WCAG link and each of those links only had one WCAG level asociated.
    That being said, I know for a fact that for example there are different contrast requirements for different levels but those don't seems to be covered in the checklist, just mention the one for level AA that is the minimum. Any thoughts on this? do you think that it's ok as I put it?
  • There was one checklist item that linked to a WCAG technique instead of a "Understanding WCAG" rule and didn't have a level asociated to for that one I put Level: doesn't apply. I thought about putting nothing at all but then it would have looked like it was missing.

Preview

image

@boring-cyborg boring-cyborg bot added data Issues with content sourced from JSON data files. markup Issues dealing with markup styling Issues dealing with our Sass/CSS. labels May 9, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented May 9, 2021

Thank you for opening this Pull Request! Please make sure you've read our Code of Conduct and Contributing guidelines.

@mxmason
Copy link
Member

mxmason commented May 9, 2021

Thank you so much for this pull request, @aguscha333! I love the idea of clarifying the WCAG level(s) associated with each checklist item.

I did a major overhaul of the checklist data structure in #1268, and that blocks this PR for now. Once #1268 merges, I am happy to take responsibility for any merge conflicts that you experience here.

Some thoughts this PR itself:

  • I will leave @ericwbailey to take point on the placement and presentation of this new information
  • I think we could expand the data to associate more WCAG criteria with each checklist item, where appropriate. This could be a separate PR; the current work is a great start
  • I think Level: doesn't apply is fine for the cases where there's not a relevant success criterion

@mxmason mxmason requested a review from ericwbailey May 9, 2021 18:44
@mxmason mxmason assigned mxmason and aguscha333 and unassigned mxmason May 9, 2021
@mxmason mxmason linked an issue May 9, 2021 that may be closed by this pull request
@boring-cyborg boring-cyborg bot added the javascript Issues dealing with JavaScript. label May 9, 2021
src/js/checklist.js Outdated Show resolved Hide resolved
src/_data/checklists.json Outdated Show resolved Hide resolved
src/_data/checklists.json Outdated Show resolved Hide resolved
@scottaohara
Copy link
Member

scottaohara commented May 10, 2021

While I realize this content predates the work being done here (which all the work in this PR seems pretty great from what I've reviewed) I was reviewing the actual checklist content as well. There are some SCs that need to be updated as they are not accurately referenced, and I figured it would be better to just lump that all into this PR, rather than create any merge conflicts or what not.

So, here's the issues that jumped out at me when reviewing:

Make sure that button, a, and label element content is unique and descriptive.

this is likely more a 2.4.4 issue for links (possibly 2.4.9 depending on context) and a 2.4.6 issue for labels... possibly buttons. It's a bit dicey putting these all together, but the point being that 1.3.1 doesn't really apply here. I'm not sure exactly what you want to do here, but I suggest at the very least removing the SC if you can't cite multiple SCs.

Use landmark elements to indicate important content regions.

This would be a 1.3.1 issue. 4.1.2 is more about interactive content

Avoid using the autofocus attribute.

Generally agree with this, but arguable if this would be a 2.4.3 issue. Sometimes using autofocus is exactly what you should do. Might be worth at least stating this is something to watch out for, rather than how this presently reads (to me) that it's an autofail.

Remove title attribute tooltips.

This is arguably more of a 2.1.1 keyboard issue as - if the content is actually important - would generally be inaccessible to keyboard only users. since title is generally exposed to AT, 4.1.2 doesn't really hold true here.

Check to see that keyboard focus order matches the visual layout.

1.3.2 may not apply here as content doesn't necessarily need to match visual focus order. 2.4.3 may be more appropriate to cite.

headings

All checklist items in the headings section incorrectly cite 2.4.6. 2.4.6 is about the quality of heading text, but has nothing to do with whether the text is properly exposed as a heading. These would all be 1.3.1/best practice issues.

Use the th element for table headers (with appropriate scope attributes).

Unsure why this is cited as a 4.1.1 issue. Seems it should be a 1.3.1

Use the caption element to provide a title for the table.

Again, not a 2.4.6 unless there is a <caption> already implemented which has crummy text.

All inputs in a form are associated with a corresponding label element.

Not a 3.2.2 issue. Typo for 3.3.2?

Associate input error messaging with the input it corresponds to.

Should be 1.3.1 and/or 4.1.2.

Ensure that media controls use appropriate markup.

Probably should be 4.1.2 since that's about interactive controls

Confirm that transcripts are available.

Technically yes, audio content is 1.1.1. But alternatives to audio content would generally be cited under 1.2.3

Check your content in specialized browsing modes.

Would generally point to this as a strongly recommended best practice, not a wcag failure, particularly not this one.

Remove horizontal scrolling.

would qualify some content - e.g., tables, maps, large graphs - are totally fine to have bi-directional scrolling. dont' want to make people think they have to negate table semantics and then potentially wind up with other wcag failures (e.g., 1.3.2) when content no longer makes sense in the reading order.

@mxmason
Copy link
Member

mxmason commented May 10, 2021

Thanks for these notes, @scottaohara!

@aguscha333, I'd be happy to implement the content changes Scott has pointed out, if you'd like. I think these changes could either go into this PR, or into a separate one dedicated to the rewrite.

@aguscha333
Copy link
Author

@mxmason thanks for offering. If you don't mind I'd like to try to implement the changes myself to keep contributing to the repo. Those changes do touch more technical subjects about WCAG itself so what I will need is help with a thorrow review since I only recently started learning about accessibility so I could have some conceptual errors in the changes.
I do think that it's better to do it in a separate PR since the new changes have nothing to do with the initial purpose of this PR. I can create a branch off of this one to get minimal conflicts.

@mxmason
Copy link
Member

mxmason commented May 11, 2021

@aguscha333 sounds good to me! feel free to tag me directly if you need anything!

@SaptakS
Copy link
Member

SaptakS commented May 11, 2021

Sounds like a good plan to me too. I guess we are tracking it in #1277 now (thanks @mxmason for creating the issue!). In that case, according to me, this PR is ready to be merged. cc @scottaohara @ericwbailey

@ericwbailey
Copy link
Member

I'd like to hold off on merging until #1277 is addressed. I don't feel comfortable merging if there are misrepresented SCs.

@aguscha333
Copy link
Author

@ericwbailey @mxmason Tried implementing #1277 but going through the suggestions there are a lot that are more open ended and would require some more decision making from someone with more knowledge on the topic. I'm glad to keep contributing in the future but I think I need to sit this one out since I am still a newbie with a11y.

@scottaohara
Copy link
Member

if you think more work needs to go into correctly citing the WCAG SCs, then I would suggest you simply comment out the SCs for the checklist items where there is not a simple swap out of the errant SC for the correct one.

Then additional thought can go into the items that need more nuance, without continuing to cite inaccurate SCs while that work is being done.

@aguscha333
Copy link
Author

@scottaohara @ericwbailey sorry it took me soo long to get back to this.

Unfortunately comments are not allowed in JSON files so I had to remove them instead.

I took the removal approach to most of the items mentioned by @scottaohara since most of the comments looked like were meant to open up discussions rather than this should be exactly like this or that.

If anyone wants to jump in and do something different for the ones I removed, totally cool with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data Issues with content sourced from JSON data files. javascript Issues dealing with JavaScript. markup Issues dealing with markup styling Issues dealing with our Sass/CSS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the level of the rule directly on the item listed
5 participants