[KListbox] Add comprehensive Single-Select mode support#1293
[KListbox] Add comprehensive Single-Select mode support#1293Prashant-thakur77 wants to merge 5 commits into
Conversation
Prashant-thakur77
requested a deployment
to
percy_tests
GitHub Actions
Waiting
|
👋 Hi @Prashant-thakur77, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
Prashant-thakur77
requested a deployment
to
percy_tests
GitHub Actions
Waiting
Prashant-thakur77
changed the title
Prashant-thakur77
force-pushed
the
feature/klistbox-single-select
branch
from
cc81794 to
4fbbea8
Compare
Prashant-thakur77
requested a deployment
to
percy_tests
GitHub Actions
Waiting
Prashant-thakur77
force-pushed
the
feature/klistbox-single-select
branch
from
4fbbea8 to
cc3c19c
Compare
Prashant-thakur77
requested a deployment
to
percy_tests
GitHub Actions
Waiting
|
I have rebased this branch with the latest updates on the release v-5 branch :) |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Implements all stated acceptance criteria from #1292 with focused, well-scoped changes and matching unit tests. One blocking accessibility issue in the new selected-option styling.
- blocking: selected-option text/background combo fails WCAG AA contrast (inline comment).
- Same root cause also bypasses the design-token pattern used elsewhere in the file (folded into the inline comment).
CI: mixed/pending at time of review — please confirm it's green before merge.
Manual QA (live docs-page session):
- Click replaces selection instead of appending — pass
- Already-selected option doesn't toggle off — pass
- Ctrl+A is blocked in single-select mode — pass
- Keyboard focus/navigation works, no focus trap — pass
- RTL layout mirrors correctly — pass
- Mobile viewport (375px) has no overflow/clipping — pass
- Selected-option color contrast — fail, see inline comment
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| const rowStyles = computed(() => ({ | ||
| backgroundColor: themeTokens().surface, | ||
| backgroundColor: | ||
| !props.showCheckbox && isSelected.value ? 'rgba(0, 0, 0, 0.05)' : themeTokens().surface, |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: This selected-state background, combined with the $themeTokens.primary text color set two lines up (line 35), fails WCAG AA contrast. Measured via axe-core on the live single-select example: foreground #4368f5 on background rgba(0,0,0,0.05) over white (≈#f2f2f2) = 4.13:1, below the 4.5:1 required for this 16px/500-weight text.
This also hardcodes a literal color instead of using the theme tokens already imported in this file (themePalette().grey.v_100 is used for :hover right below). Routing the selected-background through a verified-AA token pair (darker background and/or a less-saturated primary shade) would fix both the contrast failure and the literal-vs-token inconsistency in one change.
There was a problem hiding this comment.
The rgba(0, 0, 0, 0.05) selected-state background is a direct replication of the existing KSelectOption pattern (KSelect/KSelectOption.vue).
kolibri-design-system/lib/KSelect/KSelectOption.vue
Lines 170 to 173 in 1b9ce48
@LianaHarris360 should we change here as the bot suggest for the compliance with WCAG AA contrast?
There was a problem hiding this comment.
Confirmed — I checked KSelectOption.vue at that commit, and it has the same combination (color: this.$themeTokens.primary on .ui-select-option text, background-color: rgba(black, 0.05) on .is-selected), so this does appear to be a pre-existing contrast gap being carried over rather than something new to this PR. That said, it doesn't change the contrast math for this file — 4.13:1 is still below the 4.5:1 AA threshold here. Deferring to @LianaHarris360 on whether to fix it now in KListboxOption or track it separately alongside KSelectOption.
There was a problem hiding this comment.
Currently it is like:
I tried 2 implementation if we decided to change it here:
1)No background, primary text + bold
The selected state is now indicated by $themeTokens.primary text color + fontWeight: 500 on a plain themeTokens().surface background.
2)grey.v_100 tint + bold
Fixed by replacing rgba(0, 0, 0, 0.05) with the themePalette().grey.v_100 token,fontWeight: 500
In this we can increase font weigth if we need that:
There was a problem hiding this comment.
I checked the single selection use case in Studio, and there is no background color when a value is selected:
Looks like it only uses the $themeTokens.primary text color with the plain themeTokens().surface background to indicate what value is selected. As for the KSelectOption contrast gap, it should be changed also, but not in this PR. Instead, I can address it in a separate issue.
There was a problem hiding this comment.
Verified — confirmed against the current branch: the selected-state background was removed, so $themeTokens.primary text now sits on a plain themeTokens().surface (#FFFFFF) instead of the tinted rgba(0,0,0,0.05) background. That's a contrast ratio of ~4.63:1, which clears the 4.5:1 AA threshold (up from 4.13:1). This also resolves the literal-color concern since the background is now just the surface token. Withdrawing this as blocking.
@LianaHarris360 agreed that the same gap in KSelectOption is out of scope here and worth tracking separately.
Prashant-thakur77
requested a deployment
to
percy_tests
GitHub Actions
Waiting
… and showcheckbox defaults to true
Percy Visual Test ResultsPercy Dashboard: View Detailed Report Environment:
Instructions for Reviewers:
|
| onMounted(() => { | ||
| listbox.registerOption({ id: optionId, value: props.value }); | ||
| if (process.env.NODE_ENV !== 'production' && props.showCheckbox && !listbox.multiple) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| "[KListboxOption] 'showCheckbox' is true but the parent KListbox is in single-select mode ('multiple: false'). Set 'showCheckbox: false' to hide the checkbox.", | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Wonder if we should have something like "showSelector" and switch between a checkbox and a radio button based on whether multiple is true or false. Is there already a use case for single select?
There was a problem hiding this comment.
There's a couple of single selection use cases in Studio. For the LanguageDropdown and the CountryField in the usersTable, there is no visual selected state displayed.
But do we need to prevent the checkbox from being displayed? In both Studio cases, showCheckbox and multiple can be set to false to achieve the same result, while still allowing it to be flexible for future implementations.
There was a problem hiding this comment.
Makes sense 😅. It's only that if we have a future implementation with a radioButton, then we will need to add a showRadioButton prop to allow showing the radio button, because reusing the showCheckbox wouldn't apply (and then we would need to add another check for showRadioButton=true and multiple=true).
| :style="{ | ||
| color: isSelected ? $themeTokens.primary : 'inherit', | ||
| fontWeight: isSelected ? '500' : 'normal', | ||
| }" |
There was a problem hiding this comment.
I wonder if KDS should be less opinionated about what the style should look like, since we can use KListbox with multiple=false for layouts like this, where the "selected" state is just a different background color.
A potential solution may be to use these styles by default and provide a prop to override them.
There was a problem hiding this comment.
Now that I saw #1293 (comment). I wonder whether the default styles should be just $themeTokens.primary, or if we should use the gray-ish background KSelect uses as well? (since KSelect is already used in many places and this would make everything feel more consistent)
Ultra nitpicky, and I think this can be decided in the future 😅, but just flagging it.
There was a problem hiding this comment.
Yes,we did go with the same styles as used in Kselect before but it fails WCAG AA contrast .You can see this in this comment. And that is why we decided to use the single select selected option with a $themeTokens.primary only
| setup() { | ||
| const classLabel = 'single-select-class'; | ||
| const listboxId = 'single-select-listbox'; | ||
| const selected = ref(['literature']); |
There was a problem hiding this comment.
So, even if it's just a single select, should we still pass an array? Not saying it's wrong, although it may potentially be a bit counterintuitive.
There was a problem hiding this comment.
Yes, the v-model value is always an array regardless of the multiple mode. This was intentional from my end as it keeps the API consistent so consumers don't need to handle different value types based on the mode.So i thought it was a valid trade-off but yes it is a bit counterintuitive.Should i update it to handle modes seperatively as it is done in Kselect array for multiple and object for single select?
There was a problem hiding this comment.
Not really, just making sure it is a thoughtful decision and it is well documented :), would be nice to have thoughts from others too.
Another example of something kind of similar is the KCheckbox v-model, if we pass it an array, it adds the value to the array, but if not, it will just unset the checkbox value to the v-model (i.e., setting value to null directly to inputValue instead of [value]/[])
Prashant-thakur77
requested a deployment
to
percy_tests
GitHub Actions
Waiting
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
1 of 2 prior findings resolved; 1 still open (see below).
- [blocking]
lib/candidate/listbox/KListboxOption/index.vue:34-37— selected-state background was removed rather than replaced, leaving an acceptance criterion unimplemented. See inline comment.
CI: pending.
Prior-finding status
RESOLVED — lib/candidate/listbox/KListboxOption/index.vue:32-40 — WCAG AA contrast failure on selected-state background (rgba(0,0,0,0.05)) (see new finding above)
RESOLVED — lib/candidate/listbox/KListbox/index.vue:300-302 — injected multiple flag not reactive — fixed via getter
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| :style="{ lineHeight: '24px' }" | ||
| :style="{ | ||
| color: isSelected ? $themeTokens.primary : 'inherit', | ||
| fontWeight: isSelected ? '500' : 'normal', |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking:
- Commit
48400dfd0deleted the flaggedrgba(0,0,0,0.05)background instead of replacing it — this block now only variescolor/fontWeight, no background on selection. - Issue [KListbox] Add comprehensive Single-Select mode support #1292 requires "blue text and gray backgrounds when selected."
- Suggest
themePalette().grey.v_100(already used for:hoverbelow) as the selected-state background — same token, already AA-safe with primary text.
There was a problem hiding this comment.
I tested grey.v_100 (#F5F5F5) actually makes the contrast worse, not better:
Primary #4368F5 on grey.v_100 #F5F5F5 → 4.25:1 which fails WCAG AA
Primary #4368F5 on white #FFFFFF → 4.63:1 passes WCAG AA
Adding the grey background drops us below the 4.5:1 threshold.
There was a problem hiding this comment.
Withdrawing this. I re-checked the contrast math myself and your numbers are right — primary on grey.v_100 is ~4.25:1 (fails AA), primary on white is ~4.63:1 (passes) — so grey.v_100 isn't a viable fix either, it'd trade one AA failure for another.
More importantly, I shouldn't have re-raised this at all: this exact background-vs-no-background question was already settled earlier in this thread — I withdrew the original blocking finding once the background was removed, and @LianaHarris360 confirmed that matches the actual Studio usage (primary text only, no background). Citing the issue's literal "gray background" wording to reopen it was a mistake on my part given that prior resolution. Apologies for the noise.
There was a problem hiding this comment.
We also have a grey.v_50. Would that work better? 😅
There was a problem hiding this comment.
I tried grey.v_50 (#FAFAFA),it is close at 4.43:1 but still just below the 4.5:1 AA threshold.😅
There was a problem hiding this comment.
Thanks both — that confirms neither grey.v_100 (4.25:1) nor grey.v_50 (4.43:1) clears the 4.5:1 AA threshold with primary text, so the no-background resolution already in place (matching actual Studio usage per @LianaHarris360) is the right call. Nothing further needed from me here.
…ulti-select modes
Prashant-thakur77
requested a deployment
to
percy_tests
GitHub Actions
Waiting
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
2 of 2 prior findings resolved; no new findings on the current diff.
CI: mostly green, Percy visual tests still WAITING. Manual QA was required but did not complete this round, so this is a COMMENT rather than an approval.
Prior-finding status
RESOLVED — lib/candidate/listbox/KListboxOption/index.vue:34-37 — selected-state background/contrast: code at this location is unchanged from the upstream/release-v5 baseline (no gray/rgba background either way) and has not regressed since it was marked resolved; out of scope for this PR's delta.
RESOLVED — lib/candidate/listbox/KListboxOption/index.vue:121-128 — showCheckbox prop docstring wording cleanup.
ACKNOWLEDGED — lib/candidate/listbox/KListbox/index.vue:300 — praise: get multiple() getter avoids snapshotting props.multiple at setup time.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
🔵 Review postedLast updated: 2026-07-03 15:57 UTC |
4 participants
Description
This PR updates
KListboxto natively support strict single-selection. This is a foundational requirement for the upcomingKMultiSelectcomponent, which needs to be able to operate in a single-select mode without checkboxes. It introduces a functionalmultiple="false"guard to the listbox and ashowCheckbox="false"visual fallback to the options.Screencast.From.2026-07-01.12-11-42.mp4
Issue addressed
Addresses #1292
Changelog
KListboxandKListboxOption.multiple="false"onKListboxandshowCheckbox="false"onKListboxOptionto enforce single selection rules (disables Ctrl+A, replaces value instead of appending) while dropping the visual checkbox for plain text rendering with selection highlighting.Steps to test
KListboxcomponent page in the local documentation.Ctrl+A"select all" keyboard shortcut is blocked.(optional) Implementation notes
At a high level, how did you implement this?
multipleprop toKListbox. When false, clicking an option replaces the array and ignoresCtrl+A.showCheckboxprop toKListboxOption. When false, theKCheckboxwrapper is removed and replaced with a plain<span class="k-listbox-option-label">.<style>block and strictly maintained the36pxminimum row height and24pxline height to ensure absolute visual parity with the checkbox rows.Testing checklist
Reviewer guidance
Comments
Note to reviewers: This branch (
feature/klistbox-single-select) was branched off of my previous PR forfeature/kmultiselect-components-input-node. Therefore, the git diff currently shows theKMultiSelectInputandKMultiSelectNodefiles as well.If my previous PR (#1289) gets merged first, I can rebase this branch to clean up the diff. Otherwise, you can focus your review on the
KListboxandKListboxOptiondirectories!