Repository navigation
Render skip-to-main-content as a link instead of a button - #5610
Conversation
GauravD2t
left a comment
There was a problem hiding this comment.
Thanks @bram-atmire ! I have thoroughly tested all the steps outlined in the review instructions, and everything is working perfectly as expected.
…rowsers with JavaScript disabled
4e9d155 to
3e38c8d
Compare
alexandrevryghem
left a comment
There was a problem hiding this comment.
The previous code did not work properly in developer mode and browsers that use JavaScript because it forced a page reload. I replaced the href with a routerLink which ensures the page is not reloaded anymore and the navigation is now handled by Angular internally.
I've verified this also works correctly in prod mode when you disabled Javascript & updated the PR description
tdonohue
left a comment
There was a problem hiding this comment.
👍 Thanks @bram-atmire ! This looks good to me too.
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin dspace-8_x
git worktree add -d .worktree/backport-5610-to-dspace-8_x origin/dspace-8_x
cd .worktree/backport-5610-to-dspace-8_x
git switch --create backport-5610-to-dspace-8_x
git cherry-pick -x a24a5d6f0198a2147edc48f6f215020371a7f00d |
|
Successfully created backport PR for |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin dspace-10_x
git worktree add -d .worktree/backport-5610-to-dspace-10_x origin/dspace-10_x
cd .worktree/backport-5610-to-dspace-10_x
git switch --create backport-5610-to-dspace-10_x
git cherry-pick -x a24a5d6f0198a2147edc48f6f215020371a7f00d |
|
@bram-atmire : Somewhat strangely, this could only be backported to 9.x. So, that means it's missing from 10.x and 8.x. Could you create a backport PR minimally for 10.x against the |
References
Description
Render the skip-to-main-content control as routerLink instead of a
<button>. Aligns with the canonical WAI-ARIA Authoring Practices skip-link pattern and removes the JS click-handler dependency.Instructions for Reviewers
List of changes in this PR:
src/app/root/root.component.html- modified the skip control to becomes arouterLink. Addedtabindex="-1"to<main id="main-content">so the fragment navigation reliably moves focus across browsers (withouttabindex, some browsers do not move focus to non-form elements after fragment navigation).src/app/root/root.component.ts- removed the now-unusedskipToMainContent()method that previously focused#main-contentprogrammatically.src/app/root/root.component.spec.ts- added specs asserting (1) the skip link is an<a>, and (2) the<main>target carriestabindex="-1".How to test:
Test the following scenario in both prod mode & dev mode and try it out on at least 2 different tabs:
Tab. The screenreader should focus on the "Skip to main content"Enter. The URL should gain#main-contentand keyboard focus should land on the<main>element. A subsequentTabshould move focus to the first interactive element inside the page content rather than back to a navbar item.This is not fixing a current WCAG failure (the existing button works for any user who reaches the running SPA, since DSpace requires JS to render). It aligns the implementation with the canonical pattern - see #5605 for full rationale.
Checklist
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)root.skip-to-contentretained)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation. (n/a)