fix(list): errors following a full review of the component (#DS-5402) - #1916
Merged
Conversation
…-5402) - kbq-list-selection consumers/examples now carry an accessible name (role="listbox" needs one); kbq-optgroup gets role="group" and aria-labelledby so grouped options are announced correctly - KbqListOption.ngOnInit compares compareWith(optionValue, cvaValue), matching the documented contract and getOptionByValue - horizontal lists no longer respond to Up/Down in addition to Left/Right - list.scss token fallbacks so kbq-list-item keeps its box model outside kbq-list/kbq-list-selection - added missing JSDoc on public KbqListSelection/KbqListOption members - test fixes: shift+click test now exercises the real asymmetric fromIndex/toIndex case, dropped a near-duplicate test, added coverage for the debounced window-resize listener
|
Visit the preview URL for this PR (updated for commit 9e86538): https://koobiq-next--prs-1916-le56ibbg.web.app (expires Thu, 27 Aug 2026 10:47:33 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c |
🚨 E2E tests failedReview the report for details. 💡 Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the @koobiq/components/list package following a full component review, primarily tightening accessibility semantics, typing, and keyboard behavior, and syncing docs/examples and API snapshots accordingly.
Changes:
- Adds listbox/option ARIA semantics to
kbq-list-selection/kbq-list-option, including horizontal mode support and updated docs/examples. - Improves
KbqListSelection/KbqListOptiontyping (generics, stricter input/event types) and refines selection/disabled/resize behavior with expanded test coverage. - Refactors list styling/tokens to introduce clearer override points and state-driven caption colors.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/public_api_guard/components/list.api.md | Updates public API snapshot for new generics/types and visibility changes. |
| tools/public_api_guard/components/filter-bar.api.md | Updates API snapshot for KbqListSelection generic usage. |
| tools/cspell-locales/ru.json | Adds RU spelling whitelist entry for new terminology. |
| tools/cspell-locales/en.json | Adds EN spelling whitelist entry for new terminology. |
| packages/docs-examples/components/list/list-virtual-scroll/list-virtual-scroll-example.ts | Adds accessible name to listbox example. |
| packages/docs-examples/components/list/list-overview/list-overview-example.ts | Adds accessible name to listbox example. |
| packages/docs-examples/components/list/list-multiple-keyboard/list-multiple-keyboard-example.ts | Adds accessible name to listbox example. |
| packages/docs-examples/components/list/list-multiple-checkbox/list-multiple-checkbox-example.ts | Adds accessible name to listbox example. |
| packages/docs-examples/components/list/list-intermediate-state/list-intermediate-state-example.html | Adds aria-label and hides decorative pseudo-checkbox from a11y tree. |
| packages/docs-examples/components/list/list-groups/list-groups-example.ts | Adds accessible name to grouped listbox example. |
| packages/docs-examples/components/list/list-action-button/list-action-button-example.ts | Adds accessible name to listbox example. |
| packages/components/list/list.scss | Refactors layout/border-radius behavior; adds horizontal layout class. |
| packages/components/list/list.ru.md | Documents list/listbox accessibility behavior (RU). |
| packages/components/list/list.en.md | Documents list/listbox accessibility behavior (EN). |
| packages/components/list/list.component.ts | Clarifies non-selectable kbq-list semantics; improves typing and focus class handling. |
| packages/components/list/list.component.spec.ts | Updates focus behavior assertions and clarifies ARIA expectations for kbq-list. |
| packages/components/list/list-tokens.scss | Extends theme tokens for captions and multiple-selection states; adjusts sizing tokens. |
| packages/components/list/list-selection.component.ts | Adds ARIA semantics, horizontal navigation, resize handling, stricter typing, and behavior fixes. |
| packages/components/list/list-selection.component.spec.ts | Adds/updates tests for disabled behavior, horizontal mode, layout measurement, and a11y (incl. axe). |
| packages/components/list/list-option.html | Marks built-in pseudo-checkbox as decorative via aria-hidden. |
| packages/components/list/_list-theme.scss | Hardens state token fallbacks and refactors active-state emission. |
| packages/components/filter-bar/pipes/pipe-datetime.html | Adds accessible name to embedded listbox usage. |
| packages/components/filter-bar/pipes/pipe-date.html | Adds accessible name to embedded listbox usage. |
| packages/components/core/option/optgroup.ts | Adds role="group" and aria-labelledby for grouped option containers. |
Suppressed comments (1)
packages/components/list/list-selection.component.spec.ts:396
- This type-ahead test calls
manager.onKeydown(...)directly, which bypasses the component’s(keydown)handler and can miss regressions inKbqListSelection.onKeyDown. Prefer routing throughselectionList.componentInstance.onKeyDown(...)(or dispatching the event on the host element) so the test reflects real usage.
manager.onKeydown(createKeyboardEvent('keydown', D, undefined, 'd'));
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
580
to
584
| } else if (keyCode === PAGE_UP) { | ||
| this.keyManager.setPreviousPageItemActive(); | ||
| } else if (keyCode === PAGE_DOWN) { | ||
| this.keyManager.setNextPageItemActive(); | ||
| } |
Contributor
Author
|
/approve-snapshots |
|
🔄 Updating snapshots. |
|
🚨 Failed to update snapshots. |
`KbqListSelection` configures its `FocusKeyManager` with `.withTypeAhead()`, but `onKeyDown` handled a fixed set of keys and never forwarded the rest to `keyManager.onKeydown`, so type-ahead never fired from real keypresses. Adds the terminal fallback the other type-ahead lists already use (navbar, dropdown, select, app-switcher). The type-ahead spec drove `keyManager` directly, which is why it stayed green while the feature was dead; it now dispatches on the host element and fails without the fix.
🚨 E2E tests failedReview the report for details. 💡 Comment |
NikGurev
approved these changes
Aug 24, 2026
Contributor
Author
|
/approve-snapshots |
|
🔄 Updating snapshots. |
|
✅ Snapshots updated! |
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
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.
No description provided.