📱 fix: Show Quote Popup for Block Selections and on Touch Devices (#14777)

* 📱 fix: Show Quote Popup for Block Selections and on Touch Devices

The "Add to chat" popup never appeared for two whole classes of selection.

Block-granularity gestures (triple-click, double-click then word-drag) park
the selection's far boundary at the start of the next block. For a message's
closing block that boundary sits outside `.message-render` — on the composer
wrapper or the following message row — while selecting no text there, so the
anchor/focus equality check suppressed the popup. Triple-clicking any earlier
paragraph worked, which is what made this look like an edge case. The range is
now clamped to the message before the check, and selections that really do
carry visible text from another message are still refused.

Touch platforms could not reach the feature at all. A long-press, and every
drag of the native selection handles, emits no mouse event whatsoever — only
`selectionchange` — while the popup was shown exclusively from mouseup,
dblclick and keyup. Showing now also hangs off a settle-debounced
`selectionchange`, gated so an in-progress mouse drag still cannot flicker it.
Accepting was broken independently: the tap is also the gesture that dismisses
the selection, unmounting the button before `click` could land, so touch
commits on `pointerdown` instead. The desktop mousedown path is deliberately
unchanged, since preventDefault on `pointerdown` can suppress the
compatibility mousedown that click depends on.

Two UX consequences of the same code: scrolling re-anchors the popup rather
than dismissing it on the first event (the chat auto-scrolls constantly while
streaming, and a mobile URL bar collapsing fires resize), and touch selections
place the button below the text, clear of the OS Copy/Share callout, with a
44px tap target.

Covered by six e2e tests — three desktop, three on an emulated Pixel 5 with a
real touchscreen — each verified to fail against the pre-fix build.

* 🩹 fix: Address Review Findings and Repair the Scroll Specs

The two failing e2e shards were a defect in the specs, not the component.
`scrollMessages` reached for `.scrollbar-gutter-stable` with a document-wide
query, but the nav and side panels carry that class too, so it could grab a
sidebar list that never scrolls — 0px moved, and only in CI, where the nav
renders differently. The scroller is now reached from the message itself, the
way `MessageNav` does it. The specs also centre the selection first and nudge
by a quarter of the visible height, so the gesture cannot scroll the selection
clean out of view and then blame the popup for going with it.

Review findings, all in `QuoteButton`:

Visibility was tested against the window, but the list scrolls inside a bounded
container, so text can sit clipped under the header or the composer while its
un-clipped rect is still inside the window — leaving the popup floating over
unrelated UI. It is now clipped to the nearest scrollable ancestor.

Touch committed on the press, so starting a scroll on the button, or touching
it and thinking better of it, still added the quote. The excerpt is captured on
the press and committed on the release, and only when that release lands on the
button, restoring the cancellation every button is expected to have. Commit on
press existed because the tap dismisses the selection before `click` fires;
capturing the text up front keeps that safe, and an in-flight press is no
longer allowed to unmount its own target.

A visible popup also described the previous selection for up to the settle
window, so a tap while dragging a native selection handle queued the stale
excerpt. It is dropped as soon as a differing selection starts settling.

Finally, `viaTouch` survived from the last press into keyboard-driven
selections on hybrid devices, which could flip the popup into the touch layout;
keydown clears it.

The cancel path is covered by a new touch spec, verified to fail against a
commit-on-press build.

* 🧵 fix: Reconcile Cancelled Presses, Widen Clipping, Steady the Scroll Specs

Second review round, with one finding taken on trust and flagged rather than
claimed as proven.

A cancelled touch press could leave the popup backed by a selection that no
longer existed. A press deliberately keeps the button alive through a
collapsing selection so the release has a target to be judged against, but a
cancel then dropped the press without ever honouring the collapse it had
masked, so a later tap could add a dead excerpt. Ending a press without
committing now rechecks the live selection and dismisses if it went away.

Visibility now intersects every clipping ancestor of the message rather than
stopping at the nearest. This one is precautionary, not a proven fix: the
review that prompted it describes scroll containers *inside* a message (a wide
table, a code block) shadowing the outer chat scroller, but the walk starts
from the message element, so those are descendants and were never in the chain.
Behaviour is unchanged in the current layout — a spec covering a table-cell
selection passes identically with and without it — and it is kept only because
intersecting the whole chain stays correct if the list is ever nested inside a
further-clipped panel. The comment says exactly this.

The scroll specs were the real instability. They now move the selection between
two positions that are both on screen instead of nudging by a pixel count:
blind nudges kept pushing it under the composer, where the popup correctly
hides, and the chat's own auto-scroll made the landing spot unpredictable. They
also target the opening paragraph, since the closing one is the last content in
the conversation and cannot be carried upward from a list already at maximum
scroll.

The reply fixture gained a table so a selection inside a nested scroll container
is exercised, and a spec covers the cancelled press.

15/15 pass locally.

* 🪟 fix: Judge Quote-Popup Visibility From the Selection, on Both Axes

Third review round. All three findings held up, and each now has a spec that
fails without its fix.

Clipping is now measured from the selection rather than from the message, and
on both axes. A wide table or a long code line scrolls inside its own container
— and `overflow-x: auto` makes the computed `overflow-y` auto, so it clips
vertically too — which means scrolling it sideways carries the selected text out
of view while the message never moves. Walking up from the message could not see
those containers at all, and a vertical-only test could not see that motion.
This supersedes the previous round's precautionary widening, which was kept
without evidence; the evidence is now a spec that scrolls a table past its own
selection.

Publishing a settled selection also checks visibility. Nothing is tracked during
the 300ms settle interval, so a scroll inside that window never reached the
re-anchoring path, and the reading was published off-screen and then clamped
into view — stranding the popup over unrelated UI.

The cancelled-press spec now reproduces the ordering it describes. Collapsing
the selection and cancelling in one synchronous block let the asynchronous
`selectionchange` arrive after the press had ended, which is the ordinary path
and passes either way; it now waits for delivery in between, so the collapse
lands while the press is still masking it. Two other specs needed the same
scrutiny: `toBeHidden` is satisfied by an element that does not exist yet, so
the settle spec sits out the interval before asserting, and it scrolls just past
the container edge rather than to the end of the conversation, because a violent
scroll re-renders the messages and drops the selection for unrelated reasons.

The reply fixture's table is now wide enough to overflow sideways.

17/17 pass, and each new spec was re-run against a build with its own fix
reverted to confirm it fails there.
This commit is contained in:
Danny Avila 2026-08-13 00:36:45 -04:00 committed by GitHub
parent df6e15a0de
commit 155f71f81a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 1064 additions and 100 deletions

View file

@ -39,6 +39,8 @@ const ASK_USER_QUESTION_MARKER = 'E2E_ASK_USER_QUESTION:';
const RESUME_ICON_REPLY_MARKER = 'E2E_RESUME_ICON_REPLY:';
const FORCED_ERROR_MARKER = 'E2E_FORCED_ERROR:';
const MARKDOWN_REPLY_MARKER = 'E2E_MARKDOWN_REPLY';
/** Two prose paragraphs, so a spec can select the message's *closing* block. */
const PARAGRAPHS_REPLY_MARKER = 'E2E_PARAGRAPHS_REPLY';
const MERMAID_ARTIFACT_REPLY_MARKER = 'E2E_MERMAID_ARTIFACT_REPLY';
const LARGE_MERMAID_ARTIFACT_REPLY_MARKER = 'E2E_LARGE_MERMAID_ARTIFACT_REPLY';
const HTML_ARTIFACT_REPLY_MARKER = 'E2E_HTML_ARTIFACT_REPLY';
@ -432,6 +434,45 @@ function replyResponses(text) {
};
}
if (text.includes(PARAGRAPHS_REPLY_MARKER)) {
/** The quoted cell sits in the first column, so scrolling the table to its
* right edge carries it out of view. */
const wideColumns = [{ header: 'E2E first column header', cell: 'E2E table cell text' }];
for (let index = 1; index < 8; index++) {
wideColumns.push({
header: `E2E column ${index} with a deliberately wide header`,
cell: `E2E filler cell ${index} padding the row out`,
});
}
const filler = [];
for (let index = 0; index < 4; index++) {
filler.push(
`E2E filler paragraph ${index} keeps this reply tall enough to overflow a phone viewport so scrolling is exercised for real.`,
'',
);
}
return {
responses: [
[
'E2E opening paragraph of the reply, ahead of the closing one.',
'',
/** Renders inside `.markdown-table-wrapper`, a nested scroll container:
* its `overflow-x: auto` also makes the computed `overflow-y` auto, so
* a selection here is clipped by the table AND by the message list.
* Wide enough to actually overflow sideways, which is what lets a
* spec scroll the selected cell out of view without moving the
* message at all. */
`| ${wideColumns.map((column) => column.header).join(' | ')} |`,
`| ${wideColumns.map(() => '---').join(' | ')} |`,
`| ${wideColumns.map((column) => column.cell).join(' | ')} |`,
'',
...filler,
'E2E closing paragraph, the last block this message renders.',
].join('\n'),
],
};
}
const errorName = getMarkerValue(text, FORCED_ERROR_MARKER);
if (errorName) {
return {