Skip to content

fix(trace-viewer): store snapshot style text in an attribute - #42405

Merged
Yury Semikhatsky (yury-s) merged 7 commits into
microsoft:mainfrom
yury-s:fix-snapshot-style-escaping
Aug 27, 2026
Merged

fix(trace-viewer): store snapshot style text in an attribute#42405
Yury Semikhatsky (yury-s) merged 7 commits into
microsoft:mainfrom
yury-s:fix-snapshot-style-escaping

Conversation

@yury-s

@yury-sYury Semikhatsky (yury-s) commented Aug 25, 2026

Copy link
Copy Markdown
Member

Summary

  • A literal </style> in recorded stylesheet text closed the <style> element early, so the trailing content was reparsed as markup and the snapshot rendered incorrectly.
  • Store stylesheet text in a __playwright_style_content__ attribute and restore it as textContent in the snapshot script, where it is never HTML-parsed.
  • This makes escapeURLsInStyleSheet unnecessary, so url() values now round-trip verbatim instead of being percent-encoded.

Stylesheet text nodes were emitted into the snapshot unescaped, so a
literal "</style>" inside style content would close the <style> element
early and the trailing text would be reparsed as markup. Escape it with
a CSS backslash ("\/" is still "/" in CSS) so the stylesheet is preserved
verbatim and renders correctly.
@github-actions

This comment has been minimized.

Comment threadpackages/isomorphic/trace/snapshotRenderer.ts Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Comment threadtests/library/snapshot-renderer.spec.ts Outdated
Escape the fully serialized <style> text as one unit rather than per text
node, so a "</style>" split across adjacent text nodes cannot reassemble
after join and terminate the element. Add an end-to-end trace-viewer test
that records a real trace and asserts the style text stays contained.
Comment threadpackages/isomorphic/trace/snapshotRenderer.ts Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Comment threadpackages/isomorphic/trace/snapshotRenderer.ts Outdated
Render stylesheet text into __playwright_style_content__ on the <style>
element and restore it as textContent in the snapshot script. Content in
an attribute cannot terminate the element, so escapeURLsInStyleSheet and
the closing tag escaping are no longer needed and url() values round-trip
verbatim instead of being percent-encoded.
@yury-sYury Semikhatsky (yury-s) changed the title fix(trace-viewer): escape "</style>" in snapshot stylesheet textfix(trace-viewer): store snapshot style text in an attributeAug 27, 2026
Comment threadpackages/isomorphic/trace/snapshotRenderer.ts
The snapshotter always emits STYLE as [name, {}, cssText], so there is no
need to render children and join them back. Fall back to empty text when
a malformed trace has no text child, to keep rendering non-throwing.
Pass the attribute name and value as separate parts, matching how the
attribute loop above emits every other attribute.
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

🔴 The one hard failure is this PR's own new test, failing on Firefox

Hi, I'm the Playwright bot and I took a first look at the CI failures.

trace-viewer.spec.ts:887 › should not let style text break out of the style element — the test this PR adds — fails on ubuntu-22.04 (firefox - Node.js 20), and it failed on every retry, so it's deterministic, not a flake. It's brand new here, so it can't be pre-existing. Everything else is green; the single flaky entry is unrelated network noise that passed on retry.

Details

Overall: one real, PR-caused failure on Firefox plus one unrelated pre-existing flake. Every other job (all three engines across ubuntu/windows/macos, test-runner shards, installation) is green.

Caused by this PR

  • [firefox-library] › tests/library/trace-viewer.spec.ts:887 › should not let style text break out of the style element — the test the PR introduces. It times out at await traceViewer.snapshotFrame('Click') (locator.click: Test timeout … exceeded) and failed on all 4 attempts, i.e. deterministic on Firefox, not flaky. Since the PR adds this test, it can't be a pre-existing flake by definition. The change reworks how <style> text is stored/restored in snapshots (moving it into a __playwright_style_content__ attribute and reassigning element.textContent in snapshotScript); the new assertion doesn't hold under Firefox's trace-viewer snapshot rendering, so the frame never reaches the expected state and the snapshot click hangs. This needs a look before merge — either the restore path behaves differently on Firefox, or the test needs a browser gate.

Pre-existing flake / infra

  • [firefox-page] › tests/page/page-event-request.spec.ts:181 › should return response body when Cross-Origin-Opener-Policy is set — flaky in this same report: page.goto timed out once and it passed on retry (counted as flaky, not failed). It's a network/COOP test that this snapshot-rendering change doesn't touch, so it's unrelated noise rather than a regression.

Triaged by the Playwright bot - agent run

The body only held a display:none <style> and an empty <div>, so it had
zero height and the click never found a visible target on Firefox.
@github-actions

Copy link
Copy Markdown
Contributor

Test results for "tests 1"

4 flaky⚠️ [chromium-library] › library/beforeunload.spec.ts:130 › should support dismissing the dialog multiple times `@frozen-time-library-chromium-linux`
⚠️ [chromium-page] › page/page-screenshot.spec.ts:723 › page screenshot animations › should stop animations that happen right before screenshot `@realtime-time-library-chromium-linux`
⚠️ [chromium-library] › library/video.spec.ts:699 › screencast › should capture full viewport on hidpi `@chromium-ubuntu-22.04-node22`
⚠️ [firefox-page] › page/page-emulate-media.spec.ts:144 › should keep reduced motion and color emulation after reload `@firefox-ubuntu-22.04-node20`

51327 passed, 1240 skipped


Merge workflow run.

@github-actions

Copy link
Copy Markdown
Contributor

Test results for "MCP"

1 failed
❌ [firefox] › mcp/cli-core.spec.ts:248 › click in iframe @mcp-windows-latest-firefox

8258 passed, 1361 skipped


Merge workflow run.

@yury-s
Yury Semikhatsky (yury-s) merged commit 20e2f50 into microsoft:mainAug 27, 2026
44 of 45 checks passed
@yury-s
Yury Semikhatsky (yury-s) deleted the fix-snapshot-style-escaping branch August 27, 2026 22:19
@github-actions

Copy link
Copy Markdown
Contributor

🟢 The one hard failure is a pre-existing Firefox flake — this PR is clear

Hi, I'm the Playwright bot and I took a look at the CI failures.

The latest reports have a single real failure: [firefox] › mcp/cli-core.spec.ts:248 › click in iframe on mcp-windows-latest-firefox (run 33121958562). That test is a known Firefox-on-Windows flake, unrelated to this PR — which only touches trace-viewer snapshot rendering. Your own new trace-viewer test, which failed on Firefox earlier, now passes.

Details

This PR changes packages/isomorphic/trace/snapshotRenderer.ts (how <style> text is stored/restored in snapshots) and adds one trace-viewer test. Nothing here exercises the MCP CLI's click-in-iframe path.

Pre-existing flake / infra

  • [firefox] › mcp/cli-core.spec.ts:248 › click in iframe (@mcp-windows-latest-firefox) — pre-existing flake. Across the test-results DB it failed 5 of 702 runs (~0.7%) on this bot, always with the same signature expect(received).toBeTruthy() / Received: undefined. The other failures are on SHAs this PR can't be responsible for — main-branch pushes (fcc765f, 66c5cc9, c623920) and an unrelated PR (fix(recorder): don't record duplicate goto for repeated navigation signal #42027). Firefox-Windows only; the same test is green on every other browser/OS. Recent run history on this bot:

    …🟩🟩🟩🟩🟩🟩🟩🟩🟩🟥

Resolved (was caused by this PR, now green)

  • [firefox-library] › library/trace-viewer.spec.ts:887 › should not let style text break out of the style element — the test this PR adds. It timed out on Firefox in earlier commits (runs 32907115199, 33109193688, 33112826430) but passes on the latest commit (run 33118969599), and the newest tests 1 report is clean. No action needed.

Triaged by the Playwright bot - agent run

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@yury-s@pavelfeldman@dcrousso@dgozman