Skip to content

Commit 5ce29ff

Browse files
juliechengjuliecheng
committed
fix: skip calling setAttribute on unchanged attr in diffProps
1 parent 53e15c4 commit 5ce29ff

File tree

5 files changed

+216
-7
lines changed

5 files changed

+216
-7
lines changed

‎.changeset/new-turkeys-compare.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"rrdom": patch
3+
---
4+
5+
In Chrome, calling setAttribute on style elements in the document body can cause the browser to drop CSS rules that were previously added to the stylesheet via insertRule. This fix prevents setAttribute from being called when the attribute already exists with the correct value.

‎packages/rrdom/src/diff.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,7 @@ function diffProps(
339339

340340
for (const name in newAttributes) {
341341
const newValue = newAttributes[name];
342+
// eslint-disable-next-line
342343
const sn = rrnodeMirror.getMeta(newTree) as elementNode | null;
343344
if (sn?.isSVG && NAMESPACES[name])
344345
oldTree.setAttributeNS(NAMESPACES[name], name, newValue);
@@ -354,7 +355,9 @@ function diffProps(
354355
} else if (newTree.tagName === 'IFRAME' && name === 'srcdoc') continue;
355356
else {
356357
try {
357-
oldTree.setAttribute(name, newValue);
358+
if (oldTree.getAttribute(name) !== newValue) {
359+
oldTree.setAttribute(name, newValue);
360+
}
358361
} catch (err) {
359362
// We want to continue diffing so we quietly catch
360363
// this exception. Otherwise, this can throw and bubble up to

‎packages/rrdom/test/diff.test.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -489,6 +489,32 @@ describe('diff algorithm for rrdom', () => {
489489
diff(element, rrIframe, replayer);
490490
expect(element.getAttribute('srcdoc')).toBe(null);
491491
});
492+
493+
it('can skip setAttribute when value is unchanged', () => {
494+
const tagName = 'STYLE';
495+
const element = document.createElement(tagName);
496+
const sn = Object.assign({}, elementSn, { tagName });
497+
mirror.add(element, sn);
498+
499+
element.setAttribute('type', 'text/css');
500+
expect(element.getAttribute('type')).toEqual('text/css');
501+
502+
const rrDocument = new RRDocument();
503+
const rrNode = rrDocument.createElement(tagName);
504+
const sn2 = Object.assign({}, elementSn, { tagName });
505+
rrDocument.mirror.add(rrNode, sn2);
506+
rrNode.attributes = { type: 'text/css' };
507+
508+
const setAttributeSpy = vi.spyOn(element, 'setAttribute');
509+
diff(element, rrNode, replayer);
510+
expect(setAttributeSpy).not.toHaveBeenCalled();
511+
512+
rrNode.attributes = { type: 'application/css' };
513+
diff(element, rrNode, replayer);
514+
expect(setAttributeSpy).toHaveBeenCalledWith('type', 'application/css');
515+
516+
setAttributeSpy.mockRestore();
517+
});
492518
});
493519

494520
describe('diff children', () => {

‎packages/rrweb/test/events/inline-style-sheet-rule-events.ts‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
import { EventType, IncrementalSource } from '@rrweb/types';
2+
import type { eventWithTime } from '@rrweb/types';
3+
4+
const now = Date.now();
5+
const events: eventWithTime[] = [
6+
{
7+
type: EventType.DomContentLoaded,
8+
data: {},
9+
timestamp: now,
10+
},
11+
{
12+
type: EventType.Load,
13+
data: {},
14+
timestamp: now + 100,
15+
},
16+
{
17+
type: EventType.Meta,
18+
data: {
19+
href: 'http://localhost',
20+
width: 1000,
21+
height: 800,
22+
},
23+
timestamp: now + 100,
24+
},
25+
// full snapshot:
26+
{
27+
data: {
28+
node: {
29+
id: 1,
30+
type: 0,
31+
childNodes: [
32+
{ id: 2, name: 'html', type: 1, publicId: '', systemId: '' },
33+
{
34+
id: 3,
35+
type: 2,
36+
tagName: 'html',
37+
attributes: { lang: 'en' },
38+
childNodes: [
39+
{
40+
id: 4,
41+
type: 2,
42+
tagName: 'head',
43+
attributes: {},
44+
childNodes: [
45+
{
46+
id: 101,
47+
type: 2,
48+
tagName: 'style',
49+
attributes: {
50+
'data-meta': 'from full-snapshot, gets rule added at 500',
51+
},
52+
childNodes: [
53+
{
54+
id: 102,
55+
type: 3,
56+
isStyle: true,
57+
textContent:
58+
'\n.css-added-at-200-overwritten-at-3000 {\n opacity: 1;\n transform: translateX(0);\n}\n',
59+
},
60+
],
61+
},
62+
{
63+
id: 105,
64+
type: 2,
65+
tagName: 'style',
66+
attributes: {
67+
_cssText:
68+
'.css-added-at-200 { position: fixed; top: 0px; right: 0px; left: 4rem; z-index: 15; flex-shrink: 0; height: 0.25rem; overflow: hidden; background-color: rgb(17, 171, 209); }.css-added-at-200.alt { height: 0.25rem; background-color: rgb(190, 232, 242); opacity: 0; transition: opacity 0.5s ease-in 0.1s; }.css-added-at-200.alt2 { padding-left: 4rem; }',
69+
'data-emotion': 'css',
70+
},
71+
childNodes: [
72+
{ id: 106, type: 3, isStyle: true, textContent: '' },
73+
],
74+
},
75+
],
76+
},
77+
{
78+
id: 107,
79+
type: 2,
80+
tagName: 'body',
81+
attributes: {},
82+
childNodes: [
83+
{
84+
id: 108,
85+
type: 2,
86+
tagName: 'style',
87+
attributes: {
88+
'type': 'text/css',
89+
'gs-style-id': 'gs-id-0',
90+
'_cssText': '.original-style-rule { color: red; }'
91+
},
92+
childNodes: [
93+
{
94+
id: 109,
95+
type: 3,
96+
textContent: '',
97+
isStyle: true
98+
},
99+
],
100+
},
101+
],
102+
},
103+
],
104+
},
105+
],
106+
},
107+
initialOffset: { top: 0, left: 0 },
108+
},
109+
type: EventType.FullSnapshot,
110+
timestamp: now + 100,
111+
},
112+
// mutation that uses insertRule to add a new style rule to the existing body stylesheet
113+
{
114+
data: {
115+
id: 108,
116+
adds: [
117+
{
118+
rule: '.css-inserted-at-500 {border: 1px solid blue;}',
119+
index: 1,
120+
},
121+
],
122+
source: IncrementalSource.StyleSheetRule,
123+
},
124+
type: EventType.IncrementalSnapshot,
125+
timestamp: now + 500,
126+
},
127+
// mutation that adds a child to the body
128+
{
129+
type: EventType.IncrementalSnapshot,
130+
data: {
131+
source: IncrementalSource.Mutation,
132+
texts: [],
133+
attributes: [],
134+
removes: [],
135+
adds: [
136+
{
137+
parentId: 107,
138+
nextId: null,
139+
node: {
140+
type: 2,
141+
tagName: 'div',
142+
attributes: {},
143+
childNodes: [],
144+
id: 110,
145+
},
146+
}
147+
],
148+
},
149+
timestamp: now + 900,
150+
}
151+
];
152+
153+
export default events;

‎packages/rrweb/test/replayer.test.ts‎

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
waitForRAF,
1313
} from './utils';
1414
import styleSheetRuleEvents from './events/style-sheet-rule-events';
15+
import inlineStyleSheetRuleEvents from './events/inline-style-sheet-rule-events';
1516
import movingStyleSheetOnDiff from './events/moving-style-sheet-on-diff';
1617
import orderingEvents from './events/ordering';
1718
import scrollEvents from './events/scroll';
@@ -63,13 +64,13 @@ describe('replayer', function () {
6364
page.on('console', (msg) => console.log('PAGE LOG:', msg.text()));
6465
});
6566

66-
afterEach(async () => {
67-
await page.close();
68-
});
67+
// afterEach(async () => {
68+
// await page.close();
69+
// });
6970

70-
afterAll(async () => {
71-
await browser.close();
72-
});
71+
// afterAll(async () => {
72+
// await browser.close();
73+
// });
7374

7475
it('can get meta data', async () => {
7576
const meta = await page.evaluate(`
@@ -179,6 +180,27 @@ describe('replayer', function () {
179180
await assertDomSnapshot(page);
180181
});
181182

183+
it('should persist inserted StyleSheetRules on fast forward', async () => {
184+
await page.evaluate(`events = ${JSON.stringify(inlineStyleSheetRuleEvents)}`);
185+
const result = await page.evaluate(`
186+
const { Replayer } = rrweb;
187+
const replayer = new Replayer(events);
188+
replayer.pause(0);
189+
190+
// fast forward past the insertedRules at 500 and mutations at 900
191+
replayer.pause(905);
192+
const styleSheets = Array.from(replayer.iframe.contentDocument.styleSheets);
193+
194+
// find the inlineStyleSheet that has the cssRule with the selectorText '.original-style-rule'
195+
const inlineStyleSheet = styleSheets.find(sheet => Array.from(sheet.cssRules).find(rule => rule.selectorText === '.original-style-rule'));
196+
inlineStyleSheet.cssRules.length;
197+
`);
198+
199+
// verify the inserted cssRule didn't get dropped
200+
expect(result).toEqual(2);
201+
await browser.close();
202+
});
203+
182204
it('should persist StyleSheetRule changes when skipping triggers parent style element to move in diff', async () => {
183205
await page.evaluate(`events = ${JSON.stringify(movingStyleSheetOnDiff)}`);
184206

0 commit comments

Comments
 (0)