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 all 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
39 changes: 29 additions & 10 deletions packages/rrweb/src/replay/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1570,20 +1570,39 @@ export class Replayer {
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)
const prospectiveSiblings = Array.isArray(parent.childNodes)
? 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 prospectiveSiblings) {
if (c.nodeType === parent.TEXT_NODE) {
parent.removeChild(c as Node & RRNode);
}
}
} else if (
parentSn.tagName === 'style' &&
prospectiveSiblings.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 was ending up with two child text nodes
* We need to remove the textNode created by _cssText as it doesn't have an id in the mirror, and thus cannot be further mutated.
*/
for (const cssText of prospectiveSiblings as (Node & RRNode)[]) {
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