feat(QTIEditor): add text-entry interaction support - #6051
feat(QTIEditor): add text-entry interaction support#6051Abhishek-Punhani wants to merge 3 commits into
Conversation
|
👋 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 |
⚪ Queued for reviewLast updated: 2026-07-30 08:33 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?
There was a problem hiding this comment.
Yeah, we can change it back.
| 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.
✅ Resolved — addressed in the current code.
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.
✅ Resolved — addressed in the current code.
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.
✅ Resolved — addressed in the current code.
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.
| <div class="answer-input-wrap"> | ||
| <input | ||
| :id="`answer-input-${answer.id}`" | ||
| :value="answer.value" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
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.
✅ Resolved — addressed in the current code.
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.
✅ Resolved — addressed in the current code.
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.
✅ Resolved — addressed in the current code.
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.
- 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>
…xt-entry parsing robustness Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
d16287b to
942cf81
Compare
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks @Abhishek-Punhani! Found a couple of errors, and a couple of questions. Also found some comments that are usually left by LLMs that add no value (like duplicated describe comments on test suites), a friendly reminder to be careful with unsupervised LLM output! :)
| // Regression: inline placement — bodyXml is a full <qti-item-body> | ||
| // Before the fix, documentElement was <qti-item-body> and no descriptor | ||
| // matched it, so the code fell back to the choice descriptor for every | ||
| // text-entry item, showing them as "Multiple Choice / Single Choice". |
There was a problem hiding this comment.
Is this comment block needed? 👀
| const { result } = renderDescriptor(TEXT_ENTRY_BODY_XML, [TEXT_ENTRY_FREE_DECL_XML]); | ||
| await nextTick(); | ||
| expect(result.descriptor.value.type).not.toBe(QtiInteraction.CHOICE); | ||
| }); |
There was a problem hiding this comment.
hmm Im not getting it, is this because of a historic bug that does not exist anymore?
| const qt = ref(QuestionType.NUMERIC); | ||
| return { qt, ...useTextEntryInteraction(makeNumericBlock(answerValues), qt) }; | ||
| } | ||
|
|
||
| function setupFree() { | ||
| const qt = ref(QuestionType.FREE_RESPONSE); | ||
| return { qt, ...useTextEntryInteraction(makeFreeBlock(), qt) }; |
There was a problem hiding this comment.
Could we call them questionType instead of qt so that its more readable?
| routes: new VueRouter(), | ||
| }); | ||
|
|
||
| // ─── numeric ───────────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
We can live without these comments, it's responsibility of describe labels.
| // Helper: count only the answer-native-input elements (excludes csrf and other env inputs) | ||
| const answerInputs = () => | ||
| screen.getAllByRole('textbox').filter(el => el.classList.contains('answer-native-input')); |
There was a problem hiding this comment.
We can also filter by its accessible label, so that we make sure these textbox have an accessible label instead of checking the classList 👀
| if (!newVal.bodyXml) return; | ||
| emit('update:interaction', newVal); |
There was a problem hiding this comment.
Oh, I think we should have an if mode !== "edit" here to prevent emits from editors just because we mounted them even if they cannot edit anything.
| // NUMERIC: cardinality depends on how many acceptable answers are defined. | ||
| const answerCount = state?.answers?.length ?? 0; |
There was a problem hiding this comment.
For text entry we could also have multiple correct responses, it should take this path too, right?
| const itemBodyNode = | ||
| interactionNode.tagName.toLowerCase() === 'qti-item-body' | ||
| ? interactionNode | ||
| : buildXmlNode({ | ||
| tag: 'qti-item-body', | ||
| children: [interactionNode], | ||
| }); |
There was a problem hiding this comment.
Are there cases where the bodyXml already have the qti-item-body tag on it? 👀
There was a problem hiding this comment.
Yes, currently two paths do: buildTextEntryInteractionXML (interactions/textEntry/parse.js:230) returns a full <qti-item-body> as its bodyXml, and the inline branch in parseItem.js:100 returns serializeToString(body) — the whole <qti-item-body>, not the interaction element.
This branch was added for my finding at #6051 (comment): before it, assembleItemXml re-wrapped that bodyXml in a fresh <qti-item-body>, producing nested <qti-item-body><qti-item-body>… in the assembled output. Same two options as the <div> thread — make the bodyXml contract return inner content only, or tolerate it here; this took the second.
| /** | ||
| * Find the interaction descriptor for a given QTI interaction tag name. | ||
| * | ||
| * @param {string} tagName | ||
| * @returns {import('./defineInteraction').InteractionDescriptor|undefined} | ||
| */ | ||
| export function getDescriptorForInteractionType(tagName) { | ||
| return registry[tagName]; | ||
| } |
There was a problem hiding this comment.
Seems like this function is not being used anywhere, we can remove it.
There was a problem hiding this comment.
Noting that for some reason, when we edit answers, add/remove answers, case sensitive on/off, the xml is not being rebuilt, it is only rebuilt when the prompt changes:
Grabacion.de.pantalla.2026-07-28.a.la.s.9.24.04.a.m.mov
|
@AlexVelezLl, I have removed all the unnecessary comments. Apologies, they got overlooked, will take care of it from next time. |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6051 — delta re-review.
6 of 9 prior findings resolved, 2 acknowledged; 1 still open (QuestionType.TEXT_ENTRY scope vs #5979). The new commits fixed the prompt round-trip and the nested <qti-item-body> cleanly. The new findings are mostly in the same area: three places where this branch's serialized output does not conform to the QTI 3.0 schema Studio itself vendors, plus a view-mode editing gap.
CI passing. Manual QA was required for this PR but did not run — nothing here was visually verified, so the narrow-viewport .answer-layout wrapping and the focus-only .char-counter (which resizes the input on focus) are unchecked.
blocking
case-sensitiveon<qti-value>is not in the QTI 3.0 schema — items with case-sensitive answers fail backend validation.- Empty
<qti-correct-response/>emitted for 0-answer numeric/textEntry; the XSD requires ≥1qti-value. - Blanket
xmlnsstrip inparseXMLdestroys MathML/SVG namespaces anywhere in the document. mode="view"renders fully editable answer controls whose mutations are silently discarded.
suggestions/nitpicks — KDS KTextbox duplication, untranslated DOMParser error surfaced to authors, dynamically-built translator keys, prompt hover/focus styling, delete-button accessible names, blank-item answer seeding, demo data. All inline.
Still open from the prior round: QuestionType.TEXT_ENTRY is a third question type beyond the two #5979 scopes, and the blocking case-sensitive finding is a direct consequence — everything else in the type serializes within the schema. Worth a maintainer call on whether the type stays here or moves to its own issue.
Prior-finding status
RESOLVED — interactions/textEntry/parse.js:192 — Prompt not round-trip stable
RESOLVED — interactions/textEntry/parse.js:198 — Nested <qti-item-body> in assembled output
RESOLVED — interactions/textEntry/tests/parse.spec.js:279 — Round-trip tests never assert prompt
RESOLVED — serialization/parseItem.js:94 — Inline decision hardcodes TEXT_ENTRY
RESOLVED — interactions/textEntry/TextEntryEditor.vue:74 — Answer input lacks dir="auto"
RESOLVED — interactions/textEntry/TextEntryEditor.vue:335 — New-answer focus via document.getElementById
ACKNOWLEDGED — utils/math.js:8 — floatOrIntRegex accepts non-numeric strings
ACKNOWLEDGED — interactions/textEntry/TextEntryEditor.vue:317 — "Add acceptable answer" contrast
UNADDRESSED — QuestionType.TEXT_ENTRY scope creep beyond #5979
@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
| getXML: () => { | ||
| const valueEls = textAnswers.map(a => { | ||
| const valueEl = buildXmlNode({ tag: 'qti-value', children: [a.value] }); | ||
| if (a.caseSensitive) valueEl.setAttribute('case-sensitive', 'true'); |
There was a problem hiding this comment.
blocking: case-sensitive is not a valid attribute on <qti-value> in QTI 3.0, so any item authored with a case-sensitive answer produces XML the backend rejects outright.
Evidence from the XSD this repo vendors (contentcuration/utils/assessment/qti/schema/xsd/imsqti_itemv3p0p1_v1p0.xsd): ValueDType is a simpleContent extension of xs:normalizedString with exactly two attributes, field-identifier and base-type. case-sensitive appears only on StringMatchDType and MapEntryDType — on the response-processing operator, not on the declared value. The ingesting model agrees: contentcuration/utils/assessment/qti/assessment_item.py:27 gives Value only value / field_identifier / base_type, and qti/base.py:35 sets extra="forbid".
The round trip only looks clean because _extractAnswers (line 124) reads back the same non-standard attribute.
Options, roughly by cost: (a) drop per-answer case sensitivity for now and fold it into whatever issue scopes QuestionType.TEXT_ENTRY properly — this is the one piece of the type that forces a spec extension; (b) model it the way QTI does, via <qti-response-processing> with <qti-string-match case-sensitive="…">, which likely belongs in the response-processing issue; (c) store both casings as separate <qti-value> entries and derive the checkbox state — stays in-schema at some fidelity cost.
Note qtiDemoData.js:129 ships the same invalid attribute.
| if (a.caseSensitive) valueEl.setAttribute('case-sensitive', 'true'); | ||
| return valueEl; | ||
| }); | ||
| return buildXmlNode({ tag: 'qti-correct-response', children: valueEls }); |
There was a problem hiding this comment.
blocking: With registerCapability(CORRECT_RESPONSE, …) now called unconditionally for every non-free-response type, a 0-answer numeric or textEntry state serializes to <qti-correct-response/>. CorrectResponseDType in the vendored XSD declares <xs:element name="qti-value" … minOccurs="1" maxOccurs="unbounded"/> — at least one value is required.
Reachable immediately: _defaultState() returns answers: [], so a blank new item (demo-item-blank, or any item with no raw_data) set to numeric emits this. __tests__/parse.spec.js:255 currently asserts the invalid output as expected behaviour.
This came out of silencing the "missing correct-response" console warning on parse. Narrowing the warning itself is the better fix — _extractAnswers (lines 110–115) only needs to log when a float declaration is missing the element and the caller expects answers. Keeping <qti-correct-response> out of the output until there is ≥1 answer keeps the warning quiet and the output valid.
| let input = xmlString; | ||
| // Remove xmlns to ensure querySelector works. | ||
| if (mimeType === 'text/xml') { | ||
| input = xmlString.replace(/ xmlns="[^"]*"/g, ''); |
There was a problem hiding this comment.
blocking: This is a global regex over the whole document string, so it removes every default-namespace declaration, not just the QTI one on the root — and it never looks at document structure, so it also matches xmlns="…" occurring inside text content or attribute values.
QTI 3.0 item bodies may embed MathML, and Studio's backend models that as a first-class case (contentcuration/utils/assessment/qti/mathml/, with Math imported into prompt.py, catalog.py, and html/content_types.py). An externally-authored item whose prompt contains <math xmlns="http://www.w3.org/1998/Math/MathML">…</math> loads into the editor, loses the namespace here, and assembleItemXml writes it back namespace-less — silently reinterpreting the MathML as QTI-namespace elements.
Two structural alternatives that don't touch nested content: strip only the root element's declaration (match against the first tag), or keep the document namespaced and select with getElementsByTagNameNS('*', 'qti-item-body') / a localName predicate instead of querySelector.
Related: because prompt nodes are parsed as text/html and importNoded into a namespace-less XML doc (textEntry/parse.js:205-206, assembleItem.js:45), the emitted bodyXml carries xmlns="http://www.w3.org/1999/xhtml" on every prompt element, and this strip is what quietly cleans it up on the next parse. Building the prompt fragment in the XML document directly would remove that coupling.
| :class="{ 'small-screen': windowIsSmall }" | ||
| > | ||
| <div class="answer-input-wrap"> | ||
| <input |
There was a problem hiding this comment.
blocking: In mode="view" these controls are fully interactive but their edits are silently thrown away. The answer section renders whenever mode === 'edit' || showAnswers (line 37), and nothing inside it is gated on mode except AddListItemButton (line 157): this <input> has no readonly/disabled, the case-sensitive KCheckbox (109–116) has no :disabled, and the delete KIconButton (118–128) is disabled only on answers.length <= 1.
updateAnswerValue / toggleCaseSensitive / removeAnswer all write to state, so the preview visibly changes — but the emit watcher bails on props.mode !== 'edit' (line 351), so nothing propagates.
ChoiceInteractionEditor.vue in the same directory is the pattern to copy:
<KRadioButton ... :disabled="mode !== 'edit'" />
<KCheckbox ... :disabled="mode !== 'edit'" />
<div v-if="mode === 'edit'" class="choice-actions toolbar">Suggested: :readonly="mode !== 'edit'" on the input, :disabled="mode !== 'edit'" on the checkbox, and v-if="mode === 'edit'" on the delete button.
| runValidation(); | ||
| " | ||
| > | ||
| <span |
There was a problem hiding this comment.
suggestion: This hand-rolled character counter and the raw <input> above it duplicate KDS KTextbox, which AGENTS.md asks you to reach for first ("If a component does 80% of what you need, wrap it — do not rewrite").
KTextbox forwards :maxlength with :enforceMaxlength="true", and keen/UiTextbox.vue:150-153 renders exactly a {{ valueLength + '/' + maxlength }} counter for you. It also covers :invalid/:invalidText for the per-answer message, :readonly/:disabled for the finding above, and :label/:autofocus for the accessible name and focus-after-add. It's the established pattern here — EditTitleDescriptionModal.vue:17, NewTopicModal.vue:15, PublishSidePanel.vue:58, Create.vue:48.
If the borderless-input-inside-a-bordered-card visual genuinely can't be expressed through appearanceOverrides, that's a fair reason to keep the native input — please say so in a comment, because as written it reads as an unintentional rewrite. In that case at least drop the counter/maxlength pair in favour of KDS's.
| </div> | ||
| <KIconButton | ||
| icon="close" | ||
| :tooltip="deleteAnswerBtn$()" |
There was a problem hiding this comment.
suggestion: Every delete button gets the identical accessible name, so with three answer rows a screen-reader user hears "Delete answer" three times with nothing distinguishing them — __tests__/TextEntryEditor.spec.js:254 has to use getAllByRole for exactly this reason. Parameterise it:
deleteAnswerBtn: {
message: 'Delete answer {number}',
context: 'Accessible label for the delete icon button next to an answer row',
},and pass { number: index + 1 } — the count must go inside the translated string, not be concatenated around it.
| <div | ||
| :class="promptWrapperClass" | ||
| :style="promptWrapperStyle" | ||
| @click="handlePromptClick" |
There was a problem hiding this comment.
suggestion: The prompt is mouse-only — a plain <div> with @click, no tabindex, role, or @keydown.enter/space. The only other way in is the watcher at 253–265, which auto-opens only when the prompt is empty, so a keyboard-only author editing an existing question has no path to put the prompt into edit mode (TipTapEditor stays :mode="'view'" until isPromptOpen flips).
Carried over from ChoiceInteractionEditor, so not newly introduced — but newly duplicated into a second file, which is usually the moment to fix it in both. AGENTS.md lists "Keyboard navigation must work" under Accessibility.
| export function _defaultState() { | ||
| return { | ||
| prompt: '', | ||
| answers: [], |
There was a problem hiding this comment.
suggestion: answers: [] means a numeric item with no raw_data renders the "Acceptable answers" header and the Add button but no input row, and removeAnswer's "keep at least one" guard never gets a chance to hold. choice/parse.js:_defaultState seeds one empty choice for the same reason — seeding one empty answer here would make the two consistent and would sidestep the empty-<qti-correct-response/> case above for new items.
Separately, the TextEntryState typedef just above (lines 26–28) says expectedLength is "0 (absent) for numeric and textEntry", but buildTextEntryInteractionXML now writes expected-length for all three types (186–189) and parseTextEntryInteraction defaults an absent attribute to 50. The change looks deliberate (the 50-char limit now also drives maxlength on the input), but it diverges from #5979's buildXML criterion — "adds expected-length="50" … only for freeResponse". Worth updating the doc comment and noting the intentional divergence on the issue.
| <qti-correct-response> | ||
| <qti-value case-sensitive="true">H2O</qti-value> | ||
| <qti-value>h2o</qti-value> | ||
| <qti-value>H2o</qti-value> |
There was a problem hiding this comment.
nitpick: This demo item loads with a validation error: h2o and H2o are both case-insensitive, so validateTextEntryInteraction normalizes both to h2o|false and this third row renders DUPLICATE_ANSWER_CONTENT as soon as the debounced validator fires. Dropping H2o (or marking it case-sensitive) gives a clean demo.
| @@ -0,0 +1,76 @@ | |||
| <template> | |||
There was a problem hiding this comment.
praise: Extracting the "Add …" button and reusing it in both editors is the right call at exactly the point the duplication appeared, and it keeps the Figma styling in one place as more interaction editors land.
… parse error handling in QTI editor Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
15b30cc to
fdeef39
Compare
|
📢✨ Before we assign a reviewer, we'll turn on |
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.