Accessibility fixes (and tests) by sorin-davidoi · Pull Request #1227 · rust-lang/crates.io

sorin-davidoi · 2018-01-09T22:46:12Z

Most commits have a message explaining why I think the change is necessary. Let me know if anything is unclear or if I should follow up with other changes.

Shoutout to @chriskrycho for his talk Becoming a Contributor which encouraged me to do this!

Labels are hidden, but still accessible for screen readers (https://webaim.org/techniques/css/invisiblecontent/#absolutepositioning).

When using tab to navigate between focusable elements, it does not make sense to have two adjacent links pointing to the same address. Another solution would be to place the img tag inside the second address tag, but that leads to some style issue so I have opted for this approach instead.

Focusable elements should have a focus style, but that is overwritten. This will reuse the style applied on hover for focus.

Browsers which do not support these tags will fallback to interpreting them as divs. Ideally, the nav should be a header with a nav inside, but since the header only contains navigation link, I have omitted it and just used nav.

Will aid screen reader users find the content faster.

Semantically, the crates are sorted by time, so usage of ul is incorrect.

It is important not to break the focus order users expect. https://dequeuniversity.com/rules/axe/2.6/tabindex

Focusable elements need to have a focus style (http://www.outlinenone.com/).

It does not really make sense for it to be the first button in the tab order, since it is not one of the main actions.

sorin-davidoi · 2018-01-10T00:04:16Z

+export default {
+    rules: {
+        'color-contrast': {
+            enabled: false,


There are multiple elements that currently fail this test. I have disabled it since I think it needs to be addressed by someone more inclined towards design.

I've opened a new issue for addressing these.

sorin-davidoi · 2018-01-10T00:05:26Z

I have integrated ember-a11y-testing in order to avoid these problems from appearing again.

…opment This will prevent the violations from being highlighted on the page, which is annoying since the color-contrast rule is currently disabled during tests and thus a lot of elements would be highlighted. https://github.com/ember-a11y/ember-a11y-testing#development-usage

Screen readers can announce to the user when the section has finished loading. The loaders were missing the alt attribute, but instead of adding it I have moved they to CSS, since the content of the section is not (normally) available to screen readers when aria-busy="true".

http://www.a11ymatters.com/pattern/pagination/

sorin-davidoi · 2018-01-10T21:28:07Z

 </form>

-<div id="main" class='inner-content'>
+<main id="main" class='inner-content'>


This is not ideal since each page should define it's own main - but I'm not familiar enough with the pages and don't have time to go through them all, so I think this is good enough for now.

carols10cents · 2018-01-14T19:34:13Z

Thank you so much for this!!!!!! I'm taking a look now!!

carols10cents · 2018-01-14T20:02:42Z

@@ -1,4 +1,4 @@
-<ul>
+<ol>


Should keyword-list and category-list be changed to ols too? They're both ordered by popularity, and those components are only used on the home page.

What about crate-downloads-list? This is used on the currently-logged-in user's dashboard and are ordered by recent downloads.

Not sure how far we should go with this in this PR-- there are lists of crates that are just divs rather than uls right now that should also perhaps be ols, like every list of crates (which should always have SOME order) or the list of Latest Updates of crates you're following on your dashboard

Yes, probably those need to be updated as well. Would rather do it in a follow-up PR if that's OK.

Yup, that's totally fine!

carols10cents · 2018-01-14T20:05:17Z


 <a href='https://github.com/rust-lang/crates.io' class='fork-me'>
-  <img src='/assets/forkme.png'/>
+  <img src='/assets/forkme.png'/ alt="Fork me on GitHub">


I think the slash after the src attribute is to close the tag, so should go after the alt attribute.

carols10cents · 2018-01-14T20:33:41Z

            {{#if (is-clipboard-supported)}}
-              {{#copy-button clipboardTarget="#crate-toml" success=(action 'copySuccess') error=(action 'copyError')}}
+              {{#copy-button clipboardTarget="#crate-toml" success=(action 'copySuccess') error=(action 'copyError') title="Copy to clipboard"}}
                    {{svg-jar "copy" alt="Copy to clipboard"}}


The title on the button isn't repetitive with the alt text on the svg inside the button? I really don't know how screenreaders will read this, just wanted to point it out :)

aXe flagged this as an error, and Orca (the screen reader I test with) didn't read the text from the svg.

carols10cents · 2018-01-14T20:35:18Z

        {{/if}}
    </div>
-    <div class='authorship'>
+    <section class='authorship' aria-label="Authorship">


This class being named "authorship" is kind of... out of date. There's a lot more metadata in this crate than just the author. Not sure what a better label would be- "Crate info"? "Crate metadata"?

carols10cents · 2018-01-14T21:08:23Z

    }

-    > div { margin: 0 15px; }
+    > section { margin: 0 15px; }


I think the div on line 146 of this file needs to be updated to a section as well, otherwise the columns on the home page take up a variable amount of width when they should be taking up ~1/3 of the width, which starts happening at this commit.

carols10cents

Thank you so much for doing this important work that has been neglected for far too long!!!! I'm really excited to get these changes in. I just have a few places I commented that need some small tweaks before we can merge, and there's one spot I have a question about how many other places we should be using ol, but I don't necessarily want to hold up this whole PR on those changes if it seems like that'll be a bigger change.

carols10cents

Just one more tiny change :)

carols10cents · 2018-01-17T15:53:39Z

+</footer>
+
+<a href='https://github.com/rust-lang/crates.io' class='fork-me'>
+  <img src='/assets/forkme.png'/ alt="Fork me on GitHub">


Just one more thing, github hid my comment on this because I made it on a commit and not the overall diff-- could you move the slash that's now between the src and the alt attributes to the end of the img tag so that it closes the tag? Then this PR should be good to go!

@carols10cents Done!

carols10cents

Thank you!!

carols10cents · 2018-01-17T20:03:57Z

bors: r+

@chriskrycho

1227: Accessibility fixes (and tests) r=carols10cents Most commits have a message explaining why I think the change is necessary. Let me know if anything is unclear or if I should follow up with other changes. Shoutout to @chriskrycho for his talk [Becoming a Contributor](https://www.youtube.com/watch?v=Abu2BNixXak) which encouraged me to do this!

bors-voyager · 2018-01-17T20:24:18Z

Build succeeded

continuous-integration/travis-ci/push

sorin-davidoi added 18 commits January 9, 2018 22:29

fix(templates/application): Add labels to search input fields

0929810

Labels are hidden, but still accessible for screen readers (https://webaim.org/techniques/css/invisiblecontent/#absolutepositioning).

fix(styles/home/button): Use hover style for focus

dd877cc

Focusable elements should have a focus style, but that is overwritten. This will reuse the style applied on hover for focus.

fix(templates/index): Use section for each crate list

bd4c191

Will aid screen reader users find the content faster.

fix(templates/components/crate-list): Use ol instead of ul

89c8ede

Semantically, the crates are sorted by time, so usage of ul is incorrect.

fix(templates/application): Remove tabindex="1" from search fields

3e94117

It is important not to break the focus order users expect. https://dequeuniversity.com/rules/axe/2.6/tabindex

fix(templates/application): Add alt text for the "For me on GitHub" link

f0c8377

fix(styles/app/button): Do not set outline: 0

72407de

Focusable elements need to have a focus style (http://www.outlinenone.com/).

fix(styles/me): Set focus style for load more button

522eb3e

fix(templates/application): Move fork button to the end of the document

1aa4b29

It does not really make sense for it to be the first button in the tab order, since it is not one of the main actions.

fix(componentns/user-avatar): Add alt text (name, username)

ea4fb9c

fix(crate/version): Nest li directly inside ul

961ba48

fix(templates/crate/version): Add title to copy-to-clipboard button

7c58178

feat(templates/crate/version): Use section for readme and authorship

c27533d

fix(templates/crate/owners): Add label to username input field

4a36d50

chore(npm/dev-dependencies): Install ember-a11y-testing

274c841

test(acceptance): Create accessibility tests

ed4333a

sorin-davidoi commented Jan 10, 2018

View reviewed changes

sorin-davidoi added 3 commits January 10, 2018 10:52

fix(templates/crates): Make pagination accessible

2cd5ac1

http://www.a11ymatters.com/pattern/pagination/

sorin-davidoi changed the title ~~Accessibility fixes~~ Accessibility fixes (and tests) Jan 10, 2018

sorin-davidoi commented Jan 10, 2018

View reviewed changes

carols10cents reviewed Jan 14, 2018

View reviewed changes

carols10cents mentioned this pull request Jan 14, 2018

Enable color-contrast accessibility test and fix all violations #1233

Open

carols10cents reviewed Jan 14, 2018

View reviewed changes

carols10cents requested changes Jan 14, 2018

View reviewed changes

carols10cents added the S-waiting-on-author label Jan 14, 2018

sorin-davidoi added 2 commits January 14, 2018 22:46

fix(templates/crate/version): Change aria-label to Crate metadata

8d2157a

fix(styles/home): Update custom home crate style from div to section

e7a8209

carols10cents requested changes Jan 17, 2018

View reviewed changes

fix(themes/application): Invalid markup

0682d78

carols10cents approved these changes Jan 17, 2018

View reviewed changes

bors-voyager bot merged commit 0682d78 into rust-lang:master Jan 17, 2018

sorin-davidoi deleted the small-fixes branch January 17, 2018 20:24

carols10cents mentioned this pull request Dec 12, 2019

Try to figure out some percy problems #1948

Closed

		@@ -1,4 +1,4 @@
		<ul>
		<ol>

Conversation

sorin-davidoi commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorin-davidoi commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carols10cents commented Jan 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carols10cents Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carols10cents left a comment

Choose a reason for hiding this comment

Uh oh!

carols10cents left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carols10cents left a comment

Choose a reason for hiding this comment

Uh oh!

carols10cents commented Jan 17, 2018

Uh oh!

bors-voyager bot commented Jan 17, 2018

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

carols10cents Jan 14, 2018 •

edited

Loading