fix(a11y): accessible labels for close button, MDN icons; fix Firefox theme toggle#5206
Draft
marcoscaceres wants to merge 6 commits intomainfrom
Draft
fix(a11y): accessible labels for close button, MDN icons; fix Firefox theme toggle#5206marcoscaceres wants to merge 6 commits intomainfrom
marcoscaceres wants to merge 6 commits intomainfrom
Conversation
…s, fix Firefox theme toggle - Replace title="Close" with aria-label="Close" on modal close button (core/ui.js) - Add role="img" and aria-label to MDN engine support emoji icons (core/mdn-annotation.js) - Always add dark.css stylesheet link so fixup.js updateTheme() doesn't throw a TypeError on light-only specs, fixing Firefox theme toggle failure (#5200)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves accessibility and resolves a Firefox issue in the W3C profile by ensuring required UI elements and stylesheets are present and correctly labeled.
Changes:
- Update modal close button to use
aria-label="Close"instead of relying ontitle. - Add
role="img"+aria-labelto MDN engine-support emoji icons for screen reader compatibility. - Ensure the W3C
dark.csslink is always present (usingmedia="not all"for light-only specs) sofixup.jscan safely run theme setup without throwing (issue #5200).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/spec/w3c/style-spec.js | Adds/updates assertions around presence and media of the dark.css link. |
| tests/spec/core/ui-spec.js | Adds test ensuring modal close button has an accessible label. |
| tests/spec/core/mdn-annotation-spec.js | Updates tests to assert role="img" and aria-label on engine-support icons. |
| src/w3c/style.js | Always appends dark.css link to avoid fixup.js theme initialization crash. |
| src/core/ui.js | Replaces title with aria-label on the modal close button. |
| src/core/mdn-annotation.js | Adds accessible semantics to emoji-based engine-support indicators. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
Author
|
@copilot can you take a look at CI and fix the issue? |
Contributor
Fixed in 28dd807. I checked the failing CI run and updated the W3C style tests to target the actual Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
… link hyperHTML strips the media attribute from link elements created via template literals. Use link.disabled = true instead of media="not all" to make the dark stylesheet present but inactive for light-only specs. Also fixes optional chaining: colorScheme.content?.includes() instead of colorScheme?.content.includes() to avoid TypeError when hyperHTML creates the meta element without a standard .content property. Closes #5200
Contributor
Author
|
@copilot Can you confirm all your feedback has been addressed? Can you fix anything that was not addressed? |
Contributor
Confirmed: all prior feedback is addressed in this branch. I rechecked the a11y updates ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
title="Close"witharia-label="Close"on the modal close button (core/ui.js) —titlealone is not reliable for screen readersrole="img"andaria-labelto MDN engine support emoji icons (core/mdn-annotation.js) — emoji need explicit accessible labelsdark.csslink inw3c/style.jssofixup.js'supdateTheme()can find it on light-only specs, fixing a Firefox TypeError that prevented the theme toggle button from appearing (closes Theme toggle fails to inject in FireFox #5200)For light-only specs the link uses
media="not all"so it is present in the DOM but never applied. Dark-mode specs continue usingmedia="(prefers-color-scheme: dark)"unchanged.dark.cssis already preloaded for all specs via the resource hints, so no additional network cost.Test plan
pnpm start --browser ChromeHeadless --grep="Core - UI"— new close-button aria-label testpnpm start --browser ChromeHeadless --grep="Core - MDN Annotation"— updated engine icon test (aria-label instead of title)pnpm start --browser ChromeHeadless --grep="W3C - Style"— new light-only dark.css test + updated media assertionpnpm start --browser FirefoxHeadless— full suite