refactor: consolidate QTIEditor XML/HTML parsing on parseXML#6045
Conversation
Give parseXML a mimeType parameter (default 'text/xml') with an XML-only parsererror check, and route QTISanitizer.stripTags and the declaration test helper through it instead of constructing their own DOMParser. The HTML parser recovers silently and never emits a parsererror node, so the failure check runs only for XML. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Could you also migrate the DomParser usages on serialization/assembleItem.js? It's mentioned on the issue and acceptance criteria.
The innerHTML fragment parse (text/html) and the declaration/body parses (text/xml) in assembleItem.js now go through the shared parseXML entry point. The text/xml call sites inherit the parsererror check, surfacing malformed declaration or body XML as a thrown error; the text/html fragment parse stays lenient. The bare-Document element factory at line 11 is left as-is per the issue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@AlexVelezLl Good catch — done in 2151060. Those usages weren't present when this branch was cut (
The bare- |
| const doc = parseXML(declXml, 'text/xml'); | ||
| return doc.documentElement; | ||
| }); | ||
|
|
||
| // Parse the interaction body XML into a DOM node. | ||
| const bodyDoc = declParser.parseFromString(bodyXml || '<qti-item-body/>', 'text/xml'); | ||
| const bodyDoc = parseXML(bodyXml || '<qti-item-body/>', 'text/xml'); |
There was a problem hiding this comment.
We don't really need the type here, right?
There was a problem hiding this comment.
Right — parseXML defaults mimeType to 'text/xml', so it was redundant. Dropped it here and from the declaration parse just above it (same construct); the innerHTMLfragment parse keeps its explicit'text/html' since that one is not the default. Fixed in 2b538ac.
…alls parseXML defaults its mimeType to 'text/xml', so passing it explicitly at the declaration and body parse sites in assembleItemXml was redundant. Drop it from both. The innerHTML fragment parse keeps its explicit 'text/html' since that is not the default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary
QTIEditor code parsed markup two ways:
parseXML, which surfaces malformed input as a thrown error, and barenew DOMParser()calls that silently proceed on a broken document. This consolidates parsing ontoparseXML, which now takes amimeTypeargument (default'text/xml') so HTML-fragment callers can parse leniently while XML callers keep theparsererrorfailure check.References
Follow-up from #6012. Closes #6044.
Reviewer guidance
serialization/parseItem.js:22— theparsererrorcheck is skipped only formimeType === 'text/html'(every other mode still runs it); confirm an HTML input literally containing a<parsererror>element cannot trip it.serialization/qti/QTISanitizer.js:96—stripTagsnow routes throughparseXML('text/html'); check it stays lenient and never throws on sanitizer input.serialization/qti/__tests__/testUtils.js:14— the helper still returnsdocumentElement(anElement), not theDocument, so its five declaration-spec consumers are unchanged.AI usage
Used Claude Code to implement the change following a pre-approved plan with test-driven development. Verified with the full QTIEditor Jest suite and the pre-commit JS linter.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
🟡 Waiting for feedback
Last updated: 2026-07-17 15:17 UTC