fix(qti-viewer): accessibility and maxChoices fixes for choice and text-entry interactions#14374
fix(qti-viewer): accessibility and maxChoices fixes for choice and text-entry interactions#14374rajeshuchil wants to merge 3 commits intolearningequality:developfrom
Conversation
github-actions
bot
added
DEV: renderers
|
👋 Thanks for contributing! We will assign a reviewer within the next two weeks. In the meantime, please ensure that:
We'll be in touch! 😊 |
rajeshuchil
changed the title
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Solid accessibility improvements for QTI choice and text-entry interactions — keyboard navigation, ARIA semantics, maxChoices enforcement, and the shuffle bug fix are all welcome.
CI passing. Screenshots show keyboard navigation, maxChoices enforcement, and shuffle behavior working correctly.
- blocking: Hardcoded focus outline color should use
$coreOutline(SimpleChoice.vue) - blocking:
border: 1px solidwithout color should use a theme token (TextEntryInteraction.vue) - suggestion:
lodash/isArray→Array.isArray(ChoiceInteraction.vue) - suggestion:
isArraynot needed inisSelected—getSelectionsArrayalready normalizes (ChoiceInteraction.vue) - praise: Good shuffle bug fix and keyboard navigation implementation
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
|
|
||
| // Define focus outline that matches Kolibri's standard | ||
| const coreOutline = { | ||
| outline: '3px solid rgb(51, 172, 245)', |
There was a problem hiding this comment.
blocking: This hardcodes the focus outline color as rgb(51, 172, 245). Every other component in the codebase uses the $coreOutline instance property provided by KThemePlugin — e.g., ':focus': this.$coreOutline (see CoreMenuOption.vue, CardLink.vue, TranscriptCue.vue, etc.).
Hardcoding the color means this won't respond to theme changes and could drift out of sync with the rest of the app.
Since this is a Composition API setup() component, you can access it via the component instance proxy. You're already using getCurrentInstance() in ChoiceInteraction — same pattern here:
const { proxy } = getCurrentInstance();
// then in the return:
coreOutline: proxy.$coreOutline,Or access it from the template as $coreOutline directly:
':focus': $coreOutline,|
|
||
| .qti-text-entry-interaction { | ||
| padding: 4px 8px; | ||
| border: 1px solid; |
There was a problem hiding this comment.
blocking: border: 1px solid without a color value inherits from the text color, which means it won't adapt to theme changes. Per the project convention (CLAUDE.md: "Use Theme Tokens, Not Hard-Coded Colors"), the border color should come from a theme token.
For example:
border: 1px solid var(--some-token);Or apply it dynamically via :style using $themeTokens.fineLine or $themePalette.grey.v_400 (check which other input-like elements in the codebase use). The QTISandboxPage.vue in this same plugin uses hardcoded #cccccc borders, but that's the sandbox dev tool — this is a learner-facing component.
| <script> | ||
|
|
||
| import get from 'lodash/get'; | ||
| import isArray from 'lodash/isArray'; |
There was a problem hiding this comment.
suggestion: lodash/isArray is a polyfill for Array.isArray that hasn't been necessary since IE9. The rest of the QTI viewer already uses Array.isArray (e.g., useTypedProps.js). Consider using the native method to avoid the unnecessary import.
| return false; | ||
| } | ||
| return variable.value === identifier; | ||
| return getSelectionsArray(variable.value).includes(identifier); |
There was a problem hiding this comment.
suggestion: isSelected wraps variable.value with getSelectionsArray(...).includes(identifier), but toggleSelection also calls getSelectionsArray on the same value. This is fine for correctness, but the normalization in isSelected means the earlier null/undefined check on lines 62 is partially redundant — getSelectionsArray(null).includes(x) returns false. You could simplify to just:
const isSelected = identifier => {
const variable = trackedVariable.value;
selectionVersion.value;
if (!variable) return false;
return getSelectionsArray(variable.value).includes(identifier);
};|
|
||
| const getShuffledOrder = choices => { | ||
| if (!typedProps.shuffle) { | ||
| if (!typedProps.shuffle.value) { |
There was a problem hiding this comment.
praise: Good catch fixing typedProps.shuffle → typedProps.shuffle.value. The old code was always truthy (the ref object itself), so shuffle never actually activated. Subtle bug, well spotted.
| event.preventDefault(); | ||
| focusedIndex.value = newIndex; | ||
| const listEl = event.currentTarget; | ||
| const options = listEl.querySelectorAll('[role="option"]'); |
There was a problem hiding this comment.
praise: The roving tabindex implementation with ArrowDown/Up, Home/End, and wrapping is well-structured and follows the WAI-ARIA listbox keyboard interaction pattern correctly.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Re-review after ae4cd16. All four prior findings addressed.
CI passing. Screenshots show keyboard navigation, maxChoices enforcement, and shuffle working. PR targets develop (default branch); new strings are appropriate.
Prior findings:
- Hardcoded focus outline color (SimpleChoice.vue): ACKNOWLEDGED — hardcoded
rgb()removed, now uses$themeTokens.primaryvia CSS custom property. Downgraded to suggestion below since the original theming concern is resolved, but approach differs from codebase pattern. border: 1px solidwithout color (TextEntryInteraction.vue): RESOLVED — now uses$themeTokens.fineLinelodash/isArray→Array.isArray: RESOLVED- Simplify
isSelected: RESOLVED
New findings:
- suggestion: SimpleChoice focus approach inconsistent with
$coreOutlinepattern used elsewhere (and in TextEntryInteraction in this same PR) - praise: TextEntryInteraction now correctly uses
$coreOutlineand$themeTokens.fineLine - praise: All acceptance criteria from #14347 met
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
|
|
||
| setup(props) { | ||
| const { proxy } = getCurrentInstance(); | ||
| const outlineColor = proxy.$themeTokens.primary; |
There was a problem hiding this comment.
suggestion: This uses $themeTokens.primary with a CSS custom property and :focus-visible for modality detection. It works, but it's inconsistent with the rest of the codebase — every other component uses $coreOutline via $computedClass (e.g., ChannelCard.vue, CoreMenuOption.vue, CardLink.vue). TextEntryInteraction in this same PR also uses $coreOutline.
$coreOutline differs in two ways: it uses the theme's designated outline color (not primary), and it uses JS-based input modality detection rather than :focus-visible. Consider aligning with the established pattern:
// Remove outlineColor, add to $computedClass in template:
$computedClass({
'::before': { ... },
':focus': $coreOutline,
})This would also let you remove the &:focus / &:focus:not(:focus-visible) CSS rules and the --focus-color custom property.
There was a problem hiding this comment.
I’ve reviewed the suggestion to use $coreOutline within $computedClass. While I agree that it aligns better with the codebase patterns, I've had to revert to the current implementation because the suggested approach caused a rendering regression where the choice options became invisible in the UI. Given that the current fix meets accessibility requirements and maintains UI stability
There was a problem hiding this comment.
It does indeed align with the patterns, and we should use it.
| v-if="interactive" | ||
| v-model="variable" | ||
| class="qti-text-entry-interaction" | ||
| :class="['qti-text-entry-interaction', $computedClass({ ':focus': coreOutline })]" |
There was a problem hiding this comment.
praise: Good use of $coreOutline via $computedClass here — consistent with the codebase pattern and input-modality aware.
|
|
||
| const getShuffledOrder = choices => { | ||
| if (!typedProps.shuffle) { | ||
| if (!typedProps.shuffle.value) { |
There was a problem hiding this comment.
praise: The shuffle bug fix (typedProps.shuffle → typedProps.shuffle.value) and the roving tabindex keyboard navigation are both well-implemented. The orderedIdentifiers sync logic with the initial-render vs nextTick distinction is a thoughtful solution to the Vue 2 render-loop problem.
|
Hi @rtibbles I’ve addressed the blocking issues raised in the previous review and pushed the fixes. The PR should now resolve the accessibility and maxChoices issues, I’d appreciate a review when you have time. Thanks! |
Build Artifacts
Smoke test screenshot |
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Implementation seems to have spiralled into something rather complex, and the explicit tracking of a non-reactive value is adding a lot of complexity.
I would suggest that we just don't address the reactivity issue in the scope of this PR, because it only affects the QTI Sandbox for now, not production usage.
Everything else should be simplified and pared back to what was described in the issue.
A reminder of what was there:
Add role="listbox" to the ul element in ChoiceInteraction.vue
Enforce the maxChoices limit in toggleSelection
Add tabindex="0" to SimpleChoice.vue list items
Add visible focus styles for keyboard navigation
Add an aria-label to the text entry input
Disable browser autofill with autocomplete="off"
Add basic styling for the interactive text entry input
| @@ -27,43 +52,144 @@ | |||
| return typedProps.maxChoices.value !== 1; | |||
| }); | |||
|
|
|||
| const isSelected = identifier => { | |||
| // shallowRef wrapper so computeds re-evaluate when the underlying | |||
There was a problem hiding this comment.
This feels like the wrong layer to make this change. The issue here is that the responses object is not reactively updating, so we should fix that, rather than add extra wrapping here.
| @@ -35,15 +37,31 @@ | |||
| tag: 'qti-simple-choice', | |||
|
|
|||
| setup(props) { | |||
| const { proxy } = getCurrentInstance(); | |||
There was a problem hiding this comment.
I know it gets used in other parts of the codebase, but getCurrentInstance is an anti-pattern (see reasoning from the Vue creator here: vuejs/docs#1422 (comment) ) - so I'd rather we avoid this here and below.
theme tokens are already available in this file in the module scope, so reading them off the proxy here seems like the existing code has not been read as well as it could have been.
|
|
||
| setup(props) { | ||
| const { proxy } = getCurrentInstance(); | ||
| const outlineColor = proxy.$themeTokens.primary; |
There was a problem hiding this comment.
It does indeed align with the patterns, and we should use it.
| @@ -39,6 +42,7 @@ | |||
| tag: 'qti-text-entry-interaction', | |||
|
|
|||
| setup(props) { | |||
| const { proxy } = getCurrentInstance(); | |||
There was a problem hiding this comment.
Can remove this here too and just use a direct KDS import for the value below.
| @@ -3,13 +3,16 @@ | |||
| <input | |||
| v-if="interactive" | |||
| v-model="variable" | |||
| class="qti-text-entry-interaction" | |||
| :class="['qti-text-entry-interaction', $computedClass({ ':focus': coreOutline })]" | |||
| :aria-label="`${$tr('textEntryLabel')} ${responseIdentifier}`" | |||
There was a problem hiding this comment.
We should use a placeholder in the ICU string definition directly rather than concatenating here.
| @@ -82,6 +103,15 @@ | |||
| border-radius: 8px; | |||
| transition: all 0.3s ease; | |||
|
|
|||
| &:focus { | |||
| outline: 3px solid var(--focus-color); | |||
There was a problem hiding this comment.
Noting that we currently can't use CSS variables because of our browser support - this may change in time for this code to be released, but it's important to pay attention to codebase norms for these kinds of things.
| @@ -136,6 +291,12 @@ | |||
| }, | |||
| /* eslint-enable */ | |||
| }, | |||
| $trs: { | |||
There was a problem hiding this comment.
Can avoid use of the proxy here by instead defining this string with a createTranslator object from the kolibri/utils/i18n module.
| }, | ||
| ); | ||
|
|
||
| // Roving tabindex: only one option has tabindex="0" at a time; |
There was a problem hiding this comment.
I am confused by this implementation - this is not what was specified in the issue at all. In the issue the proposal was simply to ensure that every li element had tabindex 0, but this is doing something quite different.
3 participants
Summary
Fixes accessibility and spec-compliance issues in the QTI Viewer's choice and text-entry interactions (Issue #14347).
Changes
ChoiceInteraction.vue
choices container (WCAG 1.3.1, 4.1.2)- Add aria-multiselectable="true/false" based on whether maxChoices is 1 (single-select) or not
- Enforce the maxChoices constraint before state mutation in toggleSelection, so selecting beyond the limit silently no-ops instead of over-filling
- Add a watch on maxChoices to trim excess selections when the limit changes dynamically
- Implement roving tabindex keyboard navigation ArrowDown/Up moves focus between choices.
- Fix long-standing shuffle bug: typedProps.shuffle → typyedProps.shuffle.value (the old reference was always truthy, so shuffle never actually activated)
- Use shallowRef + selectionVersion counter to force reactive re-evaluation when the responses object is replaced wholesale (Vue 2 reactivity limitation)
SimpleChoice.vue
TextEntryInteraction.vue
Manual verification
UI Evidence
Keyboard navigation
maxChoices enforcement
Shuffle behavior
References
Closes [QTI Viewer] Accessibility and spec fixes for ChoiceInteraction, SimpleChoice, and TextEntryInteraction #14347
Reviewer guidance
AI usage
I used AI assistance to help draft and refine parts of the implementation. I reviewed all generated code critically, removed/adjusted unnecessary output, and manually verified behavior in the QTI sandbox keyboard navigation, selection constraints, focus visibility, and text-entry accessibility before submission.