feat(QTIEditor): add text-entry interaction support#6051
feat(QTIEditor): add text-entry interaction support#6051Abhishek-Punhani wants to merge 1 commit into
Conversation
- Add TextEntryInteraction component with support for numeric, string, and free-response question types - Create useTextEntryInteraction composable for managing text-entry state and validation - Add TextEntryInteractionDescriptor to define interaction metadata and parsing rules - Implement parse.js and validation.js utilities for text-entry specific logic - Add comprehensive test coverage for TextEntryEditor, parsing, and validation - Extend useInteractionDescriptor to support text-entry interaction type - Add math utilities for numeric answer evaluation - Expand qtiDemoData with four new demo items: numeric, text-entry, and free-response questions - Update QTIDemoPage to reference demo items from qtiDemoData module - Update QTIItemEditor to handle text-entry interaction rendering - Register TextEntryInteraction in interactions index Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
|
👋 Hi @Abhishek-Punhani, 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. |
|
📢✨ Before we assign a reviewer, we'll turn on |
🔵 Review postedLast updated: 2026-07-23 23:35 UTC |
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks @Abhishek-Punhani! This is looking fine! I have added some comments/questions, but it's looking great!
| const doc = parseXML(xml, 'text/html'); | ||
| // In html mode the document is: <html><head/><body>…content…</body></html> | ||
| // For block interactions the content IS the interaction element itself; | ||
| // for inline interactions it is the <qti-item-body> wrapper. |
There was a problem hiding this comment.
Hmm, I'm not finding any issues with this. I've been trying to replicate it, but haven't been able to. When does it happen? Do you have some error examples? In theory, all HTML content inside any QTI item should be XML-compliant, right?
| const root = doc.body.firstElementChild ?? doc.body; | ||
| const interactionEl = | ||
| descriptors.reduce((found, d) => { | ||
| if (found) return found; | ||
| // Try root itself first, then search descendants. | ||
| if (d.matches(root)) return root; | ||
| return root.querySelector(d.type) ?? null; | ||
| }, null) ?? root; | ||
|
|
||
| const desc = descriptors.find(d => d.matches(interactionEl)) ?? registry[DEFAULT_INTERACTION]; |
There was a problem hiding this comment.
In theory, the idea was for the matches implementation of the text entry interaction descriptor to implement the ".querySelector" call instead of just checking the interactionEl. That way, we can have a more general bodyXML with even multiple inline interactions (for inline choice, for example) just by passing the whole bodyXML for that interaction.
That'd mean this piece of code would remain the same:
const doc = parseXML(xml);
const interactionEl = doc.documentElement;
const desc = descriptors.find(d => d.matches(interactionEl)) ?? registry[DEFAULT_INTERACTION];
And it would be the responsibility of the TextEntryDescriptor's matches method to look for interactionEl.querySelector(...).
| /** | ||
| * @typedef {object} TextEntryAnswer | ||
| * @property {string} id - Client-side slug (not serialized to XML) | ||
| * @property {string} value - The answer value as a string. For numeric this is a | ||
| * float/int string (e.g. "12", "0.5"); for textEntry it | ||
| * is a free-form string (e.g. "Paris"). | ||
| * @property {boolean} caseSensitive - textEntry only. When true, "H2O" ≠ "h2o". | ||
| * Always false for numeric answers. | ||
| */ | ||
|
|
||
| /** | ||
| * @typedef {object} TextEntryState | ||
| * @property {string} prompt - HTML content of the question prompt; default "" | ||
| * @property {TextEntryAnswer[]} answers - Acceptable correct answers. | ||
| * Empty ([]) for freeResponse. | ||
| * @property {number} expectedLength - Value of the `expected-length` attribute. | ||
| * FREE_RESPONSE_EXPECTED_LENGTH for freeResponse; | ||
| * 0 (absent) for numeric and textEntry. | ||
| */ |
There was a problem hiding this comment.
Hmm, should this live in another module instead? Given that we are not using it here, it sounds like this should be defined somewhere else, perhaps in the parse.js module, which is the one that references these types. Also, could you do the same change for the choice interaction? 😅 We also have these types defined on its index module.
| * @param {Element} bodyEl - The `<qti-item-body>` element | ||
| * @returns {string} | ||
| */ | ||
| export function _extractPromptHTML(bodyEl) { |
There was a problem hiding this comment.
Should this be exported? 😅 Given the _ it seems more like a private function. I see this is being imported by the test suite, but we can test the entire function instead of all inner functions.
| * buildTextEntryInteractionXML wraps body content in a single `<div>`, so we | ||
| * look inside that wrapper for prompt children and the interaction container. | ||
| * | ||
| * @param {Element} bodyEl - The `<qti-item-body>` element |
There was a problem hiding this comment.
Won't always be the qti-item-body if we ever implement a multi-interaction editor; then this may be a div element. Let's just say it is the wrapper element that contains both the prompt and the text entry interaction.
| <KButton | ||
| v-if="mode === 'edit'" | ||
| appearance="flat-button" | ||
| :appearanceOverrides="addBtnOverrides" | ||
| class="add-answer-btn" | ||
| :aria-label="addAnswerBtn$()" | ||
| @click="onAddAnswer" | ||
| > | ||
| <div class="add-answer-btn-content"> | ||
| <KIcon | ||
| icon="plus" | ||
| :color="$themePalette.blue.v_500" | ||
| /> | ||
| <span>{{ addAnswerBtn$() }}</span> | ||
| </div> | ||
| </KButton> |
There was a problem hiding this comment.
Seems like this is getting common across the different interactions 😅. Could we extract it to a shared component?
| () => props.mode, | ||
| newMode => { | ||
| if (newMode === 'edit') { | ||
| if (!QTISanitizer.stripTags(state.value.prompt).trim()) { |
There was a problem hiding this comment.
I think we can just use the regular .trim we used on the ChoiceInteractionEditor, just to make it a bit less complex here.
| :class="{ 'small-screen': windowIsSmall }" | ||
| > | ||
| <div class="answer-input-wrap"> | ||
| <input |
There was a problem hiding this comment.
Could we add the max length for this input?
| <ValidationMessage | ||
| v-if="isNumeric && answerHasError(answer.id, ValidationError.INVALID_NUMERIC_VALUE)" | ||
| class="answer-validation-message" | ||
| > | ||
| {{ errorInvalidNumericValue$() }} | ||
| </ValidationMessage> | ||
|
|
||
| <ValidationMessage | ||
| v-if="!isNumeric && answerHasError(answer.id, ValidationError.EMPTY_ANSWER_CONTENT)" | ||
| class="answer-validation-message" | ||
| > | ||
| {{ errorEmptyAnswerContent$() }} | ||
| </ValidationMessage> |
There was a problem hiding this comment.
Oh, we should also check for duplicated answers here
| outline: none; | ||
|
|
||
| &::placeholder { | ||
| color: var(--placeholder-color); |
There was a problem hiding this comment.
Oh, we can also use v-bind('$themeTokens.annotation'
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6051 — the text-entry plugin is well-structured, reuses useInteraction/defineInteraction/floatOrIntRegex as specified, and is thoroughly tested. One blocking issue: the prompt is not round-trip stable, which violates the stated acceptance criterion.
Blocking
parse.jswraps the prompt in its own<div>on build, and parse reads that wrapper back into the prompt — so every save→reload cycle accretes one more<div>layer permanently (round-trip not stable). See inline.
Suggestions (inline)
- Round-trip tests assert
answers/expectedLengthbut neverprompt, so the accretion above slips past CI. parseItem.jshardcodesTEXT_ENTRYfor the inline decision instead of consultingdescriptor.placement.floatOrIntRegexaccepts non-numeric strings ("e","1e","1e2e3").- Answer
<input>renders author content withoutdir="auto"(RTL answers render LTR). - New-answer focus uses
document.getElementById+ a dynamic id rather than a template ref. - "Add acceptable answer" button text is below WCAG AA contrast (4.33:1); copied from
ChoiceInteractionEditor, so worth fixing in both.
Also worth a look (not line-anchored)
- The PR adds a third
QuestionType.TEXT_ENTRY(string base-type with a correct-response) beyond the two types issue #5979 scopes (numeric,freeResponse), pulling in case-sensitivity UI/i18n/serialization. If forward-looking, consider splitting into its own issue/PR. - AC lists
placeholder-text="Enter your answer here"for freeResponse butbuildXMLonly emitsexpected-length; the issue prose is internally inconsistent — confirm intent with the author. - Validating a freshly-added, never-touched answer shows a red error immediately (per AC, but harsh UX; consider validating on first blur).
CI passing. Manual QA confirmed all three editors render/validate/localize correctly, RTL mirrors, and layout wraps responsively.
@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
| // Build the body content: prompt HTML (if any) followed by the interaction paragraph. | ||
| const divChildren = []; | ||
| if (prompt) { | ||
| divChildren.push(buildXmlNode({ tag: 'div', innerHTML: prompt })); |
There was a problem hiding this comment.
blocking: Prompt is not round-trip stable. buildXML wraps the prompt in its own <div> here and wraps the whole body in an outer <div>. On parse, _extractPromptHTML (line 46) descends into the outer wrapper but keeps this inner <div> as part of the prompt, so parse returns <div>{prompt}</div> — one <div> deeper than what was built. Every save→reload cycle adds another layer permanently, violating the "re-parsed yields an equivalent state" acceptance criterion (the doubled <div><div><p>… nesting baked into TEXT_ENTRY_BODY_XML is the symptom). Fix: emit the prompt's nodes directly into the body <div> rather than in an extra <div>, or strip the wrapper on parse so build/parse are inverses.
| ); | ||
| const parsed = parseTextEntryInteraction(bodyXml, responseDeclarations); | ||
|
|
||
| expect(parsed.answers).toHaveLength(1); |
There was a problem hiding this comment.
suggestion: The round-trip tests assert answers and expectedLength but never prompt, so the <div>-accretion bug above passes CI unnoticed. Round-trip stability covers the whole state — add an assertion that parsed.prompt equals original.prompt (once the wrapper issue is fixed) so the invariant is actually protected.
| // Inline interactions (e.g. text-entry) embed their prompt in body | ||
| // siblings, not inside the element. Pass the full <qti-item-body> so | ||
| // parse() can recover the prompt from the surrounding context. | ||
| const isInline = el.tagName.toLowerCase() === QtiInteraction.TEXT_ENTRY; |
There was a problem hiding this comment.
suggestion: Hardcodes TEXT_ENTRY for the inline decision, though the adjacent comment says "For descriptors with placement: 'inline'" and the descriptor exposes exactly that field. Keying off descriptor.placement keeps the inline/block decision in one place so the next inline interaction works without editing parseItem.
| /** | ||
| * Matches valid numeric answer values: integers, decimals, and scientific notation. | ||
| */ | ||
| export const floatOrIntRegex = /^(?=.)([+-]?([0-9e]*)(\.([0-9e]+))?)$/; |
There was a problem hiding this comment.
suggestion: [0-9e]* treats e as an ordinary digit anywhere, so "e", "ee", "1e", and "1e2e3" all pass and get accepted as valid numeric answers (then parseFloat coerces/NaNs them downstream). If scientific notation is intended, an anchored form expresses it precisely: /^[+-]?(\d+\.?\d*|\.\d+)([eE][+-]?\d+)?$/.
There was a problem hiding this comment.
Hmm, given that this is the implementation that we already have, I think it's fine to keep it as is; if not, we may have a lot of errors because of a new regex.
There was a problem hiding this comment.
Makes sense — keeping parity with the existing implementation avoids introducing a behavioral regression here. Deferring to that.
| <div class="answer-input-wrap"> | ||
| <input | ||
| :id="`answer-input-${answer.id}`" | ||
| :value="answer.value" |
There was a problem hiding this comment.
suggestion: For the TEXT_ENTRY (string) type these are author-entered content values, but the native <input> has no direction handling, so an Arabic/Hebrew answer renders LTR. Add dir="auto" so the browser picks direction from the content (harmless for the numeric type).
| async function onAddAnswer() { | ||
| const newId = addAnswer(); | ||
| await nextTick(); | ||
| const input = document.getElementById(`answer-input-${newId}`); |
There was a problem hiding this comment.
suggestion: Focus reaches into the global DOM via document.getElementById and a dynamically-built id, which also risks collisions if two TextEntryEditor instances render on the same page. The idiomatic Vue approach is a function/array template ref on the v-for input, focusing the new row's ref.
There was a problem hiding this comment.
Yeah, this suggestion makes sense; using getElementById here feels a bit awkward.
| runValidation(); | ||
| } | ||
|
|
||
| const addBtnOverrides = computed(() => ({ |
There was a problem hiding this comment.
suggestion: addBtnOverrides renders the "Add acceptable answer" text as blue.v_500 on blue.v_50 at 14px/600, measuring ~4.33:1 — below the WCAG AA 4.5:1 threshold for normal text. It's byte-for-byte copied from ChoiceInteractionEditor.vue, so worth fixing in both (e.g. darken to blue.v_700). Also a candidate to hoist into a shared constant as more interaction editors land.
There was a problem hiding this comment.
This is fine, it's following the figma specs.
There was a problem hiding this comment.
Understood — if it matches the Figma spec then it is a deliberate design decision, deferring to that. Withdrawing the finding.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6051 — HEAD is byte-identical to my last review (d16287b0); 0 of 8 prior findings resolved, all 8 still open. The blocking round-trip bug (parse.js:192) re-verified as reproducing. One new defect surfaced in manual QA and is flagged inline below.
The 8 prior threads remain open against unchanged code — full audit trail in the collapsed block. Nothing to re-explain here; the blocking prompt round-trip issue is the gate.
CI passing.
Prior-finding status
UNADDRESSED — interactions/textEntry/parse.js:192 — Prompt not round-trip stable (blocking; re-verified reproduces)
UNADDRESSED — interactions/textEntry/tests/parse.spec.js:305 — Round-trip tests never assert prompt
UNADDRESSED — serialization/parseItem.js:94 — Inline decision hardcodes TEXT_ENTRY
UNADDRESSED — utils/math.js:8 — floatOrIntRegex accepts non-numeric strings
UNADDRESSED — interactions/textEntry/TextEntryEditor.vue:74 — Answer input lacks dir="auto"
UNADDRESSED — interactions/textEntry/TextEntryEditor.vue:335 — New-answer focus via document.getElementById
UNADDRESSED — interactions/textEntry/TextEntryEditor.vue:317 — "Add acceptable answer" fails WCAG AA contrast (4.33:1)
UNADDRESSED — QuestionType.TEXT_ENTRY scope creep beyond #5979 / placeholder-text AC inconsistency
@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
|
|
||
| const bodyEl = buildXmlNode({ | ||
| tag: 'qti-item-body', | ||
| children: [buildXmlNode({ tag: 'div', children: divChildren })], |
There was a problem hiding this comment.
blocking: buildTextEntryInteractionXML returns a full <qti-item-body> as bodyXml, and parseItem.js:97 (inline branch) likewise returns the serialized <qti-item-body>. assembleItemXml then wraps bodyXml in a fresh <qti-item-body>, so every numeric/text-entry/free-response item assembles to a nested <qti-item-body><qti-item-body>…</qti-item-body></qti-item-body> (observed in the [QTIItemEditor] assembled XML log during QA). Nested item bodies are not valid QTI 3.0 — an assessment item has exactly one body and its content model forbids nesting. Dev-only today (non-persisted demo), but this is the serialization output the PR exists to produce and would ship schema-invalid QTI once wired to persist/publish. Fix: have the inline bodyXml/buildXML contract return only the body's inner content, or have assembleItemXml skip re-wrapping when bodyXml is already a <qti-item-body>. Same root region as the prompt <div>-accretion blocker below — worth fixing together.
3 participants
Summary
Implemented the Text Entry, Numeric Interaction and Free Response editors, completing the QTI interaction authoring framework for short-answer questions.
This PR adds:
References
Closes #5979
Reviewer guidance
Navigate to the QTI demo page. Test both Text Entry and Numeric modes. Verify that adding, removing, and editing answers work and XML syncs correctly.
AI usage
Used Antigravity for final review and nitpicks.