Fix/accessibility-issues-in-list-component #3618
Conversation
Magnusrm
added
kind/bug
📝 WalkthroughWalkthroughAdds an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/app-components/Pagination/Pagination.tsx (2)
35-51: ARIA labels props are defined but not used (regression risk on mobile).
nextLabelAriaLabelandpreviousLabelAriaLabelexist in the type but are not destructured nor applied. When labels are hidden on mobile, prev/next buttons may lack accessible names.Apply this diff to wire them up:
export const Pagination = ({ - id, - nextLabel, - previousLabel, + id, + nextLabel, + nextLabelAriaLabel, + previousLabel, + previousLabelAriaLabel, size, compact, hideLabels, currentPage, setCurrentPage, onChange, onPageSizeChange, numberOfRows = 0, rowsPerPageOptions, rowsPerPageText, showRowsPerPageDropdown = false, pageSize, }: PaginationProps) => {
104-107: Add explicit aria-labels to prev/next buttons.Use the newly wired aria-label props so these buttons remain announced when text labels are hidden.
Apply this diff:
- <DesignSystemPagination.Button {...prevButtonProps}> + <DesignSystemPagination.Button + aria-label={previousLabelAriaLabel || previousLabel} + {...prevButtonProps} + > {!hideLabels && !isMobile && previousLabel} </DesignSystemPagination.Button> ... - <DesignSystemPagination.Button {...nextButtonProps}> + <DesignSystemPagination.Button + aria-label={nextLabelAriaLabel || nextLabel} + {...nextButtonProps} + > {!hideLabels && !isMobile && nextLabel} </DesignSystemPagination.Button>Also applies to: 122-125
src/features/instantiate/selection/InstanceSelection.tsx (1)
145-149: Missing ARIA labels on Prev/Next buttonsThe
nextLabelAriaLabelandpreviousLabelAriaLabelprops are defined in PaginationProps but never actually forwarded into theusePaginationhook (or onto the<DesignSystemPagination.Button>). As a result, whenhideLabelsistrueor on mobile (where labels are hidden), the Prev/Next buttons have noaria-label, and screen readers will announce them only as “button” without context.Please apply the following fixes in src/app-components/Pagination/Pagination.tsx ([around lines 17–25, 64, 104 & 122]):
• Include the ARIA-label props in the component’s destructuring.
• Pass them intousePagination({ … })(or directly onto the Button as an explicitaria-label).Suggested diff:
--- a/src/app-components/Pagination/Pagination.tsx +++ b/src/app-components/Pagination/Pagination.tsx @@ type PaginationProps = { nextLabel: string; - nextLabelAriaLabel: string; + nextLabelAriaLabel: string; previousLabel: string; - previousLabelAriaLabel: string; + previousLabelAriaLabel: string; size: NonNullable<Parameters<typeof DesignSystemPagination>[0]['data-size']>; @@ export const Pagination = ({ - id, - nextLabel, - previousLabel, + id, + nextLabel, + nextLabelAriaLabel, + previousLabel, + previousLabelAriaLabel, size, compact, hideLabels, @@ const { pages, prevButtonProps, nextButtonProps } = usePagination({ - currentPage, - setCurrentPage, - totalPages, - onChange, - showPages, + currentPage, + setCurrentPage, + totalPages, + onChange, + showPages, + nextLabelAriaLabel, + previousLabelAriaLabel, });If the
usePaginationAPI doesn’t accept these keys, alternatively addaria-labeldirectly:<DesignSystemPagination.Button {...prevButtonProps} aria-label={previousLabelAriaLabel} > {!hideLabels && !isMobile && previousLabel} </DesignSystemPagination.Button>src/layout/List/ListComponent.tsx (1)
279-287: Checkbox onChange is a no-op; keyboard/SR interaction won’t toggle selection. Also align visually-hidden class.
onChange={() => {}}prevents keyboard/AT users from changing state using the control itself.- Use the same
utilClasses.visuallyHiddenutility instead of'sr-only'for consistency with the table header.Apply this diff:
- label={<span className='sr-only'>{getRowLabel(row)}</span>} - onChange={() => {}} + label={<span className={utilClasses.visuallyHidden}>{getRowLabel(row)}</span>} + onChange={() => toggle(row)}Note: Keeping the row-level
onClickhandler is fine, but the control itself must toggle state viaonChangefor proper a11y.
🧹 Nitpick comments (5)
src/language/texts/en.ts (1)
381-381: Optional wording tweak for clarity/consistency.Consider “Select row” (imperative) or “Row selection” (noun) to better mirror Bokmål “Velg rad” and typical column-header wording. Current “Row selectors” is understandable but slightly less idiomatic.
Apply one of these:
- 'list_component.controlsHeader': 'Row selectors', + 'list_component.controlsHeader': 'Select row',or
- 'list_component.controlsHeader': 'Row selectors', + 'list_component.controlsHeader': 'Row selection',src/app-components/Pagination/Pagination.tsx (1)
112-114: Minor ARIA nuance: Prefer aria-current="page" instead of boolean.Spec-compliant value improves semantics for assistive tech.
Apply this diff:
- aria-current={currentPage === page} + aria-current={currentPage === page ? 'page' : undefined}src/features/instantiate/selection/InstanceSelection.tsx (1)
227-227: Tiny consistency nit: drop unnecessary unary plus.
onPageSizeChangealready receives a number; the cast is redundant and diverges from the mobile path.Apply this diff:
- onPageSizeChange={(value) => handleRowsPerPageChanged(+value)} + onPageSizeChange={(value) => handleRowsPerPageChanged(value)}src/layout/List/ListComponent.tsx (2)
211-211: Prefer indexedId for Pagination.id to guarantee uniqueness in repeating contextsUsing
baseComponentIdmay collide if the List appears in repeating groups or multiple instances.indexedIdis already computed and used for form control names; use it here too to ensure unique DOM IDs.Apply this minimal change:
- id={baseComponentId} + id={indexedId}Please verify whether List can be rendered multiple times on a page (including within repeating groups). If yes, this change prevents duplicate IDs and label collisions.
318-318: Same uniqueness concern for desktop Pagination idMirror the mobile change: prefer
indexedIdoverbaseComponentIdto avoid ID collisions if multiple Lists render on the same page.Proposed change:
- id={baseComponentId} + id={indexedId}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/app-components/Pagination/Pagination.tsx(4 hunks)src/features/instantiate/selection/InstanceSelection.tsx(2 hunks)src/language/texts/en.ts(1 hunks)src/language/texts/nb.ts(1 hunks)src/language/texts/nn.ts(1 hunks)src/layout/List/ListComponent.tsx(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/language/texts/nn.tssrc/features/instantiate/selection/InstanceSelection.tsxsrc/language/texts/nb.tssrc/language/texts/en.tssrc/app-components/Pagination/Pagination.tsxsrc/layout/List/ListComponent.tsx
🧬 Code graph analysis (1)
src/layout/List/ListComponent.tsx (1)
src/features/language/Lang.tsx (1)
Lang(15-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (11)
src/language/texts/nb.ts (1)
386-386: LGTM: New Bokmål key is correct and aligns with usage.The translation “Velg rad” fits a hidden column header describing row selection controls.
src/app-components/Pagination/Pagination.tsx (2)
78-79: LGTM: Unique Select/Label association via per-instance id.Good use of
id/htmlForto avoid duplicate labels when multiple paginations exist.Also applies to: 93-94
15-20: Paginationidprop usage verified – all call sites updatedOnly one import of the local
Paginationcomponent (src/app-components/Pagination/Pagination.tsx) is used in the repo, insrc/features/instantiate/selection/InstanceSelection.tsx(line 143), and it already passesid='instance-selection'. No other call sites import the localPagination, so no additional changes are required.src/features/instantiate/selection/InstanceSelection.tsx (2)
144-158: LGTM: Stable per-instance id for Pagination (mobile).Passing
id='instance-selection'ensures unique rows-per-page label/input IDs in this view.
214-229: LGTM: Stable per-instance id for Pagination (desktop).Matches the mobile path; only one view mounts at a time, so no duplicate IDs at runtime.
src/layout/List/ListComponent.tsx (6)
29-29: Import of utilClasses looks goodThis will be used to apply visually-hidden styles in the table header. Consider using the same utility for the checkbox label later for consistency (see comment on Lines 279-287).
245-249: Visually hidden controls header: good accessibility improvementProviding an explicit, visually hidden header for the selection column addresses “empty header” complaints from AT while keeping the UI clean.
291-293: RadioButton label/hideLabel pattern is correct; ensuregetRowLabelis localizedUsing
hideLabelwith a descriptive label is the right approach. OncegetRowLabelis localized (see Lines 127-137), this will read correctly for screen readers.Would you confirm that
getRowLabelnow returns localized text in your manual SR tests after applying the earlier change?
332-339: Requiringidon PaginationProps is reasonableMaking
idmandatory clarifies intent and helps enforce unique control IDs for pagination UI pieces (e.g., rows-per-page). No issues spotted.
342-349: Destructuringidin Pagination is fineNo functional issues. This aligns with forwarding the identifier to the underlying pagination control.
360-374: CustomPaginationidusage and translations verified
- The
idprop is consumed in Pagination.tsx to namespace the rows-per-page dropdown:
- Select element has
id={paginationRowsPerPageDropdown-${id}}- Corresponding
<Label>useshtmlFor={paginationRowsPerPageDropdown-${id}}- ARIA label props are correctly forwarded:
previousLabelAriaLabelandnextLabelAriaLabeldrive the buttonaria-labelviaprevButtonProps/nextButtonProps- All relevant translation keys are present in
en.ts,nb.ts, andnn.ts:
- list_component.controlsHeader
- list_component.nextPage / nextPageAriaLabel
- list_component.previousPage / previousPageAriaLabel
- list_component.rowsPerPage
No further changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/layout/List/ListComponent.tsx (1)
286-297: Desktop checkbox is controlled but has a no-op onChange — keyboard users can’t toggle selectionWith
checked={isChecked(row)}andonChange={() => {}}, pressing Space on the checkbox won’t update selection. Relying on rowonClickwon’t help screen reader/keyboard activation. Also, preferutilClasses.visuallyHiddenover'sr-only'for consistency.Apply this diff to make the control operable and consistent:
- <Checkbox - className={classes.toggleControl} - label={<span className='sr-only'>{getRowLabel(row)}</span>} - onChange={() => {}} - value={JSON.stringify(row)} - checked={isChecked(row)} - name={indexedId} - /> + <Checkbox + className={classes.toggleControl} + label={<span className={utilClasses.visuallyHidden}>{getRowLabel(row)}</span>} + onChange={() => toggle(row)} + onClick={(e) => e.stopPropagation()} + value={JSON.stringify(row)} + checked={isChecked(row)} + name={indexedId} + />Additionally, to avoid double toggles when clicking inside the control cell, consider stopping propagation at the cell level:
// Outside selected lines, suggested for the Table.Cell that wraps the toggle <Table.Cell onClick={(e) => e.stopPropagation()} className={cn({ [classes.selectedRowCell]: isRowSelected(row) })} > … </Table.Cell>
♻️ Duplicate comments (1)
src/layout/List/ListComponent.tsx (1)
40-40: Good: localizing SR labels via useLanguage().Introducing
langAsStringhere enables translated, human-readable labels for assistive tech. This addresses the earlier request to localize row labels.
🧹 Nitpick comments (2)
src/layout/List/ListComponent.tsx (2)
29-29: Use a single visually-hidden utility consistently (prefer utilClasses.visuallyHidden over 'sr-only')You already import utilClasses and use it for the header cell. Use the same class for the checkbox label to avoid relying on an undefined global 'sr-only' class.
Apply this diff:
- label={<span className='sr-only'>{getRowLabel(row)}</span>} + label={<span className={utilClasses.visuallyHidden}>{getRowLabel(row)}</span>}
128-141: Make SR row labels deterministic and omit non-column fields; also localize booleansIterate headers in the same order as the table (keys of
tableHeaders) so SR reads a predictable sequence and you don’t include fields that aren’t displayed. Optionally map booleans to localized Yes/No.Confirm the translation keys you pick for Yes/No exist (example uses
general.yes/general.no). If different keys are used in this repo, substitute accordingly.- const getRowLabel = (row: Row): string => - Object.entries(row) - .map(([key]) => { - const headerId = tableHeaders[key]; - if (!headerId) { - return ''; - } - const header = langAsString(headerId); - const raw = row[key]; - const value = typeof raw === 'string' ? langAsString(raw) : String(raw); - return `${header}: ${value}`; - }) - .filter(Boolean) - .join(', '); + const getRowLabel = (row: Row): string => + Object.keys(tableHeaders) + .map((key) => { + const headerId = tableHeaders[key]; + if (!headerId) return ''; + const header = langAsString(headerId); + const raw = row[key]; + const value = + typeof raw === 'boolean' + ? langAsString(raw ? 'general.yes' : 'general.no') + : typeof raw === 'string' + ? langAsString(raw) + : String(raw); + return `${header}: ${value}`; + }) + .filter(Boolean) + .join(', ');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/layout/List/ListComponent.tsx(11 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/List/ListComponent.tsx
🧬 Code graph analysis (1)
src/layout/List/ListComponent.tsx (2)
src/features/language/useLanguage.ts (1)
useLanguage(90-93)src/features/language/Lang.tsx (1)
Lang(15-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
src/layout/List/ListComponent.tsx (6)
215-215: Propagating per-instance id to Pagination (mobile) is a solid fixPassing
id={baseComponentId}should eliminate duplicate ids when multiple lists are present.
322-322: Propagating per-instance id to Pagination (desktop) looks goodAligns with the mobile path and prevents duplicate control labeling when multiple lists render.
336-336: PaginationProps now carries id — good additionThis makes id propagation explicit and type-safe across the boundary.
346-346: Accepting id in Pagination component signature — LGTMMatches the updated props and enables downstream a11y/id fixes.
364-364: Forwarding id to CustomPagination — completes the fixEnsures the underlying control can generate unique ids for rows-per-page and associated labels.
249-254: ✅ Translations Verified – Approving ChangesTranslations for
list_component.controlsHeaderare present in all supported locales:
- src/language/texts/en.ts : “Row selectors”
- src/language/texts/nb.ts : “Velg rad”
- src/language/texts/nn.ts : “Velg rad”
Great job adding the visually hidden header to satisfy accessibility requirements. Everything is in place—merging!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/e2e/pageobjects/datalist.ts (2)
3-3: Make selector reusable by parameterizing the base component idHard-coding
-listComponentmakes this page object less reusable if another form uses a different instance id. Consider parameterizing the base id with a class field and interpolating it in the selector.Apply this diff locally to improve reuse:
export class Datalist { public listComponent = '#form-content-listComponent'; - public selectComponent = '#paginationRowsPerPageDropdown-listComponent'; + public readonly baseComponentId = 'listComponent'; + public selectComponent = `#paginationRowsPerPageDropdown-${this.baseComponentId}`; public tableBody = '#form-content-listComponent table tbody'; public navigateNextButton = '[data-testid=paginationNextButton]'; public navigatePreviousButton = '[data-testid=paginationPreviousButton]'; public radioButton = '[type=radio]'; public searchInput = '#searchForList'; }
3-3: Optional: prefer data-testid for stability over DOM idsIDs tend to change with structural refactors; if feasible, exposing a
data-testid="paginationRowsPerPageDropdown"on the Pagination control and selecting via[data-testid=paginationRowsPerPageDropdown]would reduce brittleness (similar to how next/prev buttons are targeted).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/e2e/pageobjects/datalist.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
test/e2e/pageobjects/datalist.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (2)
test/e2e/pageobjects/datalist.ts (2)
3-3: Selector update aligns with new per-instance Pagination IDs — LGTMMatches the new
paginationRowsPerPageDropdown-${id}scheme; using-listComponenthere is consistent with the List component instance id.
3-3: No stale#paginationRowsPerPageDropdownselectors foundI ran the suggested searches across our E2E tests and confirmed:
rg -nP '#paginationRowsPerPageDropdown(?!-)' -g 'test/e2e/**'returned no matches, so there are no lingering references to the old ID without its-listComponentsuffix.- Tests consistently use
dataListPage.selectComponent(which points to#paginationRowsPerPageDropdown-listComponent), as seen intest/e2e/integration/frontend-test/list-component.ts.No further updates are needed here.
|
There was a problem hiding this comment.
Looks good! 🙌 If #1850 should be closed when this is merged, you should probably link it as a related issue prefixed with "closing".
Description
The issues mentioned in #1850 have been tested and are no longer an issue.
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Documentation
Tests