improve menu accessibility

[bjohansebas]

ref: withastro/docs#12171

[ArmandPhilippot]

Thanks! Overall this looks good to me, but we might have some unwanted changes here (redundant role, invalid HTML and a slightly different rendering because of the new button).

[ArmandPhilippot]

Thanks for the updates @bjohansebas. I think we need an additional CSS rule to keep the same rendering. I also left two other comments:

• We might need another aria-label to improve the accessibility
• I'm not sure if this is the scope of this PR, but if we remove the background in nav-links we could clean up other unused styles at the same time.

[HiDeoo]

Thanks for the amazing contribution 🙌

Based on the initial conversations, we dicussed the PR during our weekly Talking & Doc'ing public session on Discord and decided to move on with Chris's suggestion and explore a few other ideas that I'm sharing in this review.

Here are some screenshots of the changes:

	Closed	Opened
Dark
Light

[sarah11918]

Just leaving a general comment that since this is no longer a hamburger menu we should probably....

• rename Hamburger.astro to Menu.astro
• rename the hamburger class to menu (I guess is safest, to refer directly to the component and not get it mixed up with "nav")

And of course, all these will have to be updated in the instructions in the docs tutorial

[sarah11918]

@HiDeoo Did we still have opinions on this script? (will also have to be a casualty of the Great Hamburger Reckoning)

[HiDeoo]

I'm guessing we would also rename the variable to menu for consistency and that should be enough if I'm not forgetting anything.

[bjohansebas]

Now I renamed it correctly

[bjohansebas]

Well, I think I’ve made all the changes, thanks for the suggestions.

[sarah11918]

It won't let me suggest, but we want something more like this:

.dark .nav-links a {
  color: #fff;
}

.dark a {
  color: #ff9776;
}

We want to keep the nav links white (the pages) but only update regular links, because we still want links to be distinguished inside text:

It probably makes the most sense to have links be a different color than the text, so our friend orange seems like a good candidate here!

[bjohansebas]

Extreme cases, but it’s resolved with the text you mentioned on Discord.

[sarah11918]

Yeah, noticed that but it's not terrible... I think the text covers this fine.

[ArmandPhilippot]

Yeah, it seems fine. This is not the most readable, but according to Firefox accessibility tools this is still AA (5.19), so I think this is more than enough for the tutorial.

[sarah11918]

Also noting that this PR should not merge until the corresponding docs one does (which is still going to take a little while to update yet, with all these changes!) 😄

This big red X is going to help us remember (even if it doesn't actually block merging)

[ArmandPhilippot]

I've checked this version compared to the current one. Well, Firefox accessibility tool is still complaining about the theme switcher because there is no label. But the scope was the main menu, so everything looks good to me!

Noting that in addition to docs, we also need to update the content-collections branch! (edit: for those unfamiliar, we need the same changes; this branch is used for the optional unit that converts this branch to use content collections)

[bjohansebas]

Firefox accessibility tool is still complaining about the theme switcher because there is no label. But the scope was the main menu, so everything looks good to me!

I think we could do it in this PR, so we don’t have to make more changes in the near future for the same accessibility issue. WDYT?

Noting that in addition to docs, we also need to update the content-collections branch! (edit: for those unfamiliar, we need the same changes; this branch is used for the optional unit that converts this branch to use content collections)

Cool, I didn’t know that either. I’ll submit a PR once this one is approved.

[ArmandPhilippot]

This is a small change, so yes I guess it's fine if we do that here! But regarding screen readers, I honestly don't know what is best between:

• an aria-label on the <button /> (e.g. <button aria-label="Switch theme" ... />)
• a <title> right after the <svg> opening tag (e.g. <title>Theme</title>? I don't know, Switch theme as SVG title feels odd, but maybe it's fine in this case)
• (a visually hidden element - as suggested by Chris earlier - that would involve more changes)

We might need to do more researches before deciding... but I think Chris and HiDeoo are more aware of how they work, so maybe they have a better understanding of it than me.

I didn’t know that either

Yeah, I suspected it, that's why I edited. 😄 Thanks for your willingness!

[HiDeoo]

As @ArmandPhilippot mentioned, quite a few approaches are available. Some points we have gathered from the previous discussions and the last Talking & Doc'ing session:

• We'd like to keep the sun/moon icons
• We'd like to avoid if possible an extra visually-hidden CSS class

You mentioned relying on the SVG, e.g. a title element which would come with an aria-labelledby attribute on the SVG pointing to such title element, or it could be an aria-label attribute on the SVG itself, but both these approaches are usually not recommended as they are known to fail in some browser / screen reader combinations.

Considering all this, in the specific context of the tutorial, I think we should be able to rely on an aria-label attribute on the button itself and making the SVG decorative with aria-hidden="true". It may not be the best approach, but it's already better than what we currently have (nothing) and fine for the tutorial context imo.

Regarding the label itself, usually it should indicate the mode that will be activated when the button is pressed, altho in the spirit of avoiding too much new code for the tutorial, we could keep it to something generic like "Switch theme" or "Toggle theme" like mentioned.

Let me know what you all think.

[ArmandPhilippot]

Thanks HiDeoo for the great explanation, and the link! I wasn't aware aria-labelledby was still required with a <title>, MDN and the spec seemed to say to use whether one or the other and I know Firefox accessibility tool pick that title as label. But as I said I don't have much experience with screen readers... So I trust Sara Soueidan on this!

Your suggestion seems the best compromise to me! Even if it's not perfect, we don't want to overwhelm the learner which is here to learn Astro, not how to have an accessible website. 😅

[HiDeoo]

I wasn't aware aria-labelledby was still required with a <title>, MDN and the spec seemed to say to use whether one or the other and I know Firefox accessibility tool pick that title as label.

Yeah, most of the time that's what happens. Chrome used to have an old bug where it didn't for a long time and there are still some screen readers that don't pick it up (JAWS if I remember correctly, may need to double check which ones) so I tend to still use both for now.

[bjohansebas]

Yeah, I also think it's fine, and it's the way to go with HiDeoo's suggestions. I've made the changes

[yanthomasdev]

Accessibility looks good to me, tested with Chrome + Windows + NVDA. Might be worth someone checking with VoiceOver on Mac.

[delucis]

Great work @bjohansebas 👏

Just leaving some small suggestions from today’s Talking & Doc’ing call on Discord.

[sarah11918]

Just noting that we had a longer discussion on Discord about the fact that navy was practically imperceptible on the dark background.

We updated this colour to fulfill (some admittedly arbitrary) requirements:

• use a "named" colour (to keep the explanations and discussions around this colour simple)
• be accessible in light mode (to avoid an anti-pattern)
• be easily perceptible in dark mode, even if not fully accessible (see following explanation)

Reasons that we chose to sacrifice meeting accessibility requirements in dark mode:

• meeting the first two requirements listed above
• introducing dark mode is the very last lesson before the end of the tutorial (some may not go back and check, some may not care by this point)
• we will use this as a "teachable moment" - this lesson focuses on creating the functionality of theme toggling, but can also include guidance re: unintended consequences, always checking your site to see how you've changed it. etc. When we update the lesson on building the theme toggle, we introduce some .dark only CSS rules. We will further include guidance something like:

When you update your site to include dark mode, some colors used may need updating. Always check your rendered site when adding new styles and colors, and make adjustments when necessary! This can mean adding more .dark CSS style rules to display different styles in dark mode, or you may wish to update some colors so that they work equally well in both themes.

Consider using accessibility tools such as a [contrast checker](insert your fave here) when creating a set of colors for your site, or running a check on your website, for example with a browser extension, to spot any potential issues.

We can even have updating this skillColor variable for dark mode be a "Try it yourself" extension exercise, if we want to further emphasize checking and updating your site after introducing a new theme.

happy!