From c9085603e377ece9c430cd61d04478fe0dbc26f0 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Wed, 7 Oct 2026 12:30:52 -0700 Subject: [PATCH] fix(qti): key answers on their formulas and images, not text alone Answers differing only in a formula or image no longer merge in match/associate or flag as duplicates. Closes #6301 Co-Authored-By: Claude Opus 5.5 --- .../associate/__tests__/parse.spec.js | 18 ++ .../choice/__tests__/validation.spec.js | 13 ++ .../match/__tests__/parse.spec.js | 16 ++ .../ordering/__tests__/validation.spec.js | 13 ++ .../serialization/qti/QTISanitizer.js | 16 +- .../utils/__tests__/richText.spec.js | 166 ++++++++++++++++++ .../shared/views/QTIEditor/utils/richText.js | 121 ++++++++++++- 7 files changed, 357 insertions(+), 6 deletions(-) diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/associate/__tests__/parse.spec.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/associate/__tests__/parse.spec.js index c32e3ab262..78fa53bba0 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/associate/__tests__/parse.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/associate/__tests__/parse.spec.js @@ -13,6 +13,9 @@ const contentsOf = pairs => pairs.map(pair => pair.map(choice => choice.content) const SCHEMA = { baseType: BaseType.PAIR, cardinality: Cardinality.MULTIPLE }; +const SOLVE_X2 = '

Solve for x

'; +const SOLVE_Y3 = '

Solve for x

'; + const parseXmlString = xml => parseXML(xml).documentElement; describe('parse()', () => { @@ -430,6 +433,21 @@ describe('parse → buildXML → parse round-trip', () => { ]); }); + it('preserves two choices with the same text but different formulas', () => { + const state = { + responseIdentifier: 'RESPONSE', + prompt: '', + pairs: [ + [ + { id: 'choice_aaa11111', content: SOLVE_X2 }, + { id: 'choice_bbb22222', content: SOLVE_Y3 }, + ], + ], + distractors: [], + }; + expect(contentsOf(roundTrip(state).pairs)).toEqual([[SOLVE_X2, SOLVE_Y3]]); + }); + it('preserves the default state as one pair of two blank choices', () => { const reparsed = roundTrip(parse('', [])); expect(contentsOf(reparsed.pairs)).toEqual([['', '']]); diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/choice/__tests__/validation.spec.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/choice/__tests__/validation.spec.js index 9f6e8224e2..68bb1ea3a2 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/choice/__tests__/validation.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/choice/__tests__/validation.spec.js @@ -132,6 +132,19 @@ describe('validate()', () => { ValidationError.DUPLICATE_CHOICE_CONTENT, ); }); + + it.each([ + ['a formula', '', ''], + ['an image', '', ''], + ])('does not flag choices with the same text but %s that differs', (_, first, second) => { + const state = makeState({ + choices: [ + makeAnswer({ id: 'a', content: `

Solve ${first} for x

`, correct: true }), + makeAnswer({ id: 'b', content: `

Solve ${second} for x

`, correct: false }), + ], + }); + expect(validate(state, QuestionType.SINGLE_SELECT)).toEqual([]); + }); }); describe('EMPTY_CHOICE_CONTENT', () => { diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/match/__tests__/parse.spec.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/match/__tests__/parse.spec.js index 9f0e6f1469..3cc9394200 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/match/__tests__/parse.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/match/__tests__/parse.spec.js @@ -23,6 +23,9 @@ const declaration = (...values) => ${values.map(v => `${v}`).join('')} `; +const SOLVE_X2 = '

Solve for x

'; +const SOLVE_Y3 = '

Solve for x

'; + const ROWS = matchSet(choice('row_dog', 'Dog') + choice('row_eagle', 'Eagle')); const RESPONSES = matchSet( choice('choice_mammal', 'Mammal') + choice('choice_bird', 'Bird') + choice('choice_fur', 'Fur'), @@ -484,6 +487,19 @@ describe('parse → buildXML → parse round-trip', () => { expect(roundTrip(state)).toEqual(state); }); + it('preserves answers with the same text but different formulas', () => { + const state = { + responseIdentifier: 'RESPONSE', + prompt: '', + rows: [ + { id: 'row_dog', content: 'Dog', matches: [{ id: 'choice_a', content: SOLVE_X2 }] }, + { id: 'row_eagle', content: 'Eagle', matches: [{ id: 'choice_b', content: SOLVE_Y3 }] }, + ], + distractors: [], + }; + expect(matchContents(roundTrip(state).rows)).toEqual([[SOLVE_X2], [SOLVE_Y3]]); + }); + it('preserves the default state', () => { const original = parse('', []); expect(roundTrip(original)).toEqual(original); diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/ordering/__tests__/validation.spec.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/ordering/__tests__/validation.spec.js index f8856eaf52..b2ea2b0da2 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/ordering/__tests__/validation.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/ordering/__tests__/validation.spec.js @@ -157,5 +157,18 @@ describe('validateOrderingInteraction()', () => { ValidationError.DUPLICATE_CHOICE_CONTENT, ); }); + + it.each([ + ['a formula', '', ''], + ['an image', '', ''], + ])('does not flag items with the same text but %s that differs', (_, first, second) => { + const state = makeState({ + items: [ + makeItem({ id: 'a', content: `

Solve ${first} for x

` }), + makeItem({ id: 'b', content: `

Solve ${second} for x

` }), + ], + }); + expect(validateOrderingInteraction(state)).toEqual([]); + }); }); }); diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/qti/QTISanitizer.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/qti/QTISanitizer.js index 97736127db..298739336d 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/qti/QTISanitizer.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/qti/QTISanitizer.js @@ -93,10 +93,20 @@ export class QTISanitizer { if (typeof value !== 'string') return String(value ?? ''); // Fast path: if no `<` is present there is nothing to strip. if (!value.includes('<')) return value; - const doc = parseXML(value, 'text/html'); + return QTISanitizer.textBody(value).textContent ?? ''; + } + + /** + * Parse an HTML fragment into a body whose `textContent` is its visible text. + * + * @param {string} html + * @returns {HTMLElement} + */ + static textBody(html) { + const { body } = parseXML(html, 'text/html'); // Remove script and style elements entirely — we do NOT want their text content. - doc.querySelectorAll('script, style').forEach(el => el.remove()); - return doc.body.textContent ?? ''; + body.querySelectorAll('script, style').forEach(el => el.remove()); + return body; } /** diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/__tests__/richText.spec.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/__tests__/richText.spec.js index d398d94b0c..a8627865a4 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/__tests__/richText.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/__tests__/richText.spec.js @@ -1,3 +1,4 @@ +import { useEditor } from '../../../TipTapEditor/TipTapEditor/composables/useEditor'; import { hasRichTextContent, richTextComparisonKey } from '../richText'; describe('hasRichTextContent', () => { @@ -52,6 +53,171 @@ describe('richTextComparisonKey', () => { ); }); + it('keeps the same text apart when its formulas differ', () => { + expect(richTextComparisonKey('

Solve for x

')).not.toBe( + richTextComparisonKey('

Solve for x

'), + ); + }); + + it('keeps the same text apart when its images differ', () => { + expect(richTextComparisonKey('

see

')).not.toBe( + richTextComparisonKey('

see

'), + ); + }); + + it('keeps the same text and formula apart when the formula moves', () => { + expect(richTextComparisonKey('

solve

')).not.toBe( + richTextComparisonKey('

solve

'), + ); + }); + + it('keeps a formula apart from its LaTeX typed as text', () => { + expect(richTextComparisonKey('

a

')).not.toBe( + richTextComparisonKey('

a x^2

'), + ); + }); + + it('keeps a formula apart from its key typed as text', () => { + const key = richTextComparisonKey('

a

'); + expect(richTextComparisonKey(`

${key}

`)).not.toBe(key); + }); + + it('keeps a formula apart from an image whose identity it spells', () => { + const image = '

a

'; + const [, identity] = richTextComparisonKey(image).split('\u0000'); + expect(richTextComparisonKey(`

a

`)).not.toBe( + richTextComparisonKey(image), + ); + }); + + it.each([ + ['alt text', 'a cat', 'a dog'], + ['width', '', ''], + ['height', '', ''], + ['alignment', '', ''], + ['style', '', ''], + ])('keeps the same text and image apart when its %s differs', (_, first, second) => { + expect(richTextComparisonKey(`

see ${first}

`)).not.toBe( + richTextComparisonKey(`

see ${second}

`), + ); + }); + + it('reads attribute order and the default alignment as the same image beside text', () => { + expect(richTextComparisonKey('

see a cat

')).toBe( + richTextComparisonKey('

see a cat

'), + ); + }); + + it.each([ + ['title', ''], + ['class', ''], + ['size style', ''], + ['saved upload', ''], + ])('reads an image beside text with a %s it does not show as the same image', (_, other) => { + expect(richTextComparisonKey(`

see ${other}

`)).toBe( + richTextComparisonKey('

see

'), + ); + }); + + it.each([ + ['title', ''], + ['size style', ''], + ])('keeps an image with a %s apart when there is no text to compare', (_, other) => { + expect(richTextComparisonKey(`

${other}

`)).not.toBe( + richTextComparisonKey('

'), + ); + }); + + it.each([ + ['formula', '', ''], + ['image', '', ''], + ])('keeps the same code apart when its %s differs', (_, first, second) => { + expect(richTextComparisonKey(`
x ${first}
`)).not.toBe( + richTextComparisonKey(`
x ${second}
`), + ); + }); + + it('keeps the indentation of a code block', () => { + expect(richTextComparisonKey('
if x:\n    return 1
')).not.toBe( + richTextComparisonKey('
if x:\n  return 1
'), + ); + }); + + it.each([ + ['paragraphs', '

a

b

'], + ['a line break', '

a
b

'], + ['list items', ''], + ['a code block', '
ab
'], + ])('reads text in %s as the same text in one paragraph', (_, split) => { + expect(richTextComparisonKey(split)).toBe(richTextComparisonKey('

ab

')); + expect(richTextComparisonKey(split)).not.toBe(richTextComparisonKey('

a b

')); + }); + + it.each([ + ['a trailing', '

Paris 

'], + ['a leading', '

 Paris

'], + ['an empty paragraph of', '

Paris

 

'], + ])('drops %s non-breaking space', (_, padded) => { + expect(richTextComparisonKey(padded)).toBe(richTextComparisonKey('

Paris

')); + }); + + it('reads plain text as the same text in a paragraph', () => { + expect(richTextComparisonKey('Paris')).toBe(richTextComparisonKey('

Paris

')); + }); + + describe('after TipTap rewraps it', () => { + let editors = []; + + const rewrap = html => { + const { initializeEditor, editor } = useEditor(); + initializeEditor(html); + editors.push(editor.value); + return editor.value.getHTML(); + }; + + afterEach(() => { + editors.forEach(editor => editor.destroy()); + editors = []; + }); + + it.each([ + '

see here

', + '

\n \n caption\n

', + '

see a cat

', + '

Solve\n \n for x

', + '

a

\n

', + '

a

\n

\n \n

', + '

see

', + '
if x:\n    return 1
', + '

a
b

', + ])('%j keys the same', stored => { + expect(richTextComparisonKey(rewrap(stored))).toBe(richTextComparisonKey(stored)); + }); + }); + + it('keeps the spaces around a formula', () => { + expect(richTextComparisonKey('

a b

')).not.toBe( + richTextComparisonKey('

ab

'), + ); + }); + + it('collapses runs of whitespace beside a formula', () => { + expect(richTextComparisonKey('

a

')).toBe( + richTextComparisonKey('

a

'), + ); + }); + + it('keeps runs of whitespace in text alone', () => { + expect(richTextComparisonKey('

a b

')).not.toBe(richTextComparisonKey('

a b

')); + }); + + it('keeps a non-breaking space apart from a space', () => { + expect(richTextComparisonKey('

a  b

')).not.toBe(richTextComparisonKey('

a b

')); + expect(richTextComparisonKey('

a 

')).not.toBe( + richTextComparisonKey('

a

'), + ); + }); + it('still matches the same image offered twice', () => { expect(richTextComparisonKey('

')).toBe( richTextComparisonKey('

\n \n

'), diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/richText.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/richText.js index 622a3f823b..b2032aa437 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/richText.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/richText.js @@ -50,14 +50,90 @@ export function hasRichTextContent(content) { ); } +/** + * Elements whose surrounding whitespace is layout, not a word gap: TipTap drops it, and + * markup read back out of stored XML arrives pretty-printed with it. + */ +const BLOCK_ELEMENTS = + 'body, p, div, ul, ol, li, h1, h2, h3, h4, h5, h6, blockquote, pre, table, thead, tbody, ' + + 'tfoot, tr, th, td, hr'; + +/** Whitespace as HTML collapses it; a non-breaking space is not. */ +const HTML_WHITESPACE = /[\t\n\f\r ]+/g; +const LEADING_WHITESPACE = /^[\t\n\f\r ]+/; +const TRAILING_WHITESPACE = /[\t\n\f\r ]+$/; + +/** + * Remove whitespace-only text beside a block, which the reader never sees. + * + * @param {HTMLElement} body - changed in place + */ +function dropBlockIndentation(body) { + const isBoundary = (sibling, parent) => + sibling + ? sibling.nodeType === Node.ELEMENT_NODE && sibling.matches(BLOCK_ELEMENTS) + : parent.matches(BLOCK_ELEMENTS); + const walker = body.ownerDocument.createTreeWalker(body, NodeFilter.SHOW_TEXT); + const indentation = []; + while (walker.nextNode()) { + const { currentNode: text } = walker; + if ( + !text.parentNode.closest('pre') && + !text.nodeValue.replace(HTML_WHITESPACE, '') && + (isBoundary(text.previousSibling, text.parentNode) || + isBoundary(text.nextSibling, text.parentNode)) + ) { + indentation.push(text); + } + } + indentation.forEach(text => text.remove()); +} + +/** + * Markup with its layout normalised away, as read back out of stored XML it arrives + * pretty-printed. + * + * @param {string} html + * @returns {string} + */ +function normaliseMarkup(html) { + return html.replace(/\s+/g, ' ').replace(/>\s+<').trim(); +} + +/** + * What identifies an embedded media element, tagged with its kind so no two kinds collide, + * and delimited by NUL because the HTML parser drops it from text, so nothing typed can + * read as one. + * + * @param {Element} element - matches `EMBEDDED_MEDIA` + * @returns {string} + */ +function mediaToken(element) { + let identity = normaliseMarkup(element.outerHTML); + if (element.hasAttribute('data-latex')) { + identity = JSON.stringify(['latex', element.getAttribute('data-latex')]); + } else if (element.localName === 'img') { + // What the Image node keeps and shows, read as it reads them (`TipTapEditor/extensions/ + // Image.js`), so TipTap's re-render of the same image keys the same. + identity = JSON.stringify([ + 'img', + ...['src', 'alt', 'width', 'height'].map(name => element.getAttribute(name)), + element.style.textAlign || element.getAttribute('data-text-align') || 'left', + ]); + } + return `\u0000${identity}\u0000`; +} + /** * A key two fragments can be compared on to tell whether they say the same thing. * * Text is what an author reads, so text is what duplicates are judged on — two choices * that read the same are the same choice however differently they are marked up. A * fragment with no text has only its markup to go on, which keeps two different images - * apart while still catching the same image offered twice — read back out of stored XML - * it arrives pretty-printed, so the markup is compared with its layout normalised away. + * apart while still catching the same image offered twice. + * + * Text with formulas or images in it is judged on what a learner sees: the text as + * rendered, and each formula and image where it stands. * * Only meaningful for a fragment that `hasRichTextContent`; an empty one keys to ''. * @@ -65,5 +141,44 @@ export function hasRichTextContent(content) { * @returns {string} */ export function richTextComparisonKey(content) { - return toText(content) || (content ?? '').replace(/\s+/g, ' ').replace(/>\s+<').trim(); + const html = content ?? ''; + // As `toText` reads it: with no tag there is no media, and nothing to parse. + if (!html.includes('<')) return html.trim(); + const body = QTISanitizer.textBody(html); + const text = body.textContent.trim(); + if (!text) return normaliseMarkup(html); + if (!body.querySelector(EMBEDDED_MEDIA)) return text; + dropBlockIndentation(body); + // Document order puts an element before any media nested in it, so the outermost is + // replaced first and what is nested goes with it. + const breaks = []; + body.querySelectorAll(`${EMBEDDED_MEDIA}, pre`).forEach(el => { + if (!el.isConnected) return; + if (el.localName === 'pre') { + // A code block is text, but shows its whitespace as typed. + el.querySelectorAll(EMBEDDED_MEDIA).forEach(media => { + if (media.isConnected) media.replaceWith(mediaToken(media)); + }); + breaks.push({ text: el.textContent, isInline: false }); + } else { + breaks.push({ text: mediaToken(el), isInline: el.localName !== 'img' }); + } + el.replaceWith('\u0000'); + }); + // Whitespace beside an image or code block is dropped: TipTap moves an image out of its + // paragraph, and the space with it. A formula stays inline. At the fragment's edges a + // non-breaking space is dropped too, as the reader cannot see it. + return body.textContent + .split('\u0000') + .map((piece, i) => { + const before = breaks[i - 1]; + const after = breaks[i]; + let collapsed = piece; + if (!before) collapsed = collapsed.trimStart(); + else if (!before.isInline) collapsed = collapsed.replace(LEADING_WHITESPACE, ''); + if (!after) collapsed = collapsed.trimEnd(); + else if (!after.isInline) collapsed = collapsed.replace(TRAILING_WHITESPACE, ''); + return collapsed.replace(HTML_WHITESPACE, ' ') + (after?.text ?? ''); + }) + .join(''); }