Conversation
| <div class="lg:flex lg:h-screen lg:overflow-hidden"> | ||
| <Sidebar v-if="!hideSidebar" /> | ||
| <div class="lg:flex-1 lg:overflow-y-auto"> | ||
| <main id="main-content" tabindex="-1" class="lg:flex-1 lg:overflow-y-auto"> |
There was a problem hiding this comment.
Nested main landmarks — several pages wrapped by this layout already declare their own <main>:
app/pages/index.vue:2wraps HeroSection/JoinLiveQuiz/PublicQuizSection in<main>app/pages/admin/index.vue:2does the same
Converting this <div> to <main> means the DOM ends up with <main><main>…</main></main>. Nested/duplicate main landmarks are an ARIA issue: screen readers announce a "main" region inside the main region, and the real content landmark becomes ambiguous.
Suggestion: keep one main landmark per page. Either:
- keep the wrapper here but change it back to a
<div>and rely on the pages' own<main>(then point the skip link at the page-level main), or - keep
<main>here and remove/demote the inner<main>in the affected pages (e.g.index.vue,admin/index.vue).
| @@ -1,4 +1,9 @@ | |||
| <template> | |||
| <a | |||
| href="#main-content" | |||
There was a problem hiding this comment.
Duplicate id="main-content" + link-target depth
-
app/components/FinalScoreBoard.vuealready hardcodesid="main-content"(lines ~320 and ~526, bothrole="main"). On scoreboard pages the DOM will contain 3 elements with the same ID — invalid HTML. The browser anchors to the first match in DOM order, and screen-reader users get multiple identical regions. Worth de-duplicating (unique IDs) so the#main-contenttarget is unambiguous. -
The target resolves to the
id="main-content"defined in the layouts, i.e. the page-content wrapper. Skips are usually most useful when they land on the actual content landmark / firsth1of the page (past any page-level header/action rows, e.g.LandingTopActions). Consider pointing the anchor at the real content heading on each page, or documenting that the layout landmark is the intended stop.
Nice work on the sr-only focus - works quite well for the feature itself
ehsaaschaudhary-itpl
left a comment
There was a problem hiding this comment.
Summary — thanks for this a11y contribution! The skip link + target pattern is correct and well-tested with the keyboard.
Left two inline suggestions:
-
Nested
<main>landmarks (onempty.vue): pages likeindex.vueandadmin/index.vuealready contain their own<main>, so the layout change creates<main><main>nesting. Pick one landmark layer per page. -
Duplicate
id="main-content"+ target depth (onapp.vue):FinalScoreBoard.vuealready usesid="main-content"(twice), so on scoreboard pages there will be 3 duplicate IDs; the anchor will jump to the first match in DOM order. De-duplicate the IDs and consider linking closer to the real content heading.
cc @nva138 — happy to help iterate on the nested-landmark cleanup if useful.
9c77615 to
0ece062
Compare
|
Sorry for the delay on this one. Both fixed. Layout is a plain Also dropped |
Fixes Issue
Addresses #309, specifically the skip-to-content link part. I left it as "Addresses" and not "Closes" since the issue lists a few other a11y tasks that aren't part of this PR.
Changes proposed
I picked the skip link since it's a small, self-contained one to start with.
Right now you have to tab through the whole nav before you get to the content, so I added a skip link as the first focusable element in app.vue. It's hidden by default with sr-only and only shows up once you tab to it.
For it to actually jump somewhere I gave the layouts a main landmark to target:
Checked it with the keyboard, Tab shows the link and Enter jumps focus into the main area.
[x] - Correct; marked as done
[X] - Correct; marked as done
[ ] - Not correct; marked as not done
-->
Check List (Check all the applicable boxes)
Screenshots
Note to reviewers