search

godotengine / godot-website

The code for the official Godot Engine website. A static site built using Jekyll.
https://godotengine.org
MIT License
293 stars 142 forks source link

Fix accessibility of the mobile main menu #723

Closed larslaade closed 11 months ago

larslaade commented 11 months ago

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

YuriSizov commented 11 months ago

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

larslaade commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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.

YuriSizov commented 11 months ago

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.