Add inline choice chip and Insert action to the passage editor by rtibblesbot · Pull Request #6277 · learningequality/studio

rtibblesbot · 2026-10-02T08:10:58Z

Summary

InlineChoiceNode chip; undoable updateInlineChoice
Insert: blank chip, or chip from selection
Red chips for dropdowns in errorResponseIdentifiers
@tiptap/pm now a direct dependency

References

Fixes #6181. Builds on #6176; used by #6182.

Reviewer guidance

Unmounted until [QTI] Inline choice interaction editor #6182; tested in jsdom with the real editor.

Manual QA

QA team: blocked until #6182. In "Evidence", is the open red chip label readable?

QA steps

Insert unfocused → open "0 Add answers" chip at end.
Highlight " Moon " in "The Moon rises", Insert → "The [chip] rises", chip "1 Moon", open.
Selection across paragraphs → Insert disabled.
Arrow onto a chip → not opened; Backspace removes it; Ctrl+Z restores options and correct answer.
Tab reaches each chip; Enter/Space opens it.
Copy-paste a chip → new ids; cut-paste → same ids.

Evidence

Validation errors on chips

Step	Screenshot
Before: grey label
After: dark label; cleared errors turn chips green

Read-only, then editable passage

Step	Screenshot
Read-only: dashed chips, no toolbar
Editable: Insert in toolbar, chip open

More captures (1)

Step	Screenshot
RTL chip layout (older label)

AI usage

Claude Code implemented and self-reviewed this test-first.

🤖 Generated with Claude Code

Deviations from the issue spec

Criterion	Why not
Formula in a converted selection is deleted	Kept; blank chip goes after it
Grey "Add answer" label when options have text	Reviewer asked for "Add answers" always

Written by rtibblesbot, an LLM-based coding agent.

	🟡 Waiting for feedback Last updated: 2026-10-06 23:48 UTC

AlexVelezLl

Some changes to the requirements and a couple of things found while testing it. Claude helped in finding more specific Prose Mirror concerns.

AlexVelezLl · 2026-10-02T20:58:56Z

+        ({ tr, state, dispatch }) => {
+          const found = findChip(state.doc, responseIdentifier);
+          if (!found) return false;
+          const attrs = { ...found.node.attrs, ...changes };


blocking: This merges changes as given, so an option without an id stays id: ''. The tests make that supported input ("keeps the chip ids when an added option has no id yet" asserts the empty id is kept). But nothing downstream fills it: buildInlineChoiceInteractionXML → renameRepeatedIds says "Missing ids are left missing" (parse.js), and line 43 here already notes "the save leaves it missing". The saved item then carries <qti-inline-choice identifier="">, which the server's XSD check rejects ('' is not a valid value of the atomic type; I ran it through validate_qti_item). The whole sync of that item fails, and getDropdowns also hands validation an option error with id: ''.

Since the node is the owner of this data (#6181), I'd have the command keep its invariants: give any option without an id generateRandomSlug('choice'), as insertInlineChoice and paste already do. options: [] has the same problem (the XSD needs at least one <qti-inline-choice>, #6180 rule 8), so the command should refuse it rather than rely on #6182 never sending it.

Fixed in 85e7194: updateInlineChoice now gives an option without an id generateRandomSlug('choice') and returns false for options: []; it also only takes options and correctId from changes.

Searched every path that puts option ids into a chip: insertInlineChoice (generates), transformPasted/appendTransaction (rename missing ids), updateInlineChoice (was the gap). 3 matched, 1 changed. updateInlineChoice is the only one taking caller-supplied options, so the empty-list guard is needed only there. The test asserting the empty id is now "gives an added option with no id a fresh one"; the undo and cut-paste tests that relied on it now load the id-less option from HTML.

AlexVelezLl · 2026-10-02T20:58:57Z

+        },
+        // Catches what `transformPasted` cannot: a drop, and `insertContent` (the toolbar Paste
+        // button). An inserted chip whose ids are still in use elsewhere was copied, not moved.
+        appendTransaction(transactions, oldState, newState) {


suggestion: This is the subtlest code in the PR: an inserted chip whose response id existed before is a copy only if another chip still holds that id. Only QA steps 7–8 cover the drop path (drag-move vs Ctrl+drag). It doesn't need a simulated drag to test. Dispatching a transaction that inserts a copy of an existing chip (expect fresh ids, correct choice remapped) and one that deletes and reinserts it elsewhere in the same transaction (expect ids kept) exercises exactly this branch.

Added in 85e7194 ("inserted by a transaction"): one dispatch inserts a copy of a chip still in the passage (fresh ids, correct choice remapped); one deletes and reinserts it in the same transaction (ids kept). The moved chip holds an id-less option, so only the "response id existed before and no other chip holds it" branch keeps its ids — checked by disabling that branch, which fails the test.

AlexVelezLl · 2026-10-02T20:58:57Z

+    padding: 0 8px;
+    cursor: pointer;
+    background-color: v-bind('$themePalette.grey.v_50');
+    border: 1px dashed v-bind('$themePalette.grey.v_700');


suggestion (design): The solid/dashed difference in the design means open vs closed, not "has a correct answer vs doesn't":

Closed: dashed green border, no fill. The label is bold, in the normal text colour when it shows the correct answer (revolves).

Open (the chip whose options panel is showing): solid green border with a light green fill.

Both states: the badge is a filled green circle with the number in white, and the "Add answers" placeholder label is grey ($themeTokens.annotation).

For the colours, $themeTokens.correct for the border and badge, and the light step of $themePalette.green for the open fill. That replaces the grey/primary styling, and .has-answer no longer drives the border.

Done in e11b29b: closed = dashed $themeTokens.correct border, no fill; open = solid border, $themePalette.green.v_100 fill; badge filled correct with textInverted number; label bold, placeholder in annotation. .has-answer is gone. The same states turn red ($themeTokens.error, red.v_100) for a chip in the new error list from the updated spec.

No capture: no screen mounts the passage editor until #6182, and the QA server serves a static bundle built at startup, so a temporary mount could not be loaded.

AlexVelezLl · 2026-10-02T20:58:57Z

+      {{ label }}
+    </span>
+    <!-- The toolbar Copy button copies the node view's DOM, minus `data-copy-ignore`. -->
+    <span


suggestion: This works, and the comment explains why: the toolbar Copy in useToolbarActions.js scrapes the selection's DOM rather than using ProseMirror's clipboard serializer. But it means every node view with data has to mirror its own serialized HTML inside itself. Not for this PR, but worth a follow-up issue to have the toolbar Copy use editor.view.serializeForClipboard(selection.content()). Then this span, and the math-field special case there, could go.

Agreed it is worth doing, but it changes the TipTapEditor toolbar rather than anything #6178 covers, so I have not filed it from this branch; flagged for a maintainer to file.

Please file a follow-up issue for this @rtibblesbot so that it can be tested in Isolation.

Filed as #6296.

AlexVelezLl · 2026-10-06T18:44:20Z

+
+  <NodeViewWrapper
+    as="span"
+    role="img"


suggestion (spec change, not something this PR missed): #6181 has been updated with keyboard and focus behaviour that wasn't written down when this was built. A chip now opens a focus-trapping options panel (#6182), so it's an interactive control rather than an image. See #6181 → The Change → "The chip is a button", and the new Accessibility criteria. In short:

Render a real <button type="button"> inside the node view, carrying the accessible name, aria-haspopup="dialog" and an aria-expanded that is true while its panel is open. That replaces role="img" and aria-current. It has to be a real BUTTON: Tiptap's default NodeView.stopEvent only hands events to INPUT/BUTTON/SELECT/TEXTAREA targets, so Enter and Space then reach the button instead of ProseMirror.

Each chip is one Tab stop in reading order. Click, Enter or Space opens it.

Arrowing the caret onto a chip must not select or open it, and Enter/Space with the caret on it must not open it either; see the comment at lines 87–91.

Expose a way to focus a chip's button by response identifier, so [QTI] Inline choice interaction editor #6182 can return focus to it when the panel closes (Escape or close).

Insert now opens the new chip (previously "selects it"); that's the same moment, with the panel taking focus.

Done in e11b29b:

Node view renders <button type="button"> with the accessible name, aria-haspopup="dialog", aria-expanded; role="img" and aria-current removed.

Click, Enter or Space on the button opens it; one Tab stop per chip in reading order (tested).

useInlineChoicePassage returns focusChip(responseIdentifier); it focuses the editor when the chip is gone.

Contract renamed to match: openResponseIdentifier / openDropdown (was selected…/selectDropdown); Insert opens the new chip.

Also from the updated spec: useInlineChoicePassage({ errorResponseIdentifiers }) turns listed chips red and adds "needs attention" to their name.

No capture: nothing mounts the passage editor until #6182, and the QA server bundle is static.

AlexVelezLl · 2026-10-06T18:49:20Z

+      const hasAnswer = computed(() => correctText.value !== '');
+      const label = computed(() => {
+        if (hasAnswer.value) return correctText.value;
+        return optionCount.value ? addAnswer$() : addAnswers$();


Let's just always have "Add answers".

Done in e11b29b: label is "Add answers" whenever there is no correct answer; addAnswer string removed. Issue text updated to match.

AlexVelezLl · 2026-10-06T19:11:58Z

+      watch(
+        () => props.selected && isNodeSelected(),
+        selected => selected && select(),
+        { immediate: true },
+      );


suggestion (spec): Moving the caret onto a chip should neither select nor open it, and Enter or Space with the caret on a chip shouldn't open it either. A chip opens only from its button: Tab to it and press Enter or Space, or click it. So this watch, and isNodeSelected with it, can go: nothing should react to the chip becoming node-selected. Arrowing just moves the caret past a chip like any other inline object, and Enter there behaves as Enter normally does in the editor.

Done in e11b29b: the watch, isNodeSelected and the selected/getPos props are gone. Tests now assert that ArrowRight/ArrowLeft onto a chip node-selects it without opening it, and that Enter or Space with the chip node-selected does not open it.

- InlineChoiceNode: inline atom holding responseIdentifier, options and correctId; renders only the attributes getDropdowns reads - updateInlineChoice command, undoable: gives id-less options a fresh id, refuses an empty option list, nulls a correctId naming no option - Pasted or copied chips whose ids already exist get fresh response and choice ids - InlineChoiceChip node view: a button with aria-haspopup=dialog and aria-expanded, option-count badge, "Add answers" label, open/closed styling; an error list turns it red - Opens only on click, Enter or Space on the button, not on the caret - In a read-only passage, a non-interactive image with the same name - useInlineChoicePassage: extensions, Insert action that opens the new chip, provides the open chip and focusChip to the options panel (learningequality#6182) - Add @tiptap/pm as a direct dependency Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

rtibblesbot marked this pull request as ready for review October 2, 2026 09:15

rtibblesbot requested review from a team and rtibbles October 2, 2026 09:15

rtibblesbot force-pushed the issue-6181-538178 branch 2 times, most recently from dd17504 to 8222255 Compare October 2, 2026 10:24

rtibbles assigned AlexVelezLl Oct 2, 2026

AlexVelezLl requested changes Oct 6, 2026

View reviewed changes

rtibblesbot mentioned this pull request Oct 6, 2026

[RTE] Toolbar Copy should copy the selection's serialized content, not its DOM #6296

Open

6 tasks

rtibblesbot force-pushed the issue-6181-538178 branch from e11b29b to 1431100 Compare October 6, 2026 22:37

rtibblesbot force-pushed the issue-6181-538178 branch from 1431100 to 4fa187d Compare October 6, 2026 23:48

Conversation

rtibblesbot commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

QA steps

Evidence

Validation errors on chips

Read-only, then editable passage

AI usage

Deviations from the issue spec

🟡 Waiting for feedback

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rtibblesbot commented Oct 2, 2026 •

edited

Loading