A11y: Improve page navigation#3264
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. WalkthroughAdds accessibility navigation primitives, route-driven page titles, page-title components, semantic search wrappers, and changelog notes. ChangesAccessibility and Page Title Infrastructure
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3264 +/- ##
=============================================
- Coverage 97.15% 97.11% -0.04%
Complexity 1948 1948
=============================================
Files 477 483 +6
Lines 16546 16646 +100
Branches 2407 2407
=============================================
+ Hits 16075 16166 +91
- Misses 471 480 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PILOS
 |
||||||||||||||||||||||||||||
| Project |
PILOS
|
| Branch Review |
a11y-aria-roles-landmarks
|
| Run status |
|
| Run duration | 08m 01s |
| Commit |
|
| Committer | Samuel Weirich |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
0
|
 |
0
|
 |
635
|
| View all changes introduced in this branch ↗︎ | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
resources/js/PageTitle.vue (1)
1-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
PageTitle.vueintoresources/js/componentsto match repository conventions.This is a reusable component and should live under
resources/js/components(for example,resources/js/components/PageTitle.vue) with updated imports.As per coding guidelines, "Reusable components should be stored in resources/js/components".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/PageTitle.vue` around lines 1 - 14, Move the reusable PageTitle component into resources/js/components to match the project convention, and update any imports/usages that reference PageTitle.vue so they point to the new location. Keep the component definition intact (the PageTitle.vue script setup and template) and ensure all consumers now import it from resources/js/components/PageTitle.vue.Source: Coding guidelines
resources/js/views/RouteAnnouncer.vue (1)
10-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the initial-load suppression logic.
initialLoad.value = initialLoad.value === nullis correct (it suppresses the very first announcement triggered by the initial navigation) but obscure. A named flag reads more clearly and avoids accidental breakage on future edits.♻️ Suggested refactor
-const title = ref(null); -const initialLoad = ref(null); +const title = ref(null); +const hasNavigatedOnce = ref(false); watch( () => routeStore.pageTitle, function (newPageTitle) { - initialLoad.value = initialLoad.value === null; - title.value = newPageTitle; + title.value = newPageTitle; + if (!hasNavigatedOnce.value) { + hasNavigatedOnce.value = true; + return; // skip announcing the initial page load + } }, );Then guard the span with
v-if="title && hasNavigatedOnce".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/views/RouteAnnouncer.vue` around lines 10 - 16, In RouteAnnouncer.vue, the watcher on routeStore.pageTitle uses an obscure self-referential toggle for initial-load suppression, which should be replaced with a clearly named flag. Introduce a dedicated state such as hasNavigatedOnce in the RouteAnnouncer component, update it inside the pageTitle watcher, and use it to control whether the announcement span renders. Keep the watcher and template logic aligned so the first route change is suppressed intentionally and future edits remain clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@resources/js/components/MainNav.vue`:
- Around line 308-311: The dark mode and locale menu actions in MainNav.vue
should not emit routeChanged, since that event is reserved for real navigation
and triggers unwanted focus changes via App.vue’s moveFocus(). Update the
command handlers for changeDarkMode and changeLocale so they only perform their
own action and remove the routeChanged emit from those non-navigation items.
Keep routeChanged emission only in actual route-changing commands.
In `@resources/js/components/SkipLinks.vue`:
- Around line 6-21: The SkipLinks component currently duplicates styling in the
template while the shared `.skip-links` and `.skip-link` rules are left unused
in the navigation stylesheet. Update `SkipLinks.vue` to either restore use of
the shared skip-link classes on the `<ul>` and link items, or remove the stale
CSS definitions from `app/_navigation.css` so there is only one skip-link
styling pattern. Keep the markup and the existing `SkipLinks`/skip-link symbols
aligned with whichever approach you choose.
In `@resources/js/router.js`:
- Around line 697-713: The afterEach hook in router.js is returning too early on
the initial navigation because from.path === to.path also matches START_LOCATION
for a direct visit, so the home page never calls routeStore.setPageTitle().
Update the guard in router.afterEach to special-case the initial navigation
using START_LOCATION (or equivalent) and only skip updates for real
hash/query-only route changes, while still allowing the first render of "/" to
set the title via to.meta.title.
In `@resources/js/stores/route.js`:
- Around line 19-22: The title-building logic in route.js should guard against
an undefined app name from settings.getSetting("general.name"), which can
produce titles like "Rooms - undefined" before settings load. Update the
document.title assignment in the route store so it falls back to a safe default
or omits the postfix when the app name is missing, keeping the pageTitle/postfix
handling in the same location.
---
Nitpick comments:
In `@resources/js/PageTitle.vue`:
- Around line 1-14: Move the reusable PageTitle component into
resources/js/components to match the project convention, and update any
imports/usages that reference PageTitle.vue so they point to the new location.
Keep the component definition intact (the PageTitle.vue script setup and
template) and ensure all consumers now import it from
resources/js/components/PageTitle.vue.
In `@resources/js/views/RouteAnnouncer.vue`:
- Around line 10-16: In RouteAnnouncer.vue, the watcher on routeStore.pageTitle
uses an obscure self-referential toggle for initial-load suppression, which
should be replaced with a clearly named flag. Introduce a dedicated state such
as hasNavigatedOnce in the RouteAnnouncer component, update it inside the
pageTitle watcher, and use it to control whether the announcement span renders.
Keep the watcher and template logic aligned so the first route change is
suppressed intentionally and future edits remain clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: abb4cfe5-e432-4519-9cb4-63eb00de2228
📒 Files selected for processing (32)
CHANGELOG.mdlang/en/app.phpresources/css/app/_navigation.cssresources/js/PageTitle.vueresources/js/components/App.vueresources/js/components/AppFooter.vueresources/js/components/MainNav.vueresources/js/components/RoomHeader.vueresources/js/components/RoomTabFiles.vueresources/js/components/RoomTabHistoryAttendanceButton.vueresources/js/components/RoomTabMembers.vueresources/js/components/RoomTabPersonalizedLinks.vueresources/js/components/RoomTabRecordings.vueresources/js/components/SkipLinks.vueresources/js/composables/useMenu.jsresources/js/router.jsresources/js/stores/route.jsresources/js/views/AdminLayout.vueresources/js/views/AdminRolesIndex.vueresources/js/views/AdminRoomTypesIndex.vueresources/js/views/AdminServerPoolsIndex.vueresources/js/views/AdminServersIndex.vueresources/js/views/AdminUsersIndex.vueresources/js/views/Login.vueresources/js/views/Logout.vueresources/js/views/MeetingsIndex.vueresources/js/views/NotFound.vueresources/js/views/Profile.vueresources/js/views/RoomsIndex.vueresources/js/views/RoomsView.vueresources/js/views/RouteAnnouncer.vuevite.config.common.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@resources/js/components/SkipLinks.vue`:
- Line 11: The skip-link scrolling in SkipLinks.vue always uses smooth
animation, which ignores users’ reduced-motion preference. Update the scroll
behavior in the skip-link handler that calls scrollIntoView so it checks the
user’s prefers-reduced-motion setting and uses instant scrolling when reduced
motion is requested, while keeping smooth scrolling only for users who allow it.
- Around line 49-55: The skip link item in SkipLinks.vue is using Button with
link-like behavior instead of native anchor semantics, so update the rendered
element to use PrimeVue’s anchor mode. Replace the current Button usage in the
skip link loop with an anchor-rendered Button via as="a" and an href bound to
the target id, and remove the click-only link behavior while preserving the
existing scrollAndMoveFocus focus handling through the anchor navigation flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eedd4311-97ef-40be-b78f-18399a66a94b
📒 Files selected for processing (5)
lang/en/app.phpresources/css/app/_navigation.cssresources/js/components/AppFooter.vueresources/js/components/MainNav.vueresources/js/components/SkipLinks.vue
✅ Files skipped from review due to trivial changes (1)
- resources/css/app/_navigation.css
🚧 Files skipped from review as they are similar to previous changes (3)
- lang/en/app.php
- resources/js/components/AppFooter.vue
- resources/js/components/MainNav.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@resources/js/components/SkipLinks.vue`:
- Around line 55-56: The SkipLinks anchor currently blocks native hash
navigation because `@click.stop.prevent` cancels the real `href` fallback in the
`SkipLinks.vue` template. Update the click handling on the skip link so
`scrollAndMoveFocus(link.id)` still runs, but do not prevent the anchor’s
default navigation; keep the helper in place while allowing normal hash behavior
to remain as a fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d4bfd4c7-55b5-4760-babe-aea24aec10dd
📒 Files selected for processing (1)
resources/js/components/SkipLinks.vue
samuelwei
force-pushed
the
a11y-aria-roles-landmarks
branch
from
5d0cb57 to
6491e27
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@resources/js/components/SkipLinks.vue`:
- Line 46: The SkipLinks reveal container currently uses transition-based motion
on focus, so update the SkipLinks.vue wrapper class handling to respect
reduced-motion preferences. In the skip-link container markup, keep the current
focus-within behavior but add a reduced-motion-safe variant so the reveal does
not animate when users request reduced motion. Use the existing skip-links
container element/class as the target for the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6b8f3b0b-6598-4f78-b513-a9277998475f
📒 Files selected for processing (1)
resources/js/components/SkipLinks.vue
It's not necessary for all links to be contained in a <nav> element. <nav> is intended only for a major block of navigation links; typically the <footer> element often has a list of links that don't need to be in a <nav> element. https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/nav
We cannot show an empty menu, as this throws an error in primvue when navigating with the keyboard
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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
1 participant
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.
Type
Checklist
Changes
Other information
Summary by CodeRabbit
type="search"and<search>wrappers) across key views and controls.