-
Notifications
You must be signed in to change notification settings - Fork 22.4k
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(a11y): use readyState to run scripts in ARIA tab role example #29889
Conversation
Preview URLs (comment last updated: 2024-01-11 13:16:25) |
@zfox23 or @pepelsbey want to have a look at this one? |
Hey! Thank you for the fix. But it made me curious: this is a common pattern to wait for the It seems that the scope of the problem is broader than just this demo. If you get this code into a standalone index.html, it works fine. Something is going on with our demos/play system that breaks the The whole page source to try<!doctype html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Document</title>
<style>
.tabs {
padding: 1em;
}
[role="tablist"] {
margin-bottom: -1px;
}
[role="tab"] {
position: relative;
z-index: 1;
background: white;
border-radius: 5px 5px 0 0;
border: 1px solid grey;
border-bottom: 0;
padding: 0.2em;
}
[role="tab"][aria-selected="true"] {
z-index: 3;
}
[role="tabpanel"] {
position: relative;
padding: 0 0.5em 0.5em 0.7em;
border: 1px solid grey;
border-radius: 0 0 5px 5px;
background: white;
z-index: 2;
}
[role="tabpanel"]:focus {
border-color: orange;
outline: 1px solid orange;
}
</style>
<script>
document.addEventListener("DOMContentLoaded", () => {
console.log("waat");
const tabs = document.querySelectorAll('[role="tab"]');
const tabList = document.querySelector('[role="tablist"]');
// Add a click event handler to each tab
tabs.forEach((tab) => {
tab.addEventListener("click", changeTabs);
});
// Enable arrow navigation between tabs in the tab list
let tabFocus = 0;
tabList.addEventListener("keydown", (e) => {
// Move right
if (e.key === "ArrowRight" || e.key === "ArrowLeft") {
tabs[tabFocus].setAttribute("tabindex", -1);
if (e.key === "ArrowRight") {
tabFocus++;
// If we're at the end, go to the start
if (tabFocus >= tabs.length) {
tabFocus = 0;
}
// Move left
} else if (e.key === "ArrowLeft") {
tabFocus--;
// If we're at the start, move to the end
if (tabFocus < 0) {
tabFocus = tabs.length - 1;
}
}
tabs[tabFocus].setAttribute("tabindex", 0);
tabs[tabFocus].focus();
}
});
});
function changeTabs(e) {
const target = e.target;
const parent = target.parentNode;
const grandparent = parent.parentNode;
// Remove all current selected tabs
parent
.querySelectorAll('[aria-selected="true"]')
.forEach((t) => t.setAttribute("aria-selected", false));
// Set this tab as selected
target.setAttribute("aria-selected", true);
// Hide all tab panels
grandparent
.querySelectorAll('[role="tabpanel"]')
.forEach((p) => p.setAttribute("hidden", true));
// Show the selected panel
grandparent.parentNode
.querySelector(`#${target.getAttribute("aria-controls")}`)
.removeAttribute("hidden");
}
</script>
</head>
<body>
<div class="tabs">
<div role="tablist" aria-label="Sample Tabs">
<button
role="tab"
aria-selected="true"
aria-controls="panel-1"
id="tab-1"
tabindex="0"
>
First Tab
</button>
<button
role="tab"
aria-selected="false"
aria-controls="panel-2"
id="tab-2"
tabindex="-1"
>
Second Tab
</button>
<button
role="tab"
aria-selected="false"
aria-controls="panel-3"
id="tab-3"
tabindex="-1"
>
Third Tab
</button>
</div>
<div id="panel-1" role="tabpanel" tabindex="0" aria-labelledby="tab-1">
<p>Content for the first panel</p>
</div>
<div
id="panel-2"
role="tabpanel"
tabindex="0"
aria-labelledby="tab-2"
hidden
>
<p>Content for the second panel</p>
</div>
<div
id="panel-3"
role="tabpanel"
tabindex="0"
aria-labelledby="tab-3"
hidden
>
<p>Content for the third panel</p>
</div>
</div>
</body>
</html> |
@pepelsbey , I can agree on that. This pull request is more of a hack to a potentially bigger problem. Should we create an issue and test out the play system instead? |
I think (not sure though) that this is a regression since the new live sample runner got introduced. The old live sample code waited until DOMContentLoaded before running scripts, the new one doesn't, so any examples that depend on this don't work reliably any more. |
Closing this one in favor of the yari issue. Thanks! |
Description
This pull request fixes live example code for ARIA/Tab role sample code by checking whether
DOMContentLoaded
should be used as a trigger to initalize the code or not.Motivation
The sample code for ARIA/Tab role isn't working. This is caused by listening to
DOMContentLoaded
event to run the initialization script when the document has already been loaded (can be checked withdocument.readyState === 'loading'
.To fix this behavior, we can check if we should listen to the corresponding event:
Additional details
The change has been tested to be working on the following environments:
Related issues and pull requests
Fixes mdn/yari#10772