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

♿ [#1604] Making thumbs-up inputs tabbable #709

Merged

Conversation

jiromaykin
Copy link
Contributor

@jiromaykin jiromaykin commented Jul 4, 2023

https://taiga.maykinmedia.nl/project/open-inwoner/task/1604

to review:
NOTE: IN ORDER TO REACH the thumbs-up or thumbs-down buttons on 'tab' in the Search query page (http://127.0.0.1:8000/search/?query=gemeente ), you need to use the ARROW KEYS in order to navigate between them (= standard behaviour for radio buttons and/or check-boxes)

this PR corrects a number of accessibility errors from Lighthouse (devtools) and corrects some of the aria-labels and roles + it corrects HTML syntax: it is actually quite forbidden to put Labels inside other Labels (there's more in the entire project, but that will be solved in a different issue) + it corrects some accessibility contrast errors as well. So for documentupload widget: since all of the styling in the project for the Labels was done with a generic HTML 'Label' tag selector, all CSS styling for the DocumentUpload will have to target both the parent 'label' as well as the child 'label__label' - that's why it looks like the CSS is 'double'

Elements that are mainly affected by this PR are:

  • alt text in IMG with surrounding link
  • document upload component in Case detail page.
  • thumbs up/down in search page.
  • accessibility header Role

Done in this PR:

  • it's unnecessary to use aria-describedBy and/or title if image already has an ALT and... ariadescribedBy should always refer to an ID, not to a string, so this was wrong in the original code
  • image ALT should be the name of the site or the functionality and not contain words like 'image' or 'logo'/modern screen readers do not rely on the title attribute for conveying information, so it's best to provide a concise and meaningful ALT attribute.
  • dropdown button did not contain text, so needs aria-label and a set title attribute that explains the functionality
  • the custom file-input/document-upload widget is refactored in order to not have nested labels

The aria-label overrides anything inside the Label and other tags inside (so those will often not be read by screen-readers).

@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2023

Codecov Report

Merging #709 (887c08d) into develop (5ea47fc) will not change coverage.
The diff coverage is n/a.

❗ Current head 887c08d differs from pull request most recent head 4399ba9. Consider uploading reports for the commit 4399ba9 to get more accurate results

@@           Coverage Diff            @@
##           develop     #709   +/-   ##
========================================
  Coverage    96.20%   96.20%           
========================================
  Files          642      642           
  Lines        22934    22934           
========================================
  Hits         22064    22064           
  Misses         870      870           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jiromaykin jiromaykin force-pushed the fix/1604-A11y-2.1.1.AA-toetsenbordtoegankelijk-search-page branch 2 times, most recently from 7d54509 to e05b2a2 Compare July 10, 2023 08:41
@jiromaykin jiromaykin marked this pull request as ready for review July 10, 2023 10:13
@jiromaykin jiromaykin requested a review from pi-sigma July 10, 2023 12:01
@jiromaykin
Copy link
Contributor Author

jiromaykin commented Jul 10, 2023

@pi-sigma this one might be a bit annoying to review since most of the code is for accessibility only (like: what will be read out loud by screenreaders by "aria-labelledby" etc.) - but at least this one will be safe for merging and won't damage too much if it would be buggy.
Elements that are mainly affected by this PR are:

  • thumbs up/down in search page.
  • document upload component in Case detail page.

Accessibility references:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/complementary_role
https://html5accessibility.com/stuff/2020/11/07/not-so-short-note-on-aria-label-usage-big-table-edition/
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-label
my codepen: https://codepen.io/jirosworld/pen/XWyaLQZ

@jiromaykin jiromaykin marked this pull request as draft July 11, 2023 19:21
@jiromaykin jiromaykin force-pushed the fix/1604-A11y-2.1.1.AA-toetsenbordtoegankelijk-search-page branch 4 times, most recently from 3714e34 to 4360ebd Compare July 20, 2023 10:15
@jiromaykin jiromaykin force-pushed the fix/1604-A11y-2.1.1.AA-toetsenbordtoegankelijk-search-page branch from 887c08d to 4399ba9 Compare July 20, 2023 10:50
@jiromaykin jiromaykin marked this pull request as ready for review July 20, 2023 10:50
@jiromaykin
Copy link
Contributor Author

@pi-sigma this one took longer than expected - but it's finished now (this is the PR I retracted and put back in Draft few weeks ago but now it can be reviewed by you)

@alextreme alextreme merged commit 35933b0 into develop Aug 1, 2023
@alextreme alextreme deleted the fix/1604-A11y-2.1.1.AA-toetsenbordtoegankelijk-search-page branch August 1, 2023 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Improving accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants