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

Fix accessibility of the mobile main menu #723

Closed
wants to merge 1 commit into from
Closed

Fix accessibility of the mobile main menu #723

wants to merge 1 commit into from

Conversation

larslaade
Copy link

@larslaade larslaade commented Oct 3, 2023

Hi, I made some changes to the header and menu to be more accessible.

  • replaced checkbox/label with a button
  • moved the toggle button into the nav landmark
  • added aria-state to the button
  • unsplitted the menu list in html (visually still splitted)
  • with no js the menu is visible

@Calinou Calinou added enhancement topic:theme Issues and PRs related to styling and scripts of the frontend labels Oct 3, 2023
@YuriSizov
Copy link
Contributor

Hey, thanks for opening a PR! Could you explain what issues is this attempting to solve?

@larslaade
Copy link
Author

Using a Checkbox is not an accessible solution for this toggle. The label is not reachable via tab-key and it gives no proper state to the user. With this PR a button is used, which is tabbable, has a state via aria-expanded attribute and is connected to the nav-list-element via aria-controls attribute. As a reference I used the disclosure pattern from W3C.

The navigation lists are now unified so a screenreader user has only go through one list. Visually it is still the same layout as before.
Furthermore there was a small html-issue with a div-element as a child for the navigation ul-element, which is now resolved.

@YuriSizov
Copy link
Contributor

Thanks for the explanation. I don't think it makes sense to add ARIA attributes sporadically. Without a systematic approach these additions won't be very useful to users who rely on them. We can just replace the label with an anchor, which is what we use for all other buttons.

It also sucks to lose interactivity when JS is disabled — something that is often done to avoid trackers and ads. I would prefer a solution that still works without JS as designed.

The navigation lists are now unified so a screenreader user has only go through one list.

I'm not sure if that's a problem. The website is split into multiple sections anyway. You still need to be able to navigate between them. These DOM elements are all encapsulated by a nav tag, they should be reachable as it is.

Furthermore there was a small html-issue with a div-element as a child for the navigation ul-element, which is now resolved.

That's worth a fix, though there may be a reason for this.

@larslaade
Copy link
Author

Phew, where should I start...

A button is the correct semantic element for this case, this does not link to anything.
It is the just needed amount of aria used here (two attributes), no extra roles or other fancy things. Just best practise. Aria is needed for the state change, which is useful for screenreader user, because the change gets announced.

When JS is disabled the button is hidden and the menu is visible, so it is usable by default, interactivity is not needed and nothing is broken.

Yes not a real problem... but two lists are just not needed for the navigation. CSS takes care for the visually split. Screenreader user have one list less to go through.

The current solutions works for you, because you see the label as a "button" and can click on it, but that is not an argument to keep it like that. CSS only solutions are nice, but they are mostly not accessible for everyone.

@YuriSizov
Copy link
Contributor

A button is the correct semantic element for this case, this does not link to anything.
It is the just needed amount of aria used here (two attributes), no extra roles or other fancy things. Just best practise. Aria is needed for the state change, which is useful for screenreader user, because the change gets announced.

My point is that we don't use buttons for... well, buttons. We use anchors. Throughout the website. This change would create an inconsistency, and inconsistencies are not friends to accessibility. This argument has a bigger implication. Accessibility should not be bolted on. Pages need to be designed with it in mind.

I don't know what your commitment here is. Are you interested in this one fix? Do you plan to continue? If not, then this is where ARIA-related improvements will die and then we're better off without them. I think making pages more accessible is a worthy goal, but it should be done as a system, not as a drive-by contribution. What goals do we want to achieve? What practices should we incorporate? How should we use semantic layouts?

Without such a plan I'm not confident that what you propose is correct and future-proof, that it achieves what it sets to achieve. How should we test this, for instance? How should we ensure we don't break it in future? This is something that most developers won't notice when making changes, so how should we ensure that this doesn't happen?

If you make this change and we have no system, no guidelines, no safeguards, then it can easily be lost to some other changes in the future.

And in that case, I would prefer a simpler solution to the "can't focus menu toggle" problem.

When JS is disabled the button is hidden and the menu is visible, so it is usable by default, interactivity is not needed and nothing is broken.

I mean, this is visually broken then. It's functional, but it's not presented how it was designed.

@larslaade
Copy link
Author

Oh well, you lost me here... I was motivated to do even more for this website, but argue against this is impossible.

I wish you the best.

@larslaade larslaade closed this Oct 17, 2023
@larslaade larslaade deleted the feature/a11y-mobilenav branch October 17, 2023 09:21
@YuriSizov
Copy link
Contributor

I'm really sorry about that. I didn't want to pressure you into arguing against anything. I was merely pointing out that we need a systemic approach to accessibility rather than a one-off fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived enhancement topic:theme Issues and PRs related to styling and scripts of the frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants