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.

Sign up for GitHub

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

Jump to bottom

Screen reader accessibility #1760

Merged
merged 11 commits into from
Sep 13, 2023
Merged

Screen reader accessibility #1760

merged 11 commits into from
Sep 13, 2023

Conversation

angelikatyborska
Copy link
Contributor

@angelikatyborska angelikatyborska commented Sep 6, 2023 •
edited
Loading

Resolves #1756

Task list:

  • Missing landmark elements: <nav>, <main> ✅ c9d58ce
  • Expand menu button is missing aria-controls and aria-expanded properties ✅ 132ccb2
  • Search input: <p> is not a valid child of <label> ✅ 17adec5
  • Version select:
    • Lacks a label. ✅ 86194b2
    • Decorative character "\25bc" in version select is being read as "down pointing black pointer". ✅ 86194b2
  • Expand buttons in navigation links are missing aria-controls and aria-expanded properties. ✅ b65fe77
  • Consider using aria-current for links visually styled as "active". ✅ 56c0cf1
  • "Pages / modules / mix tasks" lack feedback after clicking, rewrite using buttons and role=tablist, use aria-selected ✅ c868c2a
  • Settings modal:
    • Close button lacks a label, being read as "times button". ✅ e4e8953
    • Theme select lacks a label. ✅ I was mistaken here, it has a label already
  • Copy button for code snippets:
    • Lacks aural feedback after successful copying, consider aria-live on the "copied!" text. ✅ 7be1ee0
  • Search HexDocs modal: close button lacks a label, being read as "times button". ✅ e4e8953
  • Consider a "skip to main content" button to quickly bypass the navigation entirely. ✅ After a second thought, I decided it's not necessary since the first element on the page is a button that closes the whole sidebar, which allows you to navigate to main content fast

angelikatyborska
angelikatyborska commented Sep 13, 2023
View reviewed changes
test/ex_doc/formatter/html/templates_test.exs
@@ -242,9 +242,16 @@ defmodule ExDoc.Formatter.HTML.TemplatesTest do
tasks: []
})

assert content =~ ~r{<li><a id="modules-list-link" href="#full-list">Modules</a></li>}
refute content =~ ~r{<li><a id="exceptions-list" href="#full-list">Exceptions</a></li>}
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 am not sure what this is about. I didn't see "exceptions" as a possible tab. It's not in SIDEBAR_NAV_TYPES.

Copy link
Member

Choose a reason for hiding this comment

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

It was a very old tab, it no longer exists. :)

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'll remove this assertion then

angelikatyborska
angelikatyborska commented Sep 13, 2023
View reviewed changes
assets/js/sidebar/sidebar-list.js
Comment on lines +19 to +21
SIDEBAR_TAB_TYPES.forEach(type => {
renderSidebarNodeList(getSidebarNodes(), type)
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sidebar tabs were rewritten to have separate ul elements for each tab. I was following https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/tab_role#example as an example of a correctly implemented tab list, and each tab needs to have a different "tabpanel" component assigned to it. Hence multiple lists were necessary.

angelikatyborska
angelikatyborska commented Sep 13, 2023
View reviewed changes
assets/css/copy-button.css
opacity: 1;
color: var(--success);
}
.copy-button.clicked::after {
content: "Copied! \2713";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to the DOM so it can be inside an aria-live element

angelikatyborska
angelikatyborska commented Sep 13, 2023
View reviewed changes
assets/css/sidebar.css
position: absolute;
left: 0;
top: 2px;
content: "\25bc";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to the DOM so it can be hidden from screen readers

@angelikatyborska angelikatyborska marked this pull request as ready for review September 13, 2023 08:40
@angelikatyborska
Copy link
Contributor Author

I'm not entirely sure if I tested all the possible variants of the components that I'm touching. I tested the "default" state you get with mix build in Firefox, Chrome, and Safari. And I tested what happens when there is only the "extras" tab and no other tabs.

angelikatyborska
angelikatyborska commented Sep 13, 2023
View reviewed changes
lib/ex_doc/formatter/html/templates/sidebar_template.eex
<% end %>
</ul>
</div>

<ul id="full-list"></ul>
</section>
<ul id="extras-full-list" class="full-list" role="tabpanel" aria-labelledby="extras-list-tab-button"></ul>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

HTML validator says role tabpanel is not allowed on list, I need to wrap them in divs

Copy link
Member

Choose a reason for hiding this comment

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

I will go ahead and merge this one, because some changes in this PR will unblock further work, but a follow up PR is welcome. :)

@josevalim josevalim merged commit d334dee into elixir-lang:main Sep 13, 2023
4 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@angelikatyborska angelikatyborska mentioned this pull request Sep 13, 2023
Merged
@angelikatyborska angelikatyborska deleted the screen-reader-accessibility branch September 13, 2023 12:22
@zachallaun zachallaun mentioned this pull request Sep 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josevalim josevalim josevalim left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Screen reader accessibility
2 participants
@angelikatyborska @josevalim