feat(QTI): implement Text Entry Interaction viewer - #14993
Conversation
github-actions
Bot
added
DEV: renderers
|
👋 Hi @habibayman, 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. |
Build Artifacts
Smoke test screenshot |
4101bf0 to
29ad599
Compare
|
📢✨ Before we assign a reviewer, we'll turn on |
🔵 Review postedLast updated: 2026-07-29 15:19 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14993 wires the numeric keypad into QTI TextEntryInteraction, with pattern-mask validation, custom widths, and content-numeral localization. The interaction logic is strong — the transient-invalid decoupling, caret-based key handling, and the 0/negative-on-empty regression fixes are all clean and QA-verified. One blocking defect: the localization path is wired to a non-existent prop and never activates.
Blocking
QTIViewer.vue:219— theprovide('lang', …)is malformed (extra positional args to theQTI_CONTEXTprovide) and sources from an undeclared prop;langis never provided, so localized keypad glyphs and report-mode numeral redisplay are dead code. Fails two #14992 acceptance criteria. See inline.
Suggestions
- Two required test cases from the issue are missing: non-ASCII digit input normalizing to ASCII in the stored response, and report-mode localizing a stored ASCII response back to the content numeral system. These are exactly the paths the blocking bug breaks — writing them (with a non-ASCII
langprovided in the test helper) would have surfaced it. - Layout regressions confirmed by manual QA — inline block wrapper + per-input AnswerGuide banner, and width-less fields filling the container. See inline on
TextEntryInteraction.vue:4and:443. dir="auto"for the authored pattern-mask message (inline on:40).- Scope: pattern-mask validation, custom widths, and AnswerGuide integration go beyond issue #14992's keypad-wiring scope. Likely intentional given the PR title, but worth confirming they belong here vs. a separate issue.
Nitpick
TextEntryInteraction.vue:241—catch (e)swallows all errors, not just the expected coercionTypeError.
Manual QA and an axe-core audit ran against the sandbox: keypad, pattern-mask a11y wiring, RTL layout, and the regression fixes all verified working; the one axe violation is on the sandbox harness, not the component. CI pending.
@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
| }, | ||
| ), | ||
| 'lang', | ||
| computed(() => props.lang), |
There was a problem hiding this comment.
blocking: lang is never provided, so all numeral localization is dead code. Two problems: (1) these two lines are extra positional args to provide('QTI_CONTEXT', <computed>) — Vue's provide(key, value) takes only two args, so 'lang' and its computed are silently dropped; (2) even as a standalone call, props.lang is undefined because QTIViewer declares no props (it's rendered by ContentViewer with only { on: … }). Consumers inject('lang', ref(null)) fall back to null → NumericKeypad shows ASCII glyphs and localizeNumerals(raw, undefined) no-ops. The content lang is already available from useContentViewer(context) (returns lang, see packages/kolibri/composables/useContentViewer.js:209), but the destructure at line 38 omits it. Fix: destructure lang and add a separate provide('lang', lang); call. Note the test helper renderAssessmentItem never provides lang, so this is invisible to the current suite.
| > | ||
| {{ variable || placeholder }} | ||
| <div class="qti-text-entry-interaction-wrapper"> | ||
| <AnswerGuide :text="answerGuideText" /> |
There was a problem hiding this comment.
suggestion: The interactive input is now inside a .qti-text-entry-interaction-wrapper (no display rule → block) carrying a per-input AnswerGuide banner. Two QA-confirmed consequences: (1) inline fill-in-the-blank items (e.g. q20-textentry-composite, sales-tax) break onto their own line, with the banner injected mid-prose; (2) multi-blank items render one "Short answer only:" banner per input (17× in the composite fixture) — contrast choice/order interactions, which show one guide for the whole interaction. The AnswerGuide also sits outside the v-if="interactive" branch, so it renders above the read-only report value too, where the instruction is meaningless. If the guide is meant once per item, it likely belongs at the AssessmentItem level.
| .qti-text-entry-interaction { | ||
| box-sizing: border-box; | ||
| display: inline-block; | ||
| width: 100%; |
There was a problem hiding this comment.
suggestion: width: 100% on the base rule means a width-less field fills its container; for the no-qti-input-width-N case widthStyle returns only { minWidth: '20ch' }, so width: 100% is never overridden and the minWidth floor never engages (QA measured field width == container width). For inline blanks this is a regression from the old min-width:20ch / max-width:90% inline sizing. Consider giving the unset case an explicit compact width (e.g. from expected-length) rather than inheriting 100%.
| icon="error" | ||
| :color="$themeTokens.error" | ||
| /> | ||
| {{ patternMaskMessage }} |
There was a problem hiding this comment.
suggestion: patternMaskMessage is author-supplied content (data-patternmask-message), not an app string. Add dir="auto" to the wrapping <p class="qti-text-entry-interaction-error"> so an item authored in Arabic/Hebrew picks direction from its content rather than inheriting the app direction.
| inputDeclaration.value.value = candidate; | ||
| lastCommittedRaw.value = candidate; | ||
| return true; | ||
| } catch (e) { |
There was a problem hiding this comment.
nitpick: catch (e) { … } swallows every error, not just the expected coercion TypeError from mid-edit "-"/".". Narrow to if (e instanceof TypeError) and rethrow otherwise, per the project's "let errors propagate" convention.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14993 — delta re-review on the same HEAD SHA 8fd73c0a as my prior CHANGES_REQUESTED. No new commits and no replies to the open threads. 0 of 6 prior findings are resolved; all 6 remain open (inline comments and audit trail below).
The blocking lang-provide bug is unchanged — see inline. The two required test cases from #14992's Testing section (non-ASCII→ASCII normalization with a provided lang; report-mode localization) are still missing; adding them would surface that bug.
CI passing. Manual QA re-confirmed the AnswerGuide-placement issue.
@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 via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
Prior-finding status
UNADDRESSED — QTIViewer.vue:219 — malformed provide('lang', …) / undeclared props.lang (blocking)
UNADDRESSED — TextEntryInteraction.vue:4 — per-input AnswerGuide banner breaks inline fill-in-the-blank items
UNADDRESSED — TextEntryInteraction.vue:443 — width-less field fills container (width:100% never overridden)
UNADDRESSED — TextEntryInteraction.vue:33 — dir="auto" missing on authored pattern-mask message
UNADDRESSED — TextEntryInteraction.vue:241 — catch (e) swallows all errors, not just coercion TypeError
UNADDRESSED — spec — missing two required test cases (non-ASCII→ASCII normalization; report-mode localization)
| }, | ||
| ), | ||
| 'lang', | ||
| computed(() => props.lang), |
There was a problem hiding this comment.
blocking: lang is never provided, so all numeral localization is dead code. (1) These two lines are passed as the 3rd/4th positional args to provide('QTI_CONTEXT', <computed>). provide(key, value) takes only two args, so 'lang' and its computed are silently dropped; no 'lang' key is registered. (2) Even standalone it would be undefined: QTIViewer declares no props, and useContentViewer is destructured without lang. Downstream AssessmentItem.vue:78 inject('lang', ref(null)) → null, so the keypad shows ASCII glyphs and report-mode localizeNumerals(raw, null) no-ops — failing two #14992 ACs. Fix: destructure lang from useContentViewer(context) (it returns it), drop these two stray args, and add a separate provide('lang', lang);.
| > | ||
| {{ variable || placeholder }} | ||
| <div class="qti-text-entry-interaction-wrapper"> | ||
| <AnswerGuide :text="answerGuideText" /> |
There was a problem hiding this comment.
suggestion: The input is now inside a block .qti-text-entry-interaction-wrapper carrying a per-input AnswerGuide banner. QA-confirmed: inline fill-in-the-blank items break onto their own line with the banner mid-prose, and multi-blank items render one "Short answer only:" banner per input (17× in the composite fixture) — unlike choice/order interactions which show one guide per interaction. The guide also sits outside the v-if="interactive" branch, so it renders above the read-only report value. If meant once per item, it likely belongs at the AssessmentItem level.
| <p | ||
| v-if="patternErrorId" | ||
| :id="patternErrorId" | ||
| class="qti-text-entry-interaction-error" |
There was a problem hiding this comment.
suggestion: patternMaskMessage is author-supplied content (data-patternmask-message), not an app string. Add dir="auto" to this <p> so an item authored in Arabic/Hebrew picks direction from its own content rather than inheriting the app direction.
| .qti-text-entry-interaction { | ||
| box-sizing: border-box; | ||
| display: inline-block; | ||
| width: 100%; |
There was a problem hiding this comment.
suggestion: width: 100% on the base rule means a width-less field fills its container. For the no-qti-input-width-N case widthStyle returns only { minWidth: '20ch' }, so width: 100% is never overridden and the minWidth floor never engages (QA measured field width == container width). This regresses inline blanks from the old min-width:20ch / max-width:90% sizing. Consider an explicit compact width (e.g. from expected-length) for the unset case.
| inputDeclaration.value.value = candidate; | ||
| lastCommittedRaw.value = candidate; | ||
| return true; | ||
| } catch (e) { |
There was a problem hiding this comment.
nitpick: catch (e) swallows every error, not just the expected coercion TypeError from mid-edit "-"/".". Narrow to if (e instanceof TypeError) and rethrow otherwise, per the project's "let errors propagate" convention.
8fd73c0 to
1f15cbb
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14993 — delta re-review. 0 of 6 prior findings resolved; all 6 still open. The branch was force-pushed (rebased onto newer develop), but the PR's own code is byte-identical to what I reviewed last round — git diff 8fd73c0a HEAD is empty for QTIViewer.vue, TextEntryInteraction.vue, and its spec. The rebase collapsed the previous inline threads as outdated, so I've re-anchored the still-valid findings at their current line numbers. No new findings.
CI: 8 checks complete, 2 still running, none failing. Manual QA did not run this round, so nothing here is visually verified.
The blocking one remains QTIViewer.vue:218 — 'lang' and its computed are passed as the 3rd/4th arguments to a two-argument provide(), and props.lang is undefined anyway since the component declares no props option. Every numeral-localization path downstream is dead.
Prior-finding status
UNADDRESSED — kolibri/plugins/qti_viewer/frontend/components/QTIViewer.vue:218 — malformed provide('lang', …) / undeclared props.lang
UNADDRESSED — kolibri/plugins/qti_viewer/frontend/components/interactions/tests/TextEntryInteraction.spec.js — two required test cases missing (non-ASCII→ASCII normalization; report-mode localization)
UNADDRESSED — kolibri/plugins/qti_viewer/frontend/components/interactions/TextEntryInteraction.vue:4 — per-input AnswerGuide banner breaks inline fill-in-the-blank items
UNADDRESSED — kolibri/plugins/qti_viewer/frontend/components/interactions/TextEntryInteraction.vue:443 — width-less field fills its container (width: 100% never overridden)
UNADDRESSED — kolibri/plugins/qti_viewer/frontend/components/interactions/TextEntryInteraction.vue:33 — dir="auto" missing on the authored pattern-mask message
UNADDRESSED — kolibri/plugins/qti_viewer/frontend/components/interactions/TextEntryInteraction.vue:241 — catch (e) swallows all errors, not just the coercion TypeError
@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
| environmentIdentifier: __version, | ||
| }, | ||
| ), | ||
| 'lang', |
There was a problem hiding this comment.
blocking: lang is never provided, so all numeral localization is dead code. Two independent defects on these two lines:
- They are the 3rd and 4th positional arguments to
provide('QTI_CONTEXT', <computed>). Vue'sprovide(key, value)takes two arguments — these are silently dropped and no'lang'key is ever registered. - Even as a standalone call it would be
undefined: this component declares nopropsoption (grep -n props QTIViewer.vuereturns only thesetup(props, context)signature and this line), soprops.langisundefined.
Downstream, AssessmentItem.vue:105 and TextEntryInteraction.vue:117 both inject('lang', ref(null)) and get null. <NumericKeypad :lang="null"> renders ASCII glyphs, and localizeNumerals(lastCommittedRaw, undefined) at TextEntryInteraction.vue:257 no-ops — failing two #14992 acceptance criteria (localized keypad glyphs; report-mode redisplay in the content's numeral system).
The content language is already to hand: useContentViewer(context) returns lang (packages/kolibri/composables/useContentViewer.js:209), but the destructure at lines 38-47 omits it. Add lang there, drop these two stray arguments, and add a separate provide('lang', lang);.
|
|
||
| expect(input).toHaveValue('192'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
suggestion: Two of the six test cases in #14992's Testing section are still missing (grep -n "normaliz\|localiz\|lang\|numeral" over this spec returns nothing):
- Non-ASCII digit input normalizes to ASCII in the stored response.
- Report-mode display localizes a stored ASCII response back to the content's numeral system.
These are exactly the paths the provide('lang', …) bug kills. Writing them needs the test helper (components/__tests__/helpers.js:20) to accept a lang provide — which is why they would have caught it.
| > | ||
| {{ variable || placeholder }} | ||
| <div class="qti-text-entry-interaction-wrapper"> | ||
| <AnswerGuide :text="answerGuideText" /> |
There was a problem hiding this comment.
suggestion: The input is now wrapped in a block .qti-text-entry-interaction-wrapper carrying its own AnswerGuide. Last round's manual QA confirmed the consequences: inline fill-in-the-blank items break onto their own line with the banner injected mid-prose, and multi-blank items render one "Short answer only:" banner per input (17× in the q20-textentry-composite fixture) — unlike choice/order interactions, which show one guide per interaction. The guide also sits outside the v-if="interactive" branch, so it renders above the read-only report value where the instruction is meaningless. If it's meant once per item, AssessmentItem is the right level.
| .qti-text-entry-interaction { | ||
| box-sizing: border-box; | ||
| display: inline-block; | ||
| width: 100%; |
There was a problem hiding this comment.
suggestion: width: 100% makes the width-less case fill its container, and expected-length can only shrink it.
For a field with no qti-input-width-N class, widthStyle (lines 191-197) returns only { minWidth }, so this base width: 100% is never overridden and the minWidth floor never engages — QA measured field width == container width. That regresses inline blanks from the previous min-width:20ch / max-width:90% sizing.
Separately, the comment on line 193 says expected-length "just sets a floor", but Math.min(expected, DEFAULT_WIDTH_CHARS) caps it at 20 — an expected-length of 40 is discarded and only values below 20 have any effect. Either the comment or the Math.min is wrong. Giving the unset case an explicit compact width derived from expected-length would settle both points.
| <p | ||
| v-if="patternErrorId" | ||
| :id="patternErrorId" | ||
| class="qti-text-entry-interaction-error" |
There was a problem hiding this comment.
nitpick: patternMaskMessage is author-supplied content (data-patternmask-message, line 290), not an app string. Add dir="auto" to this <p> so an item authored in Arabic or Hebrew takes direction from its own content rather than inheriting the app direction.
| inputDeclaration.value.value = candidate; | ||
| lastCommittedRaw.value = candidate; | ||
| return true; | ||
| } catch (e) { |
There was a problem hiding this comment.
nitpick: catch (e) swallows every error. Only the coercion TypeError from coerceValueWithBaseType (utils/qti/values.js:210, :215) is expected here, for mid-edit "-" / ".". Narrow to if (e instanceof TypeError) { return false; } and rethrow otherwise, per the project's "let errors propagate" convention.
| }, | ||
| beforeDestroy() { | ||
| // initialize() runs on nextTick, so it may not have run yet | ||
| if (!this.sortable) { |
3 participants
Summary
This PR wires the shared
NumericKeypad(moved/generalized in #14942) into QTI'sTextEntryInteraction, and fixes what broke along the way.Changes
This PR extends the Text Entry Interaction viewer with numeric input support and several UX/robustness fixes.
It introduces a shared
NumericKeypad(viauseKeypad) that opens on focus for numeric-base-type fields, supporting digit entry, negative sign, backspace, and caret-based left/right navigation, while excluding percent/pi/fraction and vertical arrow keys. Both string and numeric fields now render astype="text"(numeric fields useinputmode="decimal"instead), letting the keypad manage caret position directly viaselectionStart/selectionEnd.It also adds
pattern-maskvalidation that flags invalid input on blur (not while typing) with an accessiblearia-invalid/aria-describedbymessage, while still recording out-of-pattern responses rather than discarding them, and gracefully ignores malformed masks.Field sizing now respects
qti-input-width-Nclasses (capped to container width) with a sensible minimum when unset, matching width in report mode.A regression fix ensures a legitimate
0response displays correctly instead of falling back to a placeholder/empty state.Smaller changes include a new "short answer only" guide string in
AnswerGuide.vue, alanginjection threaded throughAssessmentItem/QTIViewer, a guard inDragContainer.vue'sbeforeDestroyto avoid callingdestroy()on an uninitialized sortable instance, a new MathML fixture variant, and substantially expanded test coverage across all of the above.References
Reviewer guidance
http://localhost:8000/en/learn/#/qti_sandbox/q20-textentry-sv-3and try all TextEntry examplesAI usage
The keyboard wiring was done by claude in addition to the unit tests. I validated all manually and they work just fine.