Skip to content

fix: skip calling setAttribute on unchanged attr in diffProps #1691

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
5 changes: 5 additions & 0 deletions .changeset/new-turkeys-compare.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"rrdom": patch
---

In Chrome, calling setAttribute('type', 'text/css') on style elements that already have it causes Chrome to drop CSS rules that were previously added to the stylesheet via insertRule. This fix prevents setAttribute from being called if the attribute already exists.
4 changes: 3 additions & 1 deletion packages/rrdom/src/diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,9 @@ function diffProps(
} else if (newTree.tagName === 'IFRAME' && name === 'srcdoc') continue;
else {
try {
oldTree.setAttribute(name, newValue);
if (oldTree.getAttribute(name) !== newValue) {
oldTree.setAttribute(name, newValue);
}
} catch (err) {
// We want to continue diffing so we quietly catch
// this exception. Otherwise, this can throw and bubble up to
Expand Down
26 changes: 26 additions & 0 deletions packages/rrdom/test/diff.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,32 @@ describe('diff algorithm for rrdom', () => {
diff(element, rrIframe, replayer);
expect(element.getAttribute('srcdoc')).toBe(null);
});

it('can skip calling setAttribute when attr/value already exists', () => {
const tagName = 'STYLE';
const element = document.createElement(tagName);
const sn = Object.assign({}, elementSn, { tagName });
mirror.add(element, sn);

element.setAttribute('type', 'text/css');
expect(element.getAttribute('type')).toEqual('text/css');

const rrDocument = new RRDocument();
const rrNode = rrDocument.createElement(tagName);
const sn2 = Object.assign({}, elementSn, { tagName });
rrDocument.mirror.add(rrNode, sn2);
rrNode.attributes = { type: 'text/css' };

const setAttributeSpy = vi.spyOn(element, 'setAttribute');
diff(element, rrNode, replayer);
expect(setAttributeSpy).not.toHaveBeenCalled();

rrNode.attributes = { type: 'application/css' };
diff(element, rrNode, replayer);
expect(setAttributeSpy).toHaveBeenCalledWith('type', 'application/css');

setAttributeSpy.mockRestore();
});
});

describe('diff children', () => {
Expand Down
153 changes: 153 additions & 0 deletions packages/rrweb/test/events/inserted-stylesheet-rule-events.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
import { EventType, IncrementalSource } from '@rrweb/types';
import type { eventWithTime } from '@rrweb/types';

const now = Date.now();
const events: eventWithTime[] = [
{
type: EventType.DomContentLoaded,
data: {},
timestamp: now,
},
{
type: EventType.Load,
data: {},
timestamp: now + 100,
},
{
type: EventType.Meta,
data: {
href: 'http://localhost',
width: 1000,
height: 800,
},
timestamp: now + 100,
},
// full snapshot:
{
data: {
node: {
id: 1,
type: 0,
childNodes: [
{ id: 2, name: 'html', type: 1, publicId: '', systemId: '' },
{
id: 3,
type: 2,
tagName: 'html',
attributes: { lang: 'en' },
childNodes: [
{
id: 4,
type: 2,
tagName: 'head',
attributes: {},
childNodes: [
{
id: 101,
type: 2,
tagName: 'style',
attributes: {
'data-meta': 'from full-snapshot, gets rule added at 500',
},
childNodes: [
{
id: 102,
type: 3,
isStyle: true,
textContent:
'\n.css-added-at-200-overwritten-at-3000 {\n opacity: 1;\n transform: translateX(0);\n}\n',
},
],
},
{
id: 105,
type: 2,
tagName: 'style',
attributes: {
_cssText:
'.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; }',
'data-emotion': 'css',
},
childNodes: [
{ id: 106, type: 3, isStyle: true, textContent: '' },
],
},
],
},
{
id: 107,
type: 2,
tagName: 'body',
attributes: {},
childNodes: [
{
id: 108,
type: 2,
tagName: 'style',
attributes: {
'type': 'text/css',
'gs-style-id': 'gs-id-0',
'_cssText': '.original-style-rule { color: red; }'
},
childNodes: [
{
id: 109,
type: 3,
textContent: '',
isStyle: true
},
],
},
],
},
],
},
],
},
initialOffset: { top: 0, left: 0 },
},
type: EventType.FullSnapshot,
timestamp: now + 100,
},
// mutation that uses insertRule to add a new style rule to the existing body stylesheet
{
data: {
id: 108,
adds: [
{
rule: '.css-inserted-at-500 {border: 1px solid blue;}',
index: 1,
},
],
source: IncrementalSource.StyleSheetRule,
},
type: EventType.IncrementalSnapshot,
timestamp: now + 500,
},
// mutation that adds a child to the body
{
type: EventType.IncrementalSnapshot,
data: {
source: IncrementalSource.Mutation,
texts: [],
attributes: [],
removes: [],
adds: [
{
parentId: 107,
nextId: null,
node: {
type: 2,
tagName: 'div',
attributes: {},
childNodes: [],
id: 110,
},
}
],
},
timestamp: now + 900,
}
];

export default events;
21 changes: 21 additions & 0 deletions packages/rrweb/test/replayer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
waitForRAF,
} from './utils';
import styleSheetRuleEvents from './events/style-sheet-rule-events';
import insertedStyleSheetRuleEvents from './events/inserted-stylesheet-rule-events';
import orderingEvents from './events/ordering';
import scrollEvents from './events/scroll';
import scrollWithParentStylesEvents from './events/scroll-with-parent-styles';
Expand Down Expand Up @@ -299,6 +300,26 @@ describe('replayer', function () {
expect(result).toEqual(false);
});

it('should persist inserted stylesheet rules on fast forward', async () => {
await page.evaluate(`events = ${JSON.stringify(insertedStyleSheetRuleEvents)}`);
const result = await page.evaluate(`
const { Replayer } = rrweb;
const replayer = new Replayer(events);
replayer.pause(0);

// fast forward past the insertedRules at 500 and other mutations at 900
replayer.pause(905);

const allStylesheets = Array.from(replayer.iframe.contentDocument.styleSheets);
// find the stylesheet that has the cssRule with the selectorText '.original-style-rule'
const stylesheetWithInsertedRule = allStylesheets.find(sheet => Array.from(sheet.cssRules).find(rule => rule.selectorText === '.original-style-rule'));
stylesheetWithInsertedRule.cssRules.length;
`);

// verify the inserted cssRule wasn't dropped
expect(result).toEqual(2);
});

it('should apply fast-forwarded StyleSheetRules that came after stylesheet textContent overwrite', async () => {
await page.evaluate(`events = ${JSON.stringify(styleSheetRuleEvents)}`);
const result = await page.evaluate(`
Expand Down
Loading