Add the QTI associate interaction plugin - #6113
rtibblesbot wants to merge 5 commits into
Conversation
6e5a1f4 to
2c9fff3
Compare
AlexVelezLl
left a comment
There was a problem hiding this comment.
@rtibblesbot, from the screenshots, I can see that the padding of the chips is not the same as in the specs, and the colors of the pair rows are not the same as in the specs. Take a closer look at the images in the issue and check that everything is correctly aligned.
|
@AlexVelezLl Fixed, and I swept every element in the editor against the six mockups rather than only the two you named — eight divergences, all changed:
Two mockup details I left out, both more than styling — say the word and I'll add them:
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly |
e0394dd to
6ce2d00
Compare
AlexVelezLl
left a comment
There was a problem hiding this comment.
Just few findings from a high-level overview.
| }, | ||
|
|
||
| associateLabel: { | ||
| message: 'Connect pairs', |
There was a problem hiding this comment.
Could you translate it to "Associate" instead?
There was a problem hiding this comment.
Renamed to Associate.
| message: 'Connect pairs', | ||
| context: 'Display name for an associate question type shown in the question type selector', | ||
| }, | ||
| associateDescription: { |
There was a problem hiding this comment.
Must associate pairs of items, or something similar
There was a problem hiding this comment.
Now Learners must associate pairs of items.
There was a problem hiding this comment.
Let's add what we added for choice interaction, and let's prevent the removal of the last pair, also let's disable the remove button if its the last pair.
There was a problem hiding this comment.
removePair is a no-op at one pair, matching removeChoice, and the delete button is disabled there. Spec gained that case plus a max-associations block mirroring choice's max-choices one. Checked the branch's other list mutators: removeDistractor is the only sibling, and distractors are optional, so it keeps no floor.
|
|
||
| it('appends a third pair when the add pair button is clicked', async () => { | ||
| renderEditor(); | ||
| await fireEvent.click(screen.getByRole('button', { name: tr.$tr('addPairBtn') })); |
There was a problem hiding this comment.
Let's use userEvent.setup() instead.
There was a problem hiding this comment.
Whole editor spec is on userEvent.setup() now — 43 interactions, no fireEvent left under interactions/associate/. It was the only spec on the branch using it; the parse, validate and composable specs call the units directly.
| it('appends a second distractor when the add distractor button is clicked', async () => { | ||
| renderEditor(); | ||
| await fireEvent.click(screen.getByRole('button', { name: tr.$tr('addDistractorBtn') })); | ||
| expect( | ||
| screen.getByRole('button', { name: tr.$tr('deleteDistractorBtn', { number: 2 }) }), | ||
| ).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
This behavior is incorrect. When the "Add distractor" button is clicked, it should show a TipTapEditor below the distractors pool, and it should be saved when the Save button is clicked.
If focus is lost and tiptap is closed, let's add a small "new distractor" label so that users can come back and edit it until they hit save. Once they hit save, then we can show the add distractor button again.
There was a problem hiding this comment.
Reworked. Add distractor opens a TipTap editor below the pool with a Save button; the draft lives outside state, so nothing joins the pool and nothing is emitted until Save. Closing the editor (by clicking a pair, say) leaves a small New distractor chip that reopens it with the written content still there; Save turns it into a chip and brings the Add distractor button back.
Verified in the running editor, not only in jsdom — worth it, because the first version passed its tests and was dead in the browser: the click that opens the draft finishes bubbling after TipTap has mounted its outside-click listener, so the editor closed itself immediately. Add pair had the same bug (the new pair's editor never stayed open). Both add buttons now stop the click.
| expect(contentsOf(parse(ASSOCIATE_XML, [decl]).pairs)).toEqual([['Capulet', 'Montague']]); | ||
| }); | ||
|
|
||
| it('drops a pair naming identifiers that only exist on Object.prototype', () => { |
There was a problem hiding this comment.
It's not clear to me what this tests for, is because these choices are not declared on ASSOCIATE_XML?
There was a problem hiding this comment.
Right — constructor/toString are not declared in ASSOCIATE_XML, and the point is that the pool lookup must not resolve them off Object.prototype (it is a Map, not a plain object). Renamed the test to say that and added the reason as a comment on both prototype cases.
| it('counts a distractor repeat of paired content towards match-max', () => { | ||
| const distractors = [{ id: 'choice_zzz00000', content: 'Antonio' }]; | ||
| const root = parseXmlString(build({ ...baseState, distractors }).bodyXml); | ||
| const antonio = choicesOf(root).filter(el => el.textContent === 'Antonio'); | ||
| expect(antonio).toHaveLength(1); | ||
| expect(antonio[0].getAttribute('match-max')).toBe('2'); | ||
| }); | ||
|
|
||
| it('reassigns the id of a later choice that reuses an id with different content', () => { | ||
| const pairs = [ | ||
| baseState.pairs[0], | ||
| [ | ||
| { id: 'choice_aaa11111', content: 'Capulet' }, | ||
| { id: 'choice_ddd44444', content: 'Montague' }, | ||
| ], | ||
| ]; | ||
| const { bodyXml, responseDeclarations } = build({ | ||
| ...baseState, | ||
| pairs, | ||
| distractors: [], | ||
| }); | ||
| const [capulet] = choicesOf(parseXmlString(bodyXml)).filter( | ||
| el => el.textContent === 'Capulet', | ||
| ); | ||
| expect(capulet.getAttribute('identifier')).toMatch(/^choice_/); | ||
| expect(capulet.getAttribute('identifier')).not.toBe('choice_aaa11111'); | ||
| expect(valuesOf(parseXmlString(responseDeclarations[0]))[1]).toBe( | ||
| `${capulet.getAttribute('identifier')} choice_ddd44444`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Could you make some permutations of these to test what happens if: the first appearance is repeated, then a following has the same id but different content, etc. Also, lets test that the first appearance keep the id.
There was a problem hiding this comment.
Added an id normalization block with four permutations: a repeat carrying a different id (the first id wins, the second never appears), a later choice reusing an id with different content (the first keeps the id), a repeat followed by an id conflict on the same choice (match-max="2" plus a fresh id for the conflict), and a third choice repeating already-reassigned content. Each asserts the pool and the <qti-value> list.
| describe('TOO_FEW_PAIRS', () => { | ||
| it('returns error when there are no pairs at all', () => { | ||
| expect(errorCodes(validateAssociateInteraction(makeState({ pairs: [] })))).toContain( | ||
| ValidationError.TOO_FEW_PAIRS, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This should test also valid pairs, if there is a pair, but its invalid, then show the too few valid pairs error.
There was a problem hiding this comment.
Added: the only pair blank, the only pair holding the same content twice, every pair invalid for a different reason, and one valid pair among invalid ones (no error).
| pairedCount.set(id, (pairedCount.get(id) || 0) + 1); | ||
| } | ||
|
|
||
| const distractors = pool.flatMap(({ id, content, matchMax }) => |
There was a problem hiding this comment.
Could we use 'lodash/flatMap' instead?
There was a problem hiding this comment.
Done, and the rest of the branch with it — 6 sites: parse.js (3), validate.js (1), the editor (2). No native flat/flatMap left in the associate plugin.
| function shuffled(items) { | ||
| const result = [...items]; | ||
| for (let i = result.length - 1; i > 0; i--) { | ||
| const j = Math.floor(Math.random() * (i + 1)); | ||
| [result[i], result[j]] = [result[j], result[i]]; | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Could we use lodash/shuffle instead?
There was a problem hiding this comment.
Done — the hand-rolled Fisher–Yates is gone.
6ce2d00 to
366bf77
Compare
| <KIconButton | ||
| icon="close" | ||
| size="mini" | ||
| :ariaLabel="deleteDistractorBtn$({ number: index + 1 })" | ||
| :tooltip="deleteDistractorBtn$({ number: index + 1 })" | ||
| :color="$themePalette.grey.v_800" | ||
| @click="onRemoveDistractor(index)" | ||
| /> |
There was a problem hiding this comment.
Lets use size="small" and v_700 for the color
There was a problem hiding this comment.
Done — size="small", $themePalette.grey.v_700.
Searched the editor for other KIconButtons: one more, the pair delete, which your other comment puts at base/gray/400 — also now size="small".
| <div | ||
| v-if="isDraftOpen" | ||
| class="draft-editor item-border" | ||
| :style="{ borderColor: $themeTokens.fineLine }" | ||
| > | ||
| <TipTapEditor | ||
| :value="draft.content" | ||
| mode="edit" | ||
| format="html" | ||
| :minHeight="'48px'" | ||
| autofocus | ||
| :imageProcessor="EditorImageProcessor" | ||
| :tabindex="-1" | ||
| class="editor" | ||
| @update="setDraftContent" | ||
| @minimize="closeOpenTarget" | ||
| /> | ||
| </div> | ||
| <ClickableRegion | ||
| v-else | ||
| class="chip draft-chip is-tinted" | ||
| :style="{ borderColor: $themeTokens.fineLine }" | ||
| :aria-label="editNewDistractorLabel$()" | ||
| @click="openDraft" | ||
| > | ||
| {{ newDistractorLabel$() }} | ||
| </ClickableRegion> | ||
| <KButton | ||
| primary | ||
| :text="saveDistractorBtn$()" | ||
| @click="saveDraft" | ||
| /> | ||
| </div> | ||
| <div | ||
| v-else | ||
| @click.stop | ||
| > | ||
| <AddListItemButton | ||
| :label="addDistractorBtn$()" | ||
| @click="onAddDistractor" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Let's make some UX changes here so that it's consistent with other RTEs where we don't need a save button.
- Let's remove the
savebutton. When the RTE is blurred or closed, it will automatically be added to the list (if it was new) or updated. If the content is empty, then remove it from the list. - The add distractor will always be present.
- With this, no "new distractor" placeholder chip should be present at any point.
- The
removebutton should always be present on the current editable row, no matter if it's a new distractor; this will be the "cancel" button in case the user regrets it after hitting the add distractor button.
There was a problem hiding this comment.
Reworked, all four points:
Savegone. Closing the editor commits: a written draft joins the pool, and an existing distractor emptied then closed leaves it. Verified in the browser that TipTap's blur-timeupdatelands before the click that closed the editor, so the committed content is the typed content.Add distractoris always rendered.newDistractorLabel/editNewDistractorLabel/saveDistractorBtndeleted.- The draft row carries the same remove button as a chip, which is also how you abandon it.
| /> | ||
| </div> | ||
|
|
||
| <ValidationMessage |
There was a problem hiding this comment.
Could we set the RTE border to red if it has an error? If it's an "Answers within a pair cannot be the same" error, then both RTEs should have a red border.
There was a problem hiding this comment.
Done. pairItemStyle turns a card's border red for a blank member, for both members when the pair duplicates itself, and for a member a distractor repeats (your validate.js comment).
Swept the other bordered editors in the file for the same gap: distractor chips get the same treatment via distractorStyle; the prompt already had it.
| <ClickableRegion | ||
| class="chip-region" | ||
| :class="{ 'is-clickable': !isDistractorOpen(index) }" | ||
| :suppressed="isDistractorOpen(index)" | ||
| :aria-label="editDistractorLabel$({ number: index + 1 })" | ||
| @click="openDistractor(index)" | ||
| > | ||
| <TipTapEditor | ||
| :value="choice.content" | ||
| :mode="isDistractorOpen(index) ? 'edit' : 'view'" | ||
| format="html" | ||
| :minHeight="'48px'" | ||
| :autofocus="isDistractorOpen(index)" | ||
| :imageProcessor="EditorImageProcessor" | ||
| :tabindex="-1" | ||
| class="editor" | ||
| @update="html => setDistractorContent(index, html)" | ||
| @minimize="closeOpenTarget" | ||
| /> | ||
| </ClickableRegion> | ||
| <KIconButton | ||
| icon="close" | ||
| size="mini" | ||
| :ariaLabel="deleteDistractorBtn$({ number: index + 1 })" | ||
| :tooltip="deleteDistractorBtn$({ number: index + 1 })" | ||
| :color="$themePalette.grey.v_800" | ||
| @click="onRemoveDistractor(index)" | ||
| /> |
There was a problem hiding this comment.
We need a small gap between the remove button and the RTE content.
There was a problem hiding this comment.
Lets also remove the li's padding, and the border color if the distractor is in edit mode.
There was a problem hiding this comment.
Both done. The gap was only missing while editing — the chip class carried gap: 8px, the editing row had no rule of its own. It now shares gap: 8px with the chip, and drops the padding and the border colour while open, since the editor draws its own.
| :disabled="state.pairs.length <= 1" | ||
| :ariaLabel="deletePairBtn$({ number: index + 1 })" | ||
| :tooltip="deletePairBtn$({ number: index + 1 })" | ||
| :color="$themePalette.grey.v_800" |
There was a problem hiding this comment.
Let's use base/gray/400 as per the specs and size="small".
There was a problem hiding this comment.
Done — $themePalette.grey.v_400, size="small".
| padding: 4px 12px; | ||
| background-color: v-bind('$themeTokens.surface'); | ||
| border: 1px solid; | ||
| border-radius: 4px; |
There was a problem hiding this comment.
Done — 8px on .chip and on .chip-region, the two rules that round a chip. The pool box, pair rows and pair cards are cards rather than chips, so they stay at 4px; say if the specs put those at 8px too.
| const byId = new Map( | ||
| [...flatten(state.value.pairs), ...state.value.distractors].map(c => [c.id, c]), | ||
| ); |
There was a problem hiding this comment.
There may be repeated IDs that are still valid, right? This Map will squash them. However, if the same response appears in two different pairs, they will see the response twice in the chips list.
There was a problem hiding this comment.
Right, the Map was wrong. It now folds by content instead of by id, which is what buildAssociateInteractionXML does — same-content choices become one qti-simple-associable-choice with a match-max. So the preview shows exactly the options delivery renders: a response used in two pairs is one chip that can be used twice, not two chips.
If you meant it should show twice, say so and I will key it by position instead — the change is one line.
| > | ||
| <li | ||
| v-for="choice in shuffledPool" | ||
| :key="choice.id" |
There was a problem hiding this comment.
We cannot use choice.id if id may be repeated across several choices.
There was a problem hiding this comment.
Fixed — keyed by position.
Swept the branch for the same assumption. Three :key on choices: the pair cards and the distractor chips already carried -${position} / -${index}, so only the pool needed changing. Three id-keyed lookups had the defect and are now matched on content: the blank-choice error set, the paired-ids set behind the green border in showAnswers, and the pool dedup above.
| } | ||
|
|
||
| function openDraft() { | ||
| if (props.mode === 'edit') openTarget.value = { kind: 'draft' }; |
There was a problem hiding this comment.
Could we have a small constant for these kinds?
There was a problem hiding this comment.
Done — module-level frozen OpenTarget with PROMPT / PAIR / DISTRACTOR / DRAFT, used by every read and write of openTarget.kind. No other bare-string discriminators in the branch.
| } else if (state.value.pairs.length > 0) { | ||
| openPairItem(0, 0); | ||
| } | ||
| emit('update:interaction', workingInteraction.value); |
There was a problem hiding this comment.
Why? This is different from what we have on other interaction editors, right?
There was a problem hiding this comment.
No change — this is the ordering editor's pattern, OrderingInteractionEditor.vue:309 and :318-321, line for line. TextEntryEditor.vue:345 gates the same way.
The gate is deliberate: a mode="view" preview must not write back to the parent, so the entry emit moved into the mode watcher and the ongoing one is gated. ChoiceInteractionEditor is the odd one out — it emits in view mode too. Covered by the does not emit update:interaction in view mode test.
Happy to align all four either way if you want one rule.
There was a problem hiding this comment.
Let's always emit only in edit mode, and only if the content changed, not just because the mode changed.
There was a problem hiding this comment.
Done. The mode watcher no longer emits; workingInteraction is now compared with isEqual, so an emit needs a real change to bodyXml or responseDeclarations — reopening an editor or retyping the same text stays silent.
Tests follow: mount in edit mode asserts no emit, plus a case for moving the open editor between pair items.
Searched every update:interaction emit under QTIEditor/ — four editors, three others match the pattern: ChoiceInteractionEditor.vue:382 (immediate, no mode gate), OrderingInteractionEditor.vue:309 (mode watcher), TextEntryEditor.vue:349 (immediate). All three are outside this diff, so I filed #6136 rather than widening the PR.
Splits the single flat pool of <qti-simple-associable-choice> elements into authoring state: `pairs` from the correct response, `distractors` from the match-max capacity the correct response does not consume. buildXML re-merges them, normalizing ids so equal content shares one pool entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
26fe418 to
7ce8f7c
Compare
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
113ae56 to
89ce19e
Compare
| class="editor" | ||
| /> | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
Fixed — the pair row now stacks every applicable message instead of showing the first match.
Audited the class rather than the line: all five codes the associate validator emits, against every control the editor reddens (prompt wrapper, pair card, distractor chip).
- One gap:
pairItemStylereddened a pair card forDUPLICATE_DISTRACTOR_CONTENT, butpairErrorMessagenever named it — and it returned one message where two can apply. - No gap on distractors: a blank distractor has no text to duplicate, so
EMPTY_CHOICE_CONTENTandDUPLICATE_DISTRACTOR_CONTENTare mutually exclusive there.
pairErrorMessage(index) is now a pairErrorMessages computed returning an array; the row renders one ValidationMessage per entry. Two new tests: the message reaches the pair row the distractor repeats, and both messages stack on a pair that repeats itself and a distractor.
| <TipTapEditor | ||
| :value="state.prompt" | ||
| :mode="isPromptOpen ? mode : 'view'" | ||
| format="html" | ||
| :minHeight="'80px'" | ||
| :autofocus="isPromptOpen" | ||
| :imageProcessor="EditorImageProcessor" | ||
| :tabindex="-1" | ||
| class="editor" | ||
| @update="setPrompt" | ||
| @minimize="closeOpenTarget" | ||
| /> |
There was a problem hiding this comment.
Fixed in all three. .editor-content now clips at the card's inner edge — the bound .choice-content already gives choice's prompt. The toolbar and popovers are siblings of that box, so they stay whole.
Searched every TipTapEditor in the QTI interaction editors — 11 across choice, ordering, text entry and associate:
- Matched: ordering's prompt, text entry's prompt, and all six in associate.
- Already bounded: choice's two (
.choice-content) and ordering's list items (.item-content).
Associate needed one more thing: its chips and distractor rows are flex items sized to content, so a wide formula grew the chip past the pool box rather than overflowing it. max-width: 100% on .chip / .distractor caps them.
Verified on the demo page with an oversized formula in the associate prompt, a pair item and a distractor, plus the ordering and text entry prompts.
Before:
After:
Registering the descriptor does not populate QUESTION_TYPE_LABELS, so QTIItemEditor gets an explicit ASSOCIATE entry — without it every associate item's view-mode header reads "Unknown type". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A formula is one unbreakable inline node, so a long one is wider than the prompt card and painted over its border. Choice already bounds its prompt this way; ordering and text entry did not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
89ce19e to
f0e8e7f
Compare













Summary
Associate questions could not be authored: an item whose body is
<qti-associate-interaction>had no plugin, so it fell back to the choice descriptor — the header read "Multiple Choice" and the editor rendered no choices at all, because an associate pool holds no<qti-simple-choice>. This adds the plugin end to end — the flat choice pool parses into correct pairs plus distractors, serializes back to acardinality="multiple" base-type="pair"declaration, validates, and renders in an editor.References
Fixes #6101.
Reviewer guidance
The 734 QTI editor Jest tests pass. Beyond those, on the QTI demo page (
/channels/<id>/#/qti-demo, question 7):<qti-value>gains a matching space-separated id couple.Worth questioning, both following from the issue's XML rules 8 and 9:
buildXMLcollapses two choices with equal text into one pool entry and bumps itsmatch-max, so an author who deliberately wants two visually identical options gets one. Blank content is exempt, otherwise a freshly added pair could not round-trip.match-maxminus the choice's appearances in the correct response, so hand-written XML with amatch-maxlarger than intended silently gains distractors on import.Screenshots
axe-core (WCAG 2 AA) on the demo page reports one violation: colour contrast on
AddListItemButton's label. It reproduces on the unchanged ordering editor, which uses the same shared component.Deviations from the issue spec
AI usage
Used Claude Code to implement the plugin test-first against a written plan, following the ordering interaction as the reference. Verified with the Jest suite,
pre-commit, manual QA on the QTI demo page, and an axe-core audit.@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
🟢 Updating PR
Last updated: 2026-09-15 22:05 UTC