♿️(frontend) improve keyboard tab order in document list#2325
Draft
Ovgodd wants to merge 1 commit into
Draft
Conversation
Ovgodd
force-pushed
the
fix/2308-all-doc-list-a11y
branch
from
94ce8e0 to
cf88efb
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThis PR centralizes aria-label and date formatting for DocsGridItem via new hooks (useDateToDisplay, useDocItemAriaLabel), applies the combined aria-label to the primary link, marks secondary link/date rendering as aria-hidden, and removes the old helper. It also adds tabIndex={-1} to the share button and draggable root, updates four locale translations for the new label, updates e2e selectors to match document titles with regex, and adds a changelog entry. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts`:
- Line 243: The test builds a RegExp from a dynamic title (getByRole('link', {
name: new RegExp(titleDoc2) })) which breaks when titles contain regex
metacharacters; update the selector to escape the title before creating the
RegExp (e.g., add or use an escapeRegExp helper and call new
RegExp(escapeRegExp(titleDoc2))) and apply the same change to the other
occurrence (the similar getByRole call around the second reported location).
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-trashbin.spec.ts`:
- Line 87: The locator uses new RegExp(title2) which breaks for titles
containing regex metacharacters; update the call that uses
row2Restored.getByRole('link', { name: new RegExp(title2) }) to pass an escaped
literal pattern instead (e.g., escape title2 with a utility that replaces all
regex metacharacters before constructing RegExp) or switch to an exact string
matcher (pass the plain title2 as the name) so matching is deterministic; target
the row2Restored call and the title2 value when applying the escape or changing
the matcher.
In
`@src/frontend/apps/impress/src/features/docs/docs-grid/components/DocsGridItem.tsx`:
- Line 126: The date anchor in DocsGridItem uses StyledLink with tabIndex={-1}
and aria-hidden="true", which hides an interactive link from assistive tech;
remove the aria-hidden attribute from the StyledLink and either (a) make the
date a normal accessible link by keeping the StyledLink without
aria-hidden/tabIndex changes, or (b) if the date should not be a separate
target, restructure the JSX in DocsGridItem so the title and date are wrapped by
a single StyledLink (ensuring only one real navigation anchor exists) and remove
the extra StyledLink for the date to preserve keyboard and screen-reader
accessibility.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: bb0a5aba-9928-4d8e-b8a7-c1c0a575f006
📒 Files selected for processing (6)
src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.tssrc/frontend/apps/e2e/__tests__/app-impress/doc-trashbin.spec.tssrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocsGridItem.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocsGridItemSharedButton.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/Draggable.tsxsrc/frontend/apps/impress/src/i18n/translations.json
| await expect(docsGrid.getByText(titleDoc1)).toBeHidden(); | ||
| await docsGrid | ||
| .getByRole('link', { name: `Open document ${titleDoc2}` }) | ||
| .getByRole('link', { name: new RegExp(titleDoc2) }) |
There was a problem hiding this comment.
Escape dynamic titles before building RegExp selectors.
Using new RegExp(titleDoc2) directly can break matching when the title contains regex special chars (()[]+?.*), making tests flaky.
💡 Proposed fix
+ const escapedTitleDoc2 = titleDoc2.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
await docsGrid
- .getByRole('link', { name: new RegExp(titleDoc2) })
+ .getByRole('link', { name: new RegExp(escapedTitleDoc2) })
.click();Also applies to: 387-387
🤖 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 `@src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts` at line
243, The test builds a RegExp from a dynamic title (getByRole('link', { name:
new RegExp(titleDoc2) })) which breaks when titles contain regex metacharacters;
update the selector to escape the title before creating the RegExp (e.g., add or
use an escapeRegExp helper and call new RegExp(escapeRegExp(titleDoc2))) and
apply the same change to the other occurrence (the similar getByRole call around
the second reported location).
| const row2Restored = await getGridRow(page, title2); | ||
| await expect(row2Restored.getByText(title2)).toBeVisible(); | ||
| await row2Restored.getByRole('link', { name: /Open document/ }).click(); | ||
| await row2Restored.getByRole('link', { name: new RegExp(title2) }).click(); |
There was a problem hiding this comment.
Escape title2 before using it in RegExp.
new RegExp(title2) can misbehave for titles containing regex metacharacters, causing false positives/negatives in locator matching.
💡 Proposed fix
+ const escapedTitle2 = title2.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
- await row2Restored.getByRole('link', { name: new RegExp(title2) }).click();
+ await row2Restored
+ .getByRole('link', { name: new RegExp(escapedTitle2) })
+ .click();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await row2Restored.getByRole('link', { name: new RegExp(title2) }).click(); | |
| const escapedTitle2 = title2.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| await row2Restored | |
| .getByRole('link', { name: new RegExp(escapedTitle2) }) | |
| .click(); |
🤖 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 `@src/frontend/apps/e2e/__tests__/app-impress/doc-trashbin.spec.ts` at line 87,
The locator uses new RegExp(title2) which breaks for titles containing regex
metacharacters; update the call that uses row2Restored.getByRole('link', { name:
new RegExp(title2) }) to pass an escaped literal pattern instead (e.g., escape
title2 with a utility that replaces all regex metacharacters before constructing
RegExp) or switch to an exact string matcher (pass the plain title2 as the name)
so matching is deterministic; target the row2Restored call and the title2 value
when applying the escape or changing the matcher.
cf88efb to
652bce7
Compare
Contributor
|
Size Change: 0 B Total Size: 4.31 MB 📦 View Changed
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/frontend/apps/e2e/__tests__/app-impress/doc-trashbin.spec.ts (1)
87-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape
title2before passing it toRegExp.Using
new RegExp(title2)can mis-match when the title includes regex characters.💡 Proposed fix
+ const escapedTitle2 = title2.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - await row2Restored.getByRole('link', { name: new RegExp(title2) }).click(); + await row2Restored + .getByRole('link', { name: new RegExp(escapedTitle2) }) + .click();🤖 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 `@src/frontend/apps/e2e/__tests__/app-impress/doc-trashbin.spec.ts` at line 87, The test builds a RegExp from title2 which can break if title2 contains regex metacharacters; before calling row2Restored.getByRole('link', { name: new RegExp(title2) }), escape title2 by transforming all regex-special characters (e.g., using a replace with /[.*+?^${}()|[\]\\]/g -> '\\$&') into literal sequences, then construct new RegExp(escapedTitle2) (add flags if needed) so the lookup matches the literal title; refer to the variables title2 and the call row2Restored.getByRole to locate where to apply this change.src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts (1)
242-242:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape dynamic titles before creating
RegExpselectors.
new RegExp(titleDoc2)is brittle when titles contain regex metacharacters, which can make these tests flaky.💡 Proposed fix
+ const escapedTitleDoc2 = titleDoc2.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - await docsGrid.getByRole('link', { name: new RegExp(titleDoc2) }).click(); + await docsGrid + .getByRole('link', { name: new RegExp(escapedTitleDoc2) }) + .click(); ... - await docsGrid.getByRole('link', { name: new RegExp(titleDoc2) }).click(); + await docsGrid + .getByRole('link', { name: new RegExp(escapedTitleDoc2) }) + .click();Also applies to: 384-384
🤖 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 `@src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts` at line 242, The test uses new RegExp(titleDoc2) directly which breaks when titles contain regex metacharacters; escape the dynamic title before building the RegExp (or avoid RegExp altogether by passing the literal string) — e.g. create an escapedTitle from titleDoc2 using a standard escape routine (replace /[-\/\\^$*+?.()|[\]{}]/g with '\\$&') and then use new RegExp(escapedTitle) (or use docsGrid.getByRole('link', { name: escapedTitle })/the literal title) in the docsGrid.getByRole call that references titleDoc2 (also apply the same fix to the other occurrence around the second test).
🤖 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
`@src/frontend/apps/impress/src/features/docs/docs-grid/components/DocsGridItem.tsx`:
- Around line 44-52: The aria-label currently uses doc.nb_accesses_direct via
participantCount which includes the owner and thus overstates participants;
update DocsGridItem (where participantCount is defined/used) to compute
otherParticipants = Math.max(0, doc.nb_accesses_direct - 1) (or use the existing
sharedCount - 1 if sharedCount is available) and pass that otherParticipants
into the t(...) call so the label reads "shared with X participant(s)" matching
the UI semantics.
---
Duplicate comments:
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts`:
- Line 242: The test uses new RegExp(titleDoc2) directly which breaks when
titles contain regex metacharacters; escape the dynamic title before building
the RegExp (or avoid RegExp altogether by passing the literal string) — e.g.
create an escapedTitle from titleDoc2 using a standard escape routine (replace
/[-\/\\^$*+?.()|[\]{}]/g with '\\$&') and then use new RegExp(escapedTitle) (or
use docsGrid.getByRole('link', { name: escapedTitle })/the literal title) in the
docsGrid.getByRole call that references titleDoc2 (also apply the same fix to
the other occurrence around the second test).
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-trashbin.spec.ts`:
- Line 87: The test builds a RegExp from title2 which can break if title2
contains regex metacharacters; before calling row2Restored.getByRole('link', {
name: new RegExp(title2) }), escape title2 by transforming all regex-special
characters (e.g., using a replace with /[.*+?^${}()|[\]\\]/g -> '\\$&') into
literal sequences, then construct new RegExp(escapedTitle2) (add flags if
needed) so the lookup matches the literal title; refer to the variables title2
and the call row2Restored.getByRole to locate where to apply this 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: ASSERTIVE
Plan: Pro
Run ID: b9bdfb54-3e17-4d7f-9f8d-964f72966ff0
📒 Files selected for processing (7)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.tssrc/frontend/apps/e2e/__tests__/app-impress/doc-trashbin.spec.tssrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocsGridItem.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocsGridItemSharedButton.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/Draggable.tsxsrc/frontend/apps/impress/src/i18n/translations.json
Announce title, update date, and participants on document link focus.
Ovgodd
force-pushed
the
fix/2308-all-doc-list-a11y
branch
from
652bce7 to
e983807
Compare
AntoLC
reviewed
May 27, 2026
Collaborator
AntoLC
left a comment
AntoLC
left a comment
There was a problem hiding this comment.
As talk together, let's see if we can harmonize keyboard navigation between feature.
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
2 participants
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.
Purpose
Improve keyboard and screen reader navigation in the "All documents" list. Tab order was inconsistent: users had to press Tab several times before reaching the next document. Each row should expose one stop for the document item, then one for its actions.
Proposal
aria-labelaria-hidden)