feat(QTI): add Gap Match interaction viewer - #15214
Conversation
|
👋 Hi @habibayman, 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. |
Build Artifacts
Smoke test screenshot |
marcellamaki
left a comment
There was a problem hiding this comment.
The interactions here are looking good, with drag and drop, click and tap, and keyboard navigation. Also, the scoring is working! (Whether or not the made up Claude-generated questions are accurate is a separate consideration; they are not. But I did review the scoring schema, and when I did what it said, the score was rendering correctly in the sandbox, so that question from our conversation earlier this week is resolved).
I'm going to assign rtibblesbot (sorry!! I know you have been enjoying the 'fewer than 80 comments' lifestyle 😉 ) just in case there are any things I missed (it's a very large diff!). Please if there is any feedback, address it after your vacation. I still consider this project successfully completed 🎉
🟡 Waiting for changesLast updated: 2026-08-21 22:05 UTC |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15214 implements every acceptance criterion in #15213 — depth-first gap discovery, click/drag/keyboard placement, the symmetric match-group predicate, pool-first pair order, max-associations, qti-input-width-N — and the useMatchRows pair-order work is a genuinely good abstraction. One blocking gap: a refused placement is completely silent, where the sibling MatchInteraction announces it. That matters most for max-associations, which QTI defaults to 1 for gap match, so a spec-legal item can dead-end the learner with no highlight and no message.
CI passing. Manual QA did not run in this pass — the UI was not visually verified, and the PR video/screenshot were not treated as design-conformance evidence.
- blocking: refused placements are silent (
GapMatchInteraction.vue) - suggestions: review-mode gaps have no accessible name; the pool's exhausted state is the one consumer that skips
isCompatible;dir="auto"is dropped on chip content; the response pool is now a third verbatim copy; candidate/target affordances are colour-only - nitpicks:
inheritAttrs: falsediscards authored attributes;shuffleuntested; the spec reads Vue's_providedinternals
@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
| if (!isCompatible(gapIds[gapIndex], identifier)) { | ||
| return; | ||
| } | ||
| place(identifier, gapIndex, 0); |
There was a problem hiding this comment.
blocking: every refusal path here is silent. placeInGap returns on an incompatible match-group, place() no-ops past a limit, and selectResponse won't even select an exhausted chip — nothing is rendered to say why.
The max-associations case has no visual explanation either. This interaction defaults maxAssociations to 1 (line 531), so on a spec-legal item that omits the attribute, filling one gap makes isOfferTarget false for every gap: selecting a response highlights nothing, clicking a gap does nothing, and no message appears. The spec pins that state at GapMatchInteraction.spec.js:339, and the sv-3 fixture prose asks for the opposite ("a Delivery Platform must trigger its default max-associations message").
MatchInteraction.vue:182-210,340-353,592-606 already has the pattern — a refusal ref set by the single place path, atMaxAssociations, and renderNotice() into a role="status" live region — and its four refused* strings are reusable. If the intent was that the qti-gap-target highlight substitutes for a notice, that covers group and occupancy refusals but not max-associations, where the highlight vanishes everywhere at once.
| ], | ||
| style: styles.value, | ||
| attrs: { | ||
| 'aria-label': label, |
There was a problem hiding this comment.
suggestion: in review mode this gap has no accessible name. useSlotListbox.slotAttrs returns {} when disabled (useSlotListbox.js:155-158), so the span renders with no role — aria-label is not exposed on a generic element — and its only child is the chip, which GapMatchInteraction.vue:420 marks aria-hidden unconditionally. An empty gap renders no children at all.
A learner reviewing a submitted answer hears "Now is the of our discontent": neither their response nor the fact that there was a blank. The interactive path is fine because role="listbox" supports naming. Cheapest fix: stop hiding the chip once interactive is false, and give an empty gap a visually-hidden text node.
| attrs: { 'aria-label': responsePoolLabel$() }, | ||
| }, | ||
| pool.value.map(identifier => { | ||
| const exhausted = !isPlaceable(identifier); |
There was a problem hiding this comment.
suggestion: exhausted comes straight from useMatchRows.isPlaceable, which knows nothing about match-group — so the pool's greying-out is a fourth consumer of "can this go anywhere?" that disagrees with the keyboard candidate list, the click highlight and the drag accepts check.
Narrow failure: a choice with match-max="2" whose match-group names a single gap it already occupies. canPlace succeeds against some other, incompatible gap, so the chip stays lit and draggable while every gap refuses it. A local wrapper is enough: gapIds.some((id, i) => isCompatible(id, choice) && canPlace(choice, i, 0)).
| }, | ||
| on, | ||
| }, | ||
| [...contentByIdentifier[identifier]], |
There was a problem hiding this comment.
suggestion: authored choice content loses dir="auto" on its way into a chip. GapText.vue:11/GapImg.vue set it on their root span, but both are never mounted — line 102 reads vnode.componentOptions.children, i.e. the children inside the qti-gap-text vnode, so that root span is never created. The chip here has no dir.
InlineChoiceInteraction.vue:18-20,37-41 sets dir="auto" on each element that actually renders authored text, with a test at InlineChoiceInteraction.spec.js:169-176. For an Arabic or Hebrew item, pool chips, in-gap chips and listbox option text render in the app's direction. One-liner: add dir: 'auto' to the attrs object here. (MatchInteraction/AssociateInteraction share the gap, so not a regression — but the new files look like it was handled.)
| interactive, | ||
| }); | ||
|
|
||
| function renderPool() { |
There was a problem hiding this comment.
suggestion: the response pool is now its third verbatim copy. renderPool (434-494), renderChip (368) and chipStyles (337) are line-for-line MatchInteraction.vue:415-446,540-587 and AssociateInteraction's equivalents, differing only in the class prefix and isCandidateForActiveEntry → isCandidateForActiveGap. The SCSS matches too, down to the $chip-* variables and the object-fit: scale-down comment; only $chip-max-size differs. responsePoolLabel is likewise the third local definition of the same string (AssociateInteraction.vue:21, MatchInteraction.vue:22) — three Crowdin entries for one UI label.
That is the Rule of Three. A useResponsePool({ pool, isPlaceable, interactive, selectedIdentifier, isCandidate, onSelect }) composable plus a shared _chip.scss parameterised on max-size removes ~120 duplicated lines, and gives the dir="auto" fix and the refusal notice one place to live. useSlotListbox.js already sets the precedent for exporting shared strings next to shared behaviour.
| } | ||
| if (active.value || target.value) { | ||
| return { | ||
| backgroundColor: $themeBrand.primary.v_50, |
There was a problem hiding this comment.
suggestion: "which gaps will take what I'm holding" is carried by colour alone — qti-gap-target/qti-gap-active change only backgroundColor/borderColor here, with no class-level shape or weight change, and qti-gap-match-chip-candidate (GapMatchInteraction.vue:352-358) changes border and text colour only. qti-gap-refusing does better (opacity + aria-disabled), and -chip-selected adds font-weight: 600. A border-width or border-style change on the target/candidate classes settles WCAG 1.4.1 without new colour pairs.
Related: pool chips are <span>s with a click handler, no role and no tabindex, so their aria-disabled reaches nothing. Keeping the pool out of the tab order is defensible (the gap listbox is the keyboard path), but the click affordance still has no role. MatchInteraction has this too — worth fixing for all three if the pool gets extracted.
| // component placeholder's attrs after the component itself has rendered — | ||
| // so an authored `class` would replace this one's own root class rather | ||
| // than joining it. Take the attrs over and merge the class in by hand. | ||
| inheritAttrs: false, |
There was a problem hiding this comment.
nitpick: the comment explains why attrs are taken over, and class is merged at line 73, but nothing else survives. QTI allows id, lang, dir and label on qti-gap, and an authored id would break any <label for>/aria-describedby the item body points at it. attrs: { ...attrs, 'aria-label': label, ... } (class still handled separately) keeps the merge intent without silently discarding the rest.
| expect(gap.className).not.toMatch(/qti-input-width/); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
nitpick: no gap-match fixture sets shuffle and the spec never exercises it, so "the response pool shuffles, the passage and gap order never do" rests on inspection. The failure it would catch is silent: gapIds is document order and choiceIds is the shuffled set, and swapping them into sourceIds/targetIds renumbers every gap without breaking a current test.
| function draggableUniverse(scope) { | ||
| let vm = findRegions().find(region => !scope || scope.contains(region.$el)); | ||
| while (vm) { | ||
| const provided = vm._provided || {}; |
There was a problem hiding this comment.
nitpick: vm._provided plus a duck-typed symbol scan is two layers past a public surface — a Vue-internal field and a module-private provide key. Any change to Vue or useDraggableUniverse makes this return null and carry() throw on universe.isDragging, pointing at gap match rather than at what moved. useDraggableRegion already returns { handleStart, handleEnd, canAccept } "exposed for unit tests" but DraggableRegion doesn't surface them; doing so would give every interaction spec a stable seam (MatchInteraction.spec.js:231-243 reaches for the same thing).
| * `qti-match-interaction` authors its pairs source-first, `qti-gap-match-interaction` | ||
| * authors them pool-choice-first. | ||
| */ | ||
| export const PAIR_ORDER = Object.freeze({ |
There was a problem hiding this comment.
praise: funnelling pair order through one toPair/fromPair inside the composable, rather than transposing at the interaction's call sites, makes it impossible to apply on write and forget on read — and the scores through the mapping test pins it via the fixture's qti-mapping keys, where a transposition scores 0 instead of 3.
Summary
This PR adds the QTI Gap Match interaction.
Gap match asks a learner to fill blanks in a passage from a pool of words. The fixtures for it already existed but I added extra examples for the answer specific pools.
What it does
qti-gap-match-interactionwith a response pool above the passage and inline blanks in the textAnswer-specific distractor pools
A gap can be fed by its own set of words instead of the whole pool. Picking up a word highlights the gaps that would take it
Behavior description:
qti-answer-specific-pools.webm
Notable details
Gap.vuetakes its attributes over rather than inheriting them. An inherited class replaces the component's own root class instead of joining it, which would break both the gap styling and the width classes.qti-input-width-Nsizes a blank to roughly N charactersReferences
Reviewer guidance
http://localhost:8000/en/learn/#/qti_sandboxbut these examples don't do all what the name implies, some of these were dropped:
AI usage
All unit tests are written by Claude in addition to the extra fixture I added