fix(web-shell): add explicit ::selection for message content in Firefox - #8417
Conversation
Firefox does not paint the default selection highlight for text whose element chain passes through a display:contents element (the data-user-selectable wrapper on MessageItem). The logical selection (copy, selectionchange popup) works fine - only the visual highlight is missing. An explicit ::selection background makes Firefox paint the highlight where the default painting fails. Fixes QwenLM#8214
--selection-bg was never defined in the codebase (only --chat-editor-selection-bg exists in App.module.css). Use a fixed hsl(210 100% 50% / 30%) to avoid confusion.
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Thanks for the PR — this one is well-scoped. Template looks good ✓ Problem: observed bug with solid evidence — #8214 has screenshots and a screen recording from a real Firefox session: selected AI-reply text copies fine but shows no highlight, and clearing cookies / hard refresh / logout / disabling plugins made no difference. The issue thread's static analysis traced it to Firefox not painting the default selection highlight for text whose element chain passes through a Direction: aligned. A human reviewer in #8214 already committed to exactly this fix — "add a defensive explicit Size: not applicable — one CSS file outside core paths (+12/−0). Approach: the scope is exactly right — one rule, scoped to the
Risk: no elevated risk signals — no high-risk paths touched. Moving on to code review. 🔍 中文说明感谢贡献——这个 PR 范围很清晰。 模板完整 ✓ 问题：已观测到的 bug，证据充分——#8214 有真实 Firefox 环境下的截图和录屏：选中的 AI 回复文字可以复制但没有高亮，清 cookie/强制刷新/登出/禁用插件都无效。issue 线程中的静态分析已将其归因于 Firefox 不会为经过 方向：对齐。#8214 中的人工 reviewer 已经明确要做同样的修复——"为可选中的消息区域添加防御性的显式 规模：不适用——核心路径之外的一个 CSS 文件（+12/−0）。 方案：范围恰好——一条规则，限定在
风险：无升级风险信号——未触及高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent baseline first: given only the title and the "why", the fix I'd propose is exactly this one — an explicit Checked against the tree at the reviewed commit:
No blockers found. The only discrepancy is the one flagged at the gate: the PR body describes a Test evidence (PR's own CI, read via API — no PR code executed here)This is an unattended CI run, so nothing was built or run locally. At the reviewed commit:
The only substantive outstanding check is the Ubuntu unit suite; the macOS/Windows test jobs are skipped by matrix gating. Nothing is red so far. The visual preview job passed and reported no screenshot delta — expected, since its scenarios never hold a text selection; read it as "no unintended visual regression in covered states", not as evidence the fix works. Sandboxed verification can only partially settle this one: the central claim — Firefox now paints the highlight — needs a real Firefox session that no CI lane here provides. The visual preview runs Chromium, which already painted the highlight before this fix, so it cannot be the oracle; 中文说明代码审查先给独立基线：只看标题和"为什么需要"，我会给出的修复就是这一个——在 按审查提交点的代码树核对：
无阻塞项。唯一的不一致就是门禁阶段指出的那个：PR 描述提到 测试证据（PR 自身的 CI，通过 API 读取——本次未执行任何 PR 代码）这是无人值守的 CI 运行，本地未构建或运行任何内容。在审查提交点上：Ubuntu 单元测试套件仍在运行（唯一实质性的未完成项），macOS/Windows 测试作业按矩阵门控跳过，目前没有任何红色项。视觉预览作业通过且未报告截图差异——符合预期，因为其场景不会保持文本选中状态；应读作"已覆盖状态无意外视觉回归"，而不是修复生效的证据。 沙箱验证只能部分解决这个 PR 的问题：核心结论——Firefox 现在会绘制高亮——需要真实的 Firefox 环境，而这里没有任何 CI 通道能提供。视觉预览跑在 Chromium 上，而 Chromium 在修复前就能绘制高亮，所以不能当 oracle； — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — clean, minimal fix for a real observed bug; only a stale PR body and the pending Firefox confirmation hold it short of a clean sweep. This is what a good small PR looks like. The problem is real — #8214 comes with screenshots and video of the missing highlight in Firefox — and the fix is exactly the low-risk direction a human reviewer already committed to in that thread: give the selectable message region an explicit The implementation matches the proposal I'd have made before reading the diff: one rule scoped to Two reservations, both non-blocking: the PR body still describes a Approval deferred until CI lands green on 中文说明置信度：4/5 —— 针对真实观测 bug 的干净、最小修复；只有 PR 描述过期和待定的 Firefox 确认两点，让它未到满分。 这是一个优秀小 PR 该有的样子。问题是真实的——#8214 附有 Firefox 中高亮缺失的截图和录屏——而修复正是人工 reviewer 在那个线程里已经认可的低风险方向：给可选中的消息区域一个显式的 实现与我在读 diff 之前会独立给出的方案一致：一条限定在 两点保留意见，均不阻塞：PR 描述仍在说 diff 里并未包含的 批准推迟到 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| * Firefox does not paint the default selection highlight for text whose | ||
| * element chain passes through a `display: contents` element (the | ||
| * `data-user-selectable` wrapper on MessageItem). The logical selection |
There was a problem hiding this comment.
[Suggestion] The PR asserts the display: contents/Firefox paint mechanism as the definitive root cause and claims "Fixes #8214", but the issue thread's maintainer analysis refutes that mechanism as the explanation of the reported symptom: the data-user-selectable wrapper is shared by user and assistant message rows alike (MessageItem.tsx:199), so it cannot cause the reported assistant-only asymmetry — and the floating quick-action bar in the reporter's screenshots (复制/询问 Qwen/解释/翻译) does not exist in this package, which suggests the reporter is on an embedding page whose own CSS is the more likely suppressor. Also, standalone.css is imported only by client/main.tsx and the e2e harness, so the reporter's context likely never loads this stylesheet. The change itself is the maintainer-endorsed defensive workaround ("low-risk and worth doing regardless") — this is about the framing, not the code. — Failure scenario: merging auto-closes #8214 via "Fixes" with zero Firefox verification (the PR admits "N/A - requires Firefox browser environment"), while the reporter's actually-observed behaviour (embedded product, assistant-only, Firefox) is almost certainly unchanged, and the thread's unanswered diagnostic questions stay orphaned.
Suggested fix: reframe the PR description (and optionally the assertive code comment) as the maintainer-endorsed defensive ::selection workaround rather than a confirmed root-cause fix; state that #8214's reported case appears to involve an embedding page whose own CSS may also need fixing; consider not using "Fixes" until the reporter confirms.
中文说明
建议：该 PR 将 display: contents/Firefox 绘制机制断言为确定的根因并声明 "Fixes #8214"，但 issue 讨论中维护者的分析反驳了该机制作为所报告症状的解释：data-user-selectable 包裹层在用户消息与助手消息行中是共用的（MessageItem.tsx:199），因此无法解释"仅 AI 回复"的不对称现象——而且报告者截图中的浮动快捷操作栏（复制/询问 Qwen/解释/翻译）并不存在于本包中，说明报告者使用的是某个嵌入页面，其自身的 CSS 更可能是抑制高亮的原因。此外，standalone.css 仅被 client/main.tsx 和 e2e 测试框架引入，报告者所在的环境很可能根本不会加载该样式表。改动本身是维护者认可的防御性方案（"low-risk and worth doing regardless"）——问题在于表述，而非代码。失败场景：合并后将通过 "Fixes" 自动关闭 #8214，而 PR 承认完全没有在 Firefox 中验证过（"N/A - requires Firefox browser environment"），报告者实际观察到的行为（嵌入产品、仅 AI 回复、Firefox）几乎不会改变，issue 中未得到回答的诊断问题也会被搁置。
建议修复：将 PR 描述（以及语气过于肯定的代码注释）改写为维护者认可的防御性 ::selection 方案，而非已确认的根因修复；说明 #8214 报告的案例似乎涉及某个嵌入页面，其自身 CSS 可能也需要修复；在报告者确认之前，考虑不要使用 "Fixes" 关键字。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| [data-user-selectable] ::selection { | ||
| background: hsl(210 100% 50% / 30%); | ||
| } |
There was a problem hiding this comment.
[Suggestion] Nothing pins this new rule: no unit, DOM, or e2e test asserts it. Unit tests run under JSDOM (no pseudo-element paint) and the Playwright projects are chromium-only, so deleting the rule, renaming/removing data-user-selectable, or breaking the selector would fail zero tests while logical selection keeps working. — Failure scenario: a future standalone.css cleanup/consolidation deletes the rule → zero tests fail → the exact invisible-selection-highlight regression this PR fixes returns, discoverable only via a user report. This review's probe confirmed the deletion mutant IS observable in Chromium: getComputedStyle(el, '::selection').backgroundColor on a [data-user-selectable] descendant flips from rgba(0, 128, 255, 0.3) to rgba(0, 0, 0, 0) — so a discriminating pin is possible at the existing e2e smoke seam (web-shell.smoke.spec.ts already asserts getComputedStyle properties).
Suggested fix: add one assertion at that seam, e.g.
const bg = await page.evaluate(() =>
getComputedStyle(
document.querySelector('[data-user-selectable] *')!,
'::selection',
).backgroundColor,
);
expect(bg).toBe('rgba(0, 128, 255, 0.3)');and note in the test that it pins rule presence/matching, not the Firefox paint effect itself (this repo has no Firefox Playwright project).
中文说明
建议：没有任何测试固定这条新规则——没有单元测试、DOM 测试或 e2e 测试断言它。单元测试运行在 JSDOM 下（不渲染伪元素），Playwright 项目仅包含 chromium，因此删除该规则、重命名/移除 data-user-selectable 或破坏选择器都不会让任何测试失败，而逻辑选区功能依然正常。失败场景：未来某次 standalone.css 的清理/合并删除了该规则 → 没有任何测试失败 → 本 PR 修复的"选区高亮不可见"回归原样复现，只能靠用户报告才能发现。本次评审的探针实验确认该"删除突变体"在 Chromium 中是可观测的：[data-user-selectable] 后代元素上的 getComputedStyle(el, '::selection').backgroundColor 会从 rgba(0, 128, 255, 0.3) 变为 rgba(0, 0, 0, 0)——因此在现有的 e2e smoke 接缝处（web-shell.smoke.spec.ts 已有 getComputedStyle 断言）可以加一个有区分度的固定测试。
建议修复：在该接缝处添加一个断言（见上方英文示例代码），并在测试中注明它固定的是规则的存在/匹配，而不是 Firefox 的绘制效果本身（本仓库没有 Firefox Playwright 项目）。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
Reframe the standalone.css comment and PR description as a defensive workaround, not a confirmed root-cause fix: the data-user-selectable wrapper is shared by user and assistant rows, and the reporter's screenshot shows an embedding-page toolbar this package does not ship. Add a getComputedStyle(..., '::selection') assertion to the smoke e2e so a future cleanup cannot silently drop the rule.
|
@qwen-code /triage |
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| [data-user-selectable] ::selection { | ||
| background: hsl(210 100% 50% / 30%); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The workaround sits at a shallower depth than the defect: the data-user-selectable wrapper ships in the component library (MessageItem.tsx), but this rule lives in standalone.css, which is imported only by client/main.tsx (standalone app) and the e2e harness — the npm-package entry (client/index.tsx via vite.lib.config.ts) never loads it. Verified against this PR's own lib build: the injected component CSS in dist/index.js contains zero occurrences of ::selection or data-user-selectable, while the standalone asset does contain the rule. — Failure scenario: after this merges, a Firefox user selecting an AI reply in any embedded deployment of @qwen-code/web-shell — starting with the reporter of issue 8214, who per this PR's own analysis is on an embedding page — still sees no selection highlight, and the PR description's claim that the change "also makes the selection highlight visible in standalone/embedded deployments" does not hold for embedded consumers.
Suggested fix: move the rule into CSS that ships with the component — e.g. client/styles/globals.css (imported by App.tsx, bundled into the injected component stylesheet). Fix-flip verified by rebuild: the rule then appears in the injected CSS correctly scoped under [data-web-shell-root][data-web-shell-shadcn], and the e2e pin still passes since the standalone app also loads globals.css. Alternatively, amend the PR description to state that the rule covers the standalone app only.
中文说明
建议：该补救措施所处的深度比缺陷本身更浅：data-user-selectable 包裹层随组件库一起发布（MessageItem.tsx），但这条规则位于 standalone.css，而该文件仅被 client/main.tsx（独立应用）和 e2e 测试框架引入 —— npm 包入口（client/index.tsx，经由 vite.lib.config.ts）从不加载它。已在本 PR 自己的 lib 构建产物中验证：dist/index.js 注入的组件 CSS 中 ::selection 与 data-user-selectable 的出现次数均为零，而 standalone 产物中则包含该规则。失败场景：合并之后，在任何嵌入部署 @qwen-code/web-shell 的页面上（首先是 issue 8214 的报告者——按本 PR 自己的分析，其正使用某个嵌入页面），Firefox 用户选中 AI 回复文字时依然看不到选区高亮；且 PR 描述中“该改动也能让 standalone/embedded 部署显示选区高亮”的说法对嵌入式使用者并不成立。
建议修复：把规则移到随组件一起发布的 CSS 中——例如 client/styles/globals.css（由 App.tsx 引入，会被打包进注入的组件样式表）。已通过重建验证：移动后该规则以正确的 [data-web-shell-root][data-web-shell-shadcn] 作用域出现在注入 CSS 中，且由于 standalone 应用同样加载 globals.css，e2e 固定测试仍然通过。或者，修改 PR 描述，说明该规则仅覆盖 standalone 应用。
— qwen3.8-max via Qwen Code /review (v0.21.4)
The defensive ::selection rule for [data-user-selectable] message content was in standalone.css, which is only loaded by the standalone app entry (client/main.tsx) and the e2e harness. The npm package entry (client/index.tsx via vite.lib.config.ts) never loads standalone.css, so embedded deployments of @qwen-code/web-shell - including the reporter of QwenLM#8214 - did not receive the rule and still saw no selection highlight. Move it to globals.css, which is imported by App.tsx and WebShellTranscript.tsx and therefore ships with the component-scoped stylesheet. Verified against the lib build: the rule now appears in dist/index.js correctly scoped under [data-web-shell-root][data-web-shell-shadcn]. The standalone app also loads globals.css, so the e2e smoke pin still passes. Addresses the review finding on standalone.css:119.
|
@qwen-code /review |
|
The It fails inside I verified it fails on #8387's own commit ( Two prior commits on this branch ( Could a maintainer either re-run CI (it may also be order-dependent) or merge |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.3)
| * This rule lives in the component-scoped stylesheet (loaded by App.tsx and | ||
| * WebShellTranscript.tsx) rather than standalone.css so it ships with the |
There was a problem hiding this comment.
[Suggestion] The PR description is stale: "What changed" still says the rule is added to standalone.css ("Added one CSS rule to standalone.css" / "在 standalone.css 中添加一条 CSS 规则"), but this round moved it to globals.css — verified at HEAD: the rule lives just below this comment, and standalone.css contains no ::selection rule at all.
Failure scenario: a maintainer verifying the round-3 move from the PR description checks the wrong file; since this repo squash-merges, the commit message built from this body permanently records the wrong stylesheet, misleading anyone later bisecting or changelogging the ::selection rule.
Suggested fix: update both language sections of the PR description's "What changed" to say the rule was moved from standalone.css to client/styles/globals.css so it ships with the npm package. (PR body edit only — no code change needed.)
中文说明
PR 描述已过时："改动"部分仍说规则加在 standalone.css（"Added one CSS rule to standalone.css" / "在 standalone.css 中添加一条 CSS 规则"），但本轮已把规则移到 globals.css —— 已在 HEAD 核实：规则就在本注释下方，standalone.css 中已无任何 ::selection 规则。
失败场景：维护者按 PR 描述去验证第 3 轮的移动时会查错文件；本仓库使用 squash 合并，由该描述生成的提交信息会把错误的样式表永久记录下来，误导后续 bisect 或编写 changelog 的人。
建议修复：更新 PR 描述两个语言版本的"改动"部分，说明规则已从 standalone.css 移到 client/styles/globals.css，以便随 npm 包发布。（仅需修改 PR 描述，无需改代码。）
— qwen3.8-max via Qwen Code /review (v0.21.4)
| const content = document.querySelector( | ||
| '[data-user-selectable] *', | ||
| ) as Element | null; |
There was a problem hiding this comment.
[Suggestion] The assertion samples only the first [data-user-selectable] wrapper — probe-confirmed to be the user row in this fixture (wrapperCount=2; the first match is "Hello from replay") — while the rule under test spans every message row, user and assistant alike.
Failure scenario: a future edit that narrows the rule to user rows keeps this assertion green while assistant rows silently lose the selection highlight — a re-recurrence of the exact #8214 symptom on half the rows the rule exists to protect.
Suggested fix (the change spans both this block and the expect below it, so no one-click suggestion): query all wrappers and assert every one:
const selectionBackgrounds = await page.evaluate(() => {
const wrappers = Array.from(
document.querySelectorAll('[data-user-selectable]'),
);
return wrappers.map((wrapper) => {
const content = wrapper.querySelector('*');
if (!content) return null;
return getComputedStyle(content, '::selection').backgroundColor;
});
});
expect(selectionBackgrounds.length).toBeGreaterThan(1);
for (const background of selectionBackgrounds) {
expect(background).toBe('rgba(0, 128, 255, 0.3)');
}中文说明
该断言只采样了第一个 [data-user-selectable] 包装元素 —— 经探针实测确认是本 fixture 中的用户消息行（wrapperCount=2，首个匹配是 "Hello from replay"）—— 而被测规则覆盖所有消息行（用户与 AI 回复都包括）。
失败场景：未来若有人把规则收窄到只覆盖用户行，此断言仍然为绿，而 AI 回复行会悄悄失去选区高亮 —— 恰好在这条规则要保护的一半行上复发 #8214 的症状。
建议修复（改动同时涉及此代码块和下方的 expect，因此不用一键 suggestion）：查询所有包装元素并逐一断言（代码见英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| if (!content) return null; | ||
| return getComputedStyle(content, '::selection').backgroundColor; | ||
| }); | ||
| expect(selectionBackground).toBe('rgba(0, 128, 255, 0.3)'); |
There was a problem hiding this comment.
[Suggestion] Nothing asserts this rule survives in the npm lib bundle — the deployment this round exists to fix. This smoke pin runs against the Vite dev server, which loads BOTH globals.css (App.tsx import chain) and standalone.css (via main.tsx) unscoped, while the lib entry chain imports only globals.css. Flagged independently by three review lenses; the counterfactual was reproduced with a probe: with the rule moved back to standalone.css, this smoke test still passes (1 passed, 6.3s) yet dist/index.js loses the rule entirely — and build-artifact.test.ts still passes 10/10.
Failure scenario: if the rule moves back to standalone.css (the round-2 state this commit corrects), or a future scopeComponentCss change drops or invalidates it, the published package silently loses the rule and embedded deployments — the stated target of this round and the #8214 reporter's environment — regress to no selection highlight while all CI stays green.
Suggested fix: add an existence assertion in client/build-artifact.test.ts (which already parses the injected CSS): expect a rule whose selector contains both [data-user-selectable] and ::selection. Assert rule presence by selector substring — build-artifact.test.ts runs in node with no browser, so a computed-style or source-color-text equality is not the right shape here.
中文说明
没有任何测试断言这条规则能存活在 npm lib 产物中 —— 而那正是本轮修改要覆盖的部署形态。这条 smoke 断言跑在 Vite dev server 上，该环境会同时加载未加作用域的 globals.css（App.tsx 导入链）和 standalone.css（经 main.tsx）；而 lib 入口的导入链只包含 globals.css。三个独立的审查视角都发现了这一点；反事实场景已用探针复现：把规则移回 standalone.css 后，本 smoke 测试仍然通过（1 passed, 6.3s），但 dist/index.js 中已完全没有该规则 —— 且 build-artifact.test.ts 依然 10/10 全绿。
失败场景：若规则被移回 standalone.css（即本提交所纠正的第 2 轮状态），或未来 scopeComponentCss 的改动丢弃/破坏了该规则，发布出去的 npm 包会悄悄失去这条规则，嵌入式部署（本轮的目标场景，也是 #8214 报告者的环境）将再次出现无选区高亮的问题，而所有 CI 仍是绿的。
建议修复：在 client/build-artifact.test.ts（已在解析注入的 CSS）中加一条存在性断言：期望存在选择器同时包含 [data-user-selectable] 和 ::selection 的规则。用选择器子串断言规则存在即可 —— build-artifact.test.ts 在 node 中运行、没有浏览器环境，计算样式或源码颜色文本相等都不是合适的形式。
— qwen3.8-max via Qwen Code /review (v0.21.4)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.8-max via Qwen Code /review (v0.21.3)
Address review findings on the round-3 move to globals.css: - The smoke e2e only sampled the first [data-user-selectable] row (the user row in this fixture). Assert the rule on every selectable row so a future narrowing to user rows keeps assistant rows covered. - Nothing asserted the rule survives in the npm lib bundle - the deployment this fix exists for. Add a build-artifact test that parses the injected component CSS in dist/index.js and pins the scoped [data-user-selectable] ::selection rule under [data-web-shell-root].
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| // The fixture renders both a user and an assistant message, so there must | ||
| // be at least two selectable rows and every one must carry the rule. | ||
| expect(selectionBackgrounds.length).toBeGreaterThanOrEqual(2); |
There was a problem hiding this comment.
[Suggestion] The assertion counts element descendants matched by [data-user-selectable] *, not the [data-user-selectable] wrapper rows themselves, so it does not enforce the "at least two selectable rows" invariant the comment claims — one user row alone renders 4+ element descendants. — Failure scenario: a future MessageItem.tsx edit drops/bypasses the selectableSafeBody wrapper for one role (e.g. assistant rows) → that row's nodes leave the querySelectorAll sample, the remaining row still yields >= 2 descendants, the length check and per-node color loop both stay green, and the row silently loses the ::selection highlight in Firefox — a re-recurrence of the #8214 symptom this pin was written to catch. Probe-confirmed on the real component: removing the assistant wrapper's attribute leaves descendants=4 with the assertion still green, while counting wrappers flips 2 → 1.
| // The fixture renders both a user and an assistant message, so there must | |
| // be at least two selectable rows and every one must carry the rule. | |
| expect(selectionBackgrounds.length).toBeGreaterThanOrEqual(2); | |
| // The fixture renders both a user and an assistant message, so there must | |
| // be at least two selectable rows and every one must carry the rule. | |
| expect(selectionBackgrounds.length).toBeGreaterThanOrEqual(2); | |
| expect( | |
| await page.locator('[data-user-selectable]').count(), | |
| ).toBeGreaterThanOrEqual(2); |
中文说明
建议：该断言统计的是 [data-user-selectable] * 匹配到的元素后代节点，而不是 [data-user-selectable] 包装行本身，因此并未落实注释所声称的“至少两个可选区行”这一不变量——仅一个用户行就能渲染出 4 个以上的元素后代节点。失败场景：未来对 MessageItem.tsx 的修改移除/绕过了某一角色（如助手行）的 selectableSafeBody 包装 → 该行的节点会离开 querySelectorAll 的采样集合，而剩余行仍能提供 >= 2 个后代节点，长度检查与逐节点颜色循环都保持为绿，该行在 Firefox 中悄悄失去 ::selection 高亮——恰好复发本固定测试要防范的 #8214 症状。已在真实组件上探针验证：移除助手行包装的属性后 descendants=4，断言仍为绿；改为统计包装数则由 2 → 1，可捕获该回归。
— qwen3.8-max via Qwen Code /review (v0.21.5)
…escendants
Per review: querySelectorAll('[data-user-selectable] *') counts element
descendants, not the wrapper rows themselves - a single user row renders
4+ descendants, so the >=2 invariant did not actually enforce that both
roles are present. Match the [data-user-selectable] wrappers directly and
sample one descendant per row.
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| let matched: Rule | undefined; | ||
| postcss.parse(readInjectedCss()).walkRules((rule) => { | ||
| if (rule.selector.includes('data-user-selectable] ::selection')) { | ||
| matched = rule; | ||
| } | ||
| }); | ||
| expect( | ||
| matched, | ||
| '::selection rule for [data-user-selectable] missing from lib bundle', | ||
| ).toBeDefined(); | ||
| expect(matched?.selector).toContain('[data-web-shell-root]'); | ||
| expect( | ||
| matched?.nodes.some((n) => n.type === 'decl' && n.prop === 'background'), | ||
| ).toBe(true); |
There was a problem hiding this comment.
[Suggestion] R6-1: The lib-bundle pin matches the rule by an exact selector substring (including the space before ::selection) and an exact declaration prop name (background), coupling the test to the workaround's current notation instead of its effect — probe-verified against the real built artifact: rewriting the declaration to background-color: fails the prop assertion with an unannotated expected false to be true while the e2e pin stays green; removing the space from the selector fails with "rule missing from lib bundle" although the rule is still present. — Failure scenario: a maintainer makes an innocuous notation edit (e.g. background: → background-color:, the property CSS Pseudo-Elements-4 lists for ::selection); the rule still ships and the e2e pin still passes, but this test fails with a misleading message, so the two pins added by this PR give contradictory signals and the maintainer debugs a nonexistent bundling regression.
| let matched: Rule | undefined; | |
| postcss.parse(readInjectedCss()).walkRules((rule) => { | |
| if (rule.selector.includes('data-user-selectable] ::selection')) { | |
| matched = rule; | |
| } | |
| }); | |
| expect( | |
| matched, | |
| '::selection rule for [data-user-selectable] missing from lib bundle', | |
| ).toBeDefined(); | |
| expect(matched?.selector).toContain('[data-web-shell-root]'); | |
| expect( | |
| matched?.nodes.some((n) => n.type === 'decl' && n.prop === 'background'), | |
| ).toBe(true); | |
| let matched: Rule | undefined; | |
| postcss.parse(readInjectedCss()).walkRules((rule) => { | |
| if ( | |
| rule.selector.includes('[data-user-selectable]') && | |
| rule.selector.includes('::selection') | |
| ) { | |
| matched = rule; | |
| } | |
| }); | |
| expect( | |
| matched, | |
| '::selection rule for [data-user-selectable] missing from lib bundle', | |
| ).toBeDefined(); | |
| expect(matched?.selector).toContain('[data-web-shell-root]'); | |
| expect( | |
| matched?.nodes.some( | |
| (n) => | |
| n.type === 'decl' && | |
| (n.prop === 'background' || n.prop === 'background-color'), | |
| ), | |
| ).toBe(true); |
中文说明
lib bundle 固定测试通过精确的选择器子串（包括 ::selection 前的空格）和精确的声明属性名（background）来匹配规则，使测试耦合于 workaround 的当前写法而非其效果——已对真实构建产物做探针验证：将声明改写为 background-color: 时，属性断言以无注释的 expected false to be true 失败，而 e2e 固定测试仍然通过；去掉选择器中的空格时，会报出误导性信息 "rule missing from lib bundle"，而规则实际仍在 bundle 中。— 失败场景：维护者做一次无害的写法调整（例如 background: → background-color:，即 CSS Pseudo-Elements-4 为 ::selection 列出的属性）；规则仍然生效、e2e 固定测试仍然通过，但本测试以误导性信息失败——本 PR 新增的两个固定测试给出矛盾信号，维护者会去排查一个并不存在的打包回归。
— qwen3.8-max via Qwen Code /review (v0.21.5)
…tion Per review (R6-1): the pin matched an exact selector substring (including the space) and an exact prop name, coupling to the current notation. A maintainer switching 'background' to 'background-color' (the CSS Pseudo-Elements-4 name) would fail this test with a misleading message while the e2e pin stayed green. Match the two selector halves independently and accept either prop name.
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.8-max via Qwen Code /review (v0.21.5)
|
Released in v0.21.6. |
3 participants
What this PR does
Adds an explicit
::selectionCSS rule for message content ([data-user-selectable] ::selection) in the component-scoped stylesheet (globals.css), a low-risk defensive workaround for the missing selection highlight reported in #8214.Why it's needed
Firefox is suspected of not painting the default selection highlight for text whose element chain passes through a
display: contentselement. The web shell wraps every message body in<div data-user-selectable="true" style="{{ display: 'contents' }}">, so reply text can be copied but may show no visual highlight when selected. Chromium paints it correctly.This is not a confirmed root-cause fix: the
display: contentswrapper is shared by user and assistant rows alike, so it cannot by itself explain the reported assistant-only asymmetry. The reporter's screenshots show a floating action bar (复制 / 询问 Qwen / 解释 / 翻译) that does not exist in this package, suggesting they run the web shell on an embedding page whose own CSS may be the actual suppressor. An explicit background on::selectionis the maintainer-endorsed "low-risk and worth doing regardless" defensive fix (see #8214 triage).What changed
The rule lives in
packages/web-shell/client/styles/globals.css(imported byApp.tsxandWebShellTranscript.tsx), notstandalone.css— so it ships with the npm package's injected component stylesheet and applies to embedded deployments of@qwen-code/web-shell, not just the standalone app. (An earlier round had it instandalone.css, which the lib entry never loads; the review caught that.)Coverage:
getComputedStyle(..., '::selection')assertion in the web-shell smoke e2e test pins the rule on every[data-user-selectable]row (user and assistant alike), not just the first.build-artifact.test.tsassertion pins that the rule survives in the lib bundle (dist/index.js), correctly scoped under[data-web-shell-root][data-web-shell-shadcn]— the deployment this fix exists for.Reviewer Test Plan
How to verify
Evidence (Before & After)
N/A - requires Firefox browser environment, not available in CI. The reporter of #8214 can verify. Note the reporter's observed environment appears to be an embedding page, so their specific case may need a fix there too.
Tested on
Risk & Scope
Linked Issues
Related to #8214
中文说明
这个 PR 做了什么
为消息内容添加显式
::selectionCSS 规则，放在组件作用域样式表globals.css中，作为 #8214 中选区高亮不显示问题的低风险防御性方案。为什么需要
Firefox 被怀疑在选区经过
display: contents元素时不绘制默认高亮。Web Shell 把每条消息体包裹在<div data-user-selectable="true" style="{{ display: 'contents' }}">中，所以回复文字可以复制但选中时可能不显示高亮。Chromium 正常。这不是已确认的根因修复：
display: contents包装在用户与 AI 消息中是共用的，无法单独解释报告中的"仅 AI 回复"不对称。报告者截图中的浮动操作栏（复制 / 询问 Qwen / 解释 / 翻译）不存在于本包中，说明他们很可能在一个嵌入页面上使用，其自身 CSS 才可能是真正的抑制者。显式的::selection背景是维护者认可的"低风险、无论如何都值得做"的防御性修复。改动
规则放在
packages/web-shell/client/styles/globals.css（由App.tsx和WebShellTranscript.tsx引入），不是standalone.css——这样它会随 npm 包的注入组件样式表一起发布，对@qwen-code/web-shell的嵌入部署生效，而不只是独立应用。（早期版本曾放在standalone.css，但 lib 入口不加载它，review 指出了这点。）测试覆盖：smoke e2e 用
getComputedStyle(..., '::selection')断言每个[data-user-selectable]行（user 和 assistant 都包含）；build-artifact.test.ts断言规则在 lib bundle（dist/index.js）中存活且正确 scoped。风险与范围
关联 Issue
Related to #8214