Skip to content
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.

Sign up for GitHub

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

fix: duplicate textContent for style element cause incremental style mutation invalid #1417

Merged
merged 19 commits into from
Jul 8, 2024
Merged

fix: duplicate textContent for style element cause incremental style mutation invalid #1417

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
4272390
fix: style element corner case
YunFeng0817 Feb 23, 2024
0c90863
Merge branch 'master' into fix-styleelement-replay-bug
YunFeng0817 Jun 12, 2024
8c5a69f
Apply formatting changes
YunFeng0817 Jun 12, 2024
c294538
fix: extension doesn't work after vite bump
YunFeng0817 Jun 12, 2024
5d18b46
Merge remote-tracking branch 'origin/master' into fix-styleelement-re…
YunFeng0817 Jun 17, 2024
8825d53
create an integration test to reproduce the issue
YunFeng0817 Jun 17, 2024
3e00f09
remove duplicate fix
YunFeng0817 Jun 18, 2024
a83dc52
improve test case and improve fix
YunFeng0817 Jun 18, 2024
9331f17
Merge remote-tracking branch 'origin/master' into fix-styleelement-re…
YunFeng0817 Jun 18, 2024
226bd9b
fix
YunFeng0817 Jun 18, 2024
9256f7e
changeset
YunFeng0817 Jun 18, 2024
55dbb38
Merge branch 'master' into fix-styleelement-replay-bug
YunFeng0817 Jul 3, 2024
71245d7
remove redundant text nodes for script node
YunFeng0817 Jul 3, 2024
d1fba35
apply review suggestions
YunFeng0817 Jul 3, 2024
1e8a670
Copy the bad data generated by the 'should record style mutations and…
eoghanmurray Jul 8, 2024
5211578
Rename for clarity
eoghanmurray Jul 8, 2024
d778c77
Tweak color to be explicit about which one we intend to keep
eoghanmurray Jul 8, 2024
2a91f6f
Eoghan's tweaks to comments
eoghanmurray Jul 8, 2024
02a50bc
Apply formatting changes
eoghanmurray Jul 8, 2024
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
6 changes: 6 additions & 0 deletions .changeset/cuddly-bikes-fail.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"rrweb-snapshot": patch
"rrweb": patch
---

fix: duplicate textContent for style elements cause incremental style mutations to be invalid
3 changes: 2 additions & 1 deletion packages/rrweb-snapshot/src/rebuild.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,9 @@ function buildNode(
value = adaptCssForReplay(value, cache);
}
if ((isTextarea || isRemoteOrDynamicCss) && typeof value === 'string') {
node.appendChild(doc.createTextNode(value));
// https://github.com/rrweb-io/rrweb/issues/112
// https://github.com/rrweb-io/rrweb/pull/1351
node.appendChild(doc.createTextNode(value));
n.childNodes = []; // value overrides childNodes
continue;
}
Expand Down
37 changes: 28 additions & 9 deletions packages/rrweb/src/replay/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,7 +920,7 @@
sn?.type === NodeType.Element &&
sn?.tagName.toUpperCase() === 'HTML'
) {
const { documentElement, head } = iframeEl.contentDocument!;

Check warning on line 923 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L923 

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
this.insertStyleRules(
documentElement as HTMLElement | RRElement,
head as HTMLElement | RRElement,
Expand All @@ -939,14 +939,14 @@
};

buildNodeWithSN(mutation.node, {
doc: iframeEl.contentDocument! as Document,

Check warning on line 942 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L942 

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
mirror: mirror as Mirror,
hackCss: true,
skipChild: false,
afterAppend,
cache: this.cache,
});
afterAppend(iframeEl.contentDocument! as Document, mutation.node.id);

Check warning on line 949 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L949 

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.

for (const { mutationInQueue, builtNode } of collected) {
this.attachDocumentToIframe(mutationInQueue, builtNode);
Expand Down Expand Up @@ -1061,7 +1061,7 @@
const canvas = document.createElement('canvas');
const ctx = canvas.getContext('2d');
const imgd = ctx?.createImageData(canvas.width, canvas.height);
ctx?.putImageData(imgd!, 0, 0);

Check warning on line 1064 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L1064 

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
}
}
private async deserializeAndPreloadCanvasEvents(
Expand Down Expand Up @@ -1382,7 +1382,7 @@
// Only apply virtual dom optimization if the fast-forward process has node mutation. Because the cost of creating a virtual dom tree and executing the diff algorithm is usually higher than directly applying other kind of events.
if (this.config.useVirtualDom && !this.usingVirtualDom && isSync) {
this.usingVirtualDom = true;
buildFromDom(this.iframe.contentDocument!, this.mirror, this.virtualDom);

Check warning on line 1385 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L1385 

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
// If these legacy missing nodes haven't been resolved, they should be converted to virtual nodes.
if (Object.keys(this.legacy_missingNodeRetryMap).length) {
for (const key in this.legacy_missingNodeRetryMap) {
Expand Down Expand Up @@ -1499,7 +1499,7 @@
// If the parent is attached a shadow dom after it's created, it won't have a shadow root.
if (!hasShadowRoot(parent)) {
(parent as Element | RRElement).attachShadow({ mode: 'open' });
parent = (parent as Element | RRElement).shadowRoot! as Node | RRNode;

Check warning on line 1502 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L1502 

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
} else parent = parent.shadowRoot as Node | RRNode;
}

Expand Down Expand Up @@ -1570,20 +1570,39 @@
if (
parentSn &&
parentSn.type === NodeType.Element &&
parentSn.tagName === 'textarea' &&
mutation.node.type === NodeType.Text
eoghanmurray marked this conversation as resolved.
Show resolved Hide resolved
) {
const childNodeArray = Array.isArray(parent.childNodes)
eoghanmurray marked this conversation as resolved.
Show resolved Hide resolved
? parent.childNodes
: Array.from(parent.childNodes);
// This should be redundant now as we are either recording the value or the childNode, and not both
// keeping around for backwards compatibility with old bad double data, see

// https://github.com/rrweb-io/rrweb/issues/745
// parent is textarea, will only keep one child node as the value
for (const c of childNodeArray) {
if (c.nodeType === parent.TEXT_NODE) {
parent.removeChild(c as Node & RRNode);
if (parentSn.tagName === 'textarea') {
// This should be redundant now as we are either recording the value or the childNode, and not both
// keeping around for backwards compatibility with old bad double data, see

// https://github.com/rrweb-io/rrweb/issues/745
// parent is textarea, will only keep one child node as the value
for (const c of childNodeArray) {
if (c.nodeType === parent.TEXT_NODE) {
parent.removeChild(c as Node & RRNode);
}
}
} else if (
parentSn.tagName === 'style' &&
childNodeArray.length === 1
) {
// https://github.com/rrweb-io/rrweb/pull/1417
/**
* If both _cssText and textContent are present for a style element due to some existing bugs, the element will have two child text nodes.
eoghanmurray marked this conversation as resolved.
Show resolved Hide resolved
* We need to remove the textNode created by _cssText to avoid issue.
eoghanmurray marked this conversation as resolved.
Show resolved Hide resolved
*/
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

if (
cssText.nodeType === parent.TEXT_NODE &&
!mirror.hasNode(cssText)
) {
target.textContent = cssText.textContent;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this always going to be the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should raise a warning instead in case these are not the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be the same, but I’m not entirely sure if there might be any corner cases that could cause issues. I believe that _cssText should always contain the most accurate style content, so I use that value to avoid any potential problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've tweaked the color in d778c77 to show which one we intend to use.
I don't think a warning is worth it; this fixup is for 'historic' bad data and the only case we know is that they are both the same

parent.removeChild(cssText);
}
}
}
} else if (parentSn?.type === NodeType.Document) {
Expand Down
218 changes: 218 additions & 0 deletions packages/rrweb/test/__snapshots__/integration.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15851,6 +15851,224 @@ exports[`record integration tests > should record shadow doms polyfilled by synt
]"
`;

exports[`record integration tests > should record style mutations and replay them correctly 1`] = `
"[
{
\\"type\\": 4,
\\"data\\": {
\\"href\\": \\"about:blank\\",
\\"width\\": 1920,
\\"height\\": 1080
}
},
{
\\"type\\": 2,
\\"data\\": {
\\"node\\": {
\\"type\\": 0,
\\"childNodes\\": [
{
\\"type\\": 1,
\\"name\\": \\"html\\",
\\"publicId\\": \\"\\",
\\"systemId\\": \\"\\",
\\"id\\": 2
},
{
\\"type\\": 2,
\\"tagName\\": \\"html\\",
\\"attributes\\": {
\\"lang\\": \\"en\\"
},
\\"childNodes\\": [
{
\\"type\\": 2,
\\"tagName\\": \\"head\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n\\\\t \\",
\\"id\\": 5
},
{
\\"type\\": 2,
\\"tagName\\": \\"style\\",
\\"attributes\\": {
\\"_cssText\\": \\"#one { color: rgb(255, 0, 0); }\\"
},
\\"childNodes\\": [
{
\\"type\\": 3,
\\"textContent\\": \\"#one { color: rgb(255, 0, 0); }\\",
\\"isStyle\\": true,
\\"id\\": 7
}
],
\\"id\\": 6
},
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\",
\\"id\\": 8
},
{
\\"type\\": 2,
\\"tagName\\": \\"script\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 3,
\\"textContent\\": \\"SCRIPT_PLACEHOLDER\\",
\\"id\\": 10
}
],
\\"id\\": 9
}
],
\\"id\\": 4
},
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\",
\\"id\\": 11
},
{
\\"type\\": 2,
\\"tagName\\": \\"body\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n\\\\t \\",
\\"id\\": 13
},
{
\\"type\\": 2,
\\"tagName\\": \\"div\\",
\\"attributes\\": {
\\"id\\": \\"one\\"
},
\\"childNodes\\": [],
\\"id\\": 14
},
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\",
\\"id\\": 15
},
{
\\"type\\": 2,
\\"tagName\\": \\"div\\",
\\"attributes\\": {
\\"id\\": \\"two\\"
},
\\"childNodes\\": [],
\\"id\\": 16
},
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n\\\\t \\",
\\"id\\": 17
},
{
\\"type\\": 2,
\\"tagName\\": \\"script\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 3,
\\"textContent\\": \\"SCRIPT_PLACEHOLDER\\",
\\"id\\": 19
}
],
\\"id\\": 18
},
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\\\n \\",
\\"id\\": 20
}
],
\\"id\\": 12
}
],
\\"id\\": 3
}
],
\\"id\\": 1
},
\\"initialOffset\\": {
\\"left\\": 0,
\\"top\\": 0
}
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [],
\\"removes\\": [],
\\"adds\\": [
{
\\"parentId\\": 4,
\\"nextId\\": null,
\\"node\\": {
\\"type\\": 2,
\\"tagName\\": \\"style\\",
\\"attributes\\": {
\\"_cssText\\": \\"#two { color: rgb(255, 0, 0); }\\"
},
\\"childNodes\\": [],
\\"id\\": 21
}
},
{
\\"parentId\\": 21,
\\"nextId\\": null,
\\"node\\": {
\\"type\\": 3,
\\"textContent\\": \\"#two { color: rgb(255, 0, 0); }\\",
\\"isStyle\\": true,
\\"id\\": 22
}
}
]
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 13,
\\"id\\": 6,
\\"set\\": {
\\"property\\": \\"color\\",
\\"value\\": \\"rgb(255, 255, 0)\\"
},
\\"index\\": [
0
]
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 13,
\\"id\\": 21,
\\"set\\": {
\\"property\\": \\"color\\",
\\"value\\": \\"rgb(255, 255, 0)\\"
},
\\"index\\": [
0
]
}
}
]"
`;

exports[`record integration tests > should record webgl canvas mutations 1`] = `
"[
{
Expand Down
Loading
Loading