Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
apply review suggestions
  • Loading branch information
YunFeng0817 committed Jul 3, 2024
commit d1fba35c38be0c42487aeaaf8554e5cfa42caad9
2 changes: 1 addition & 1 deletion .changeset/cuddly-bikes-fail.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
"rrweb": patch
---

fix: duplicate textContent for style element cause incremental style mutation invalid
fix: duplicate textContent for style elements cause incremental style mutations to be invalid
2 changes: 1 addition & 1 deletion packages/rrweb/src/replay/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1592,7 +1592,7 @@ export class Replayer {
) {
// https://github.com/rrweb-io/rrweb/pull/1417
/**
* If both _cssText and textContent are present for a style element due to some exist bugs, the element will have two child text nodes.
* If both _cssText and textContent are present for a style element due to some existing bugs, the element will have two child text nodes.
* We need to remove the textNode created by _cssText to avoid issue.
*/
for (const cssText of childNodeArray as (Node & RRNode)[]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given childNodeArray is of length 1, couldn't we go directly to
const cssText = childNodeArray[0] as (Node & RRNode) without the for loop?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe I should be asking why you are checking that the length is only one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it a proxy for '_cssText attribute was present'?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I see that the !mirror.hasNode(cssText) check makes sure it's not a previously legitimately added text content

Expand Down
7 changes: 6 additions & 1 deletion packages/rrweb/test/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1287,7 +1287,11 @@ describe('record integration tests', function (this: ISuite) {
document.head.appendChild(incrementalStyle);
incrementalStyle.sheet!.insertRule(`#two { color: ${OldColor}; }`, 0);

await new Promise((resolve) => setTimeout(resolve, 0));
await new Promise((resolve) =>
requestAnimationFrame(() => {
requestAnimationFrame(resolve);
}),
);

// Change the color of the #one div element to yellow as an incremental style mutation
const styleElement = document.querySelector('style')!;
Expand All @@ -1304,6 +1308,7 @@ describe('record integration tests', function (this: ISuite) {
OldColor,
NewColor,
);
await waitForRAF(page);

const snapshots = (await page.evaluate(
'window.snapshots',
Expand Down