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-22 15:10 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.
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.