-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(web-shell): add explicit ::selection for message content in Firefox #8417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6f7a027
66fbd94
334a868
b918dfd
0a28f6f
d4e93da
e804620
870d759
2a21e9a
5ebdd5d
a48f3c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -40,6 +40,30 @@ test('loads replayed transcript and connects to fake daemon @smoke', async ({ | |||||||||||||||||||
| await expect(page.locator('[data-web-shell-message-list]')).toContainText( | ||||||||||||||||||||
| 'Hello from fake daemon', | ||||||||||||||||||||
| ); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // #8214: pin the explicit ::selection rule on message content. This | ||||||||||||||||||||
| // asserts the rule is present and matches every [data-user-selectable] | ||||||||||||||||||||
| // wrapper row (user and assistant alike), not just the first one; it | ||||||||||||||||||||
| // does not verify the Firefox paint effect itself (this repo's Playwright | ||||||||||||||||||||
| // projects are chromium-only). | ||||||||||||||||||||
| const selectionBackgrounds = await page.evaluate(() => { | ||||||||||||||||||||
| // Match the wrapper rows themselves, not their descendants - a single | ||||||||||||||||||||
| // row renders many descendant elements, so counting descendants does | ||||||||||||||||||||
| // not enforce the "both roles present" invariant. | ||||||||||||||||||||
| const rows = document.querySelectorAll('[data-user-selectable]'); | ||||||||||||||||||||
| return Array.from(rows, (row) => { | ||||||||||||||||||||
| // ::selection applies to the element's text content; sample the first | ||||||||||||||||||||
| // text-bearing descendant (or the row itself if it has none). | ||||||||||||||||||||
| const target = row.querySelector('*') ?? row; | ||||||||||||||||||||
| return getComputedStyle(target, '::selection').backgroundColor; | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| // 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); | ||||||||||||||||||||
|
Comment on lines
+61
to
+63
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The assertion counts element descendants matched by
Suggested change
中文说明建议:该断言统计的是 — qwen3.8-max via Qwen Code /review (v0.21.5) |
||||||||||||||||||||
| for (const bg of selectionBackgrounds) { | ||||||||||||||||||||
| expect(bg).toBe('rgba(0, 128, 255, 0.3)'); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| test('submits a prompt and renders a streamed assistant response @smoke', async ({ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -132,3 +132,25 @@ | |
| *::-webkit-scrollbar-thumb:hover { | ||
| background: var(--scrollbar-thumb-hover); | ||
| } | ||
|
|
||
| /* | ||
| * Defensive selection-highlight rule for message content. #8214. | ||
| * | ||
| * Firefox is suspected of not painting 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 may fail. This is a low-risk | ||
| * workaround, not a confirmed root-cause fix: the wrapper is shared by user | ||
| * and assistant rows alike, and the reporter's environment appears to be an | ||
| * embedding page, so its own CSS may also need adjustment. | ||
| * | ||
| * This rule lives in the component-scoped stylesheet (loaded by App.tsx and | ||
| * WebShellTranscript.tsx) rather than standalone.css so it ships with the | ||
|
Comment on lines
+149
to
+150
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The PR description is stale: "What changed" still says the rule is added to 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 Suggested fix: update both language sections of the PR description's "What changed" to say the rule was moved from 中文说明PR 描述已过时:"改动"部分仍说规则加在 失败场景:维护者按 PR 描述去验证第 3 轮的移动时会查错文件;本仓库使用 squash 合并,由该描述生成的提交信息会把错误的样式表永久记录下来,误导后续 bisect 或编写 changelog 的人。 建议修复:更新 PR 描述两个语言版本的"改动"部分,说明规则已从 — qwen3.8-max via Qwen Code /review (v0.21.4) |
||
| * npm package and applies to embedded deployments, not just the standalone | ||
| * app. | ||
| */ | ||
| [data-user-selectable] ::selection { | ||
| background: hsl(210 100% 50% / 30%); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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 tobackground-color:fails the prop assertion with an unannotatedexpected false to be truewhile 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.中文说明
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)