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.

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: inserted styles lost when moving elements #233

Open
wants to merge 2 commits into
base: sentry-v2
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
71 changes: 66 additions & 5 deletions packages/rrdom/src/diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ function diffChildren(
nodeMatching(oldStartNode, newEndNode, replayer.mirror, rrnodeMirror)
) {
try {
oldTree.insertBefore(oldStartNode, oldEndNode.nextSibling);
handleInsertBefore(oldTree, oldStartNode, oldEndNode.nextSibling);
} catch (e) {
console.warn(e);
}
Expand All @@ -401,7 +401,7 @@ function diffChildren(
nodeMatching(oldEndNode, newStartNode, replayer.mirror, rrnodeMirror)
) {
try {
oldTree.insertBefore(oldEndNode, oldStartNode);
handleInsertBefore(oldTree, oldEndNode, oldStartNode);
} catch (e) {
console.warn(e);
}
Expand All @@ -426,7 +426,7 @@ function diffChildren(
nodeMatching(nodeToMove, newStartNode, replayer.mirror, rrnodeMirror)
) {
try {
oldTree.insertBefore(nodeToMove, oldStartNode);
handleInsertBefore(oldTree, nodeToMove, oldStartNode);
} catch (e) {
console.warn(e);
}
Expand Down Expand Up @@ -460,7 +460,7 @@ function diffChildren(
}

try {
oldTree.insertBefore(newNode, oldStartNode || null);
handleInsertBefore(oldTree, newNode, oldStartNode || null);
} catch (e) {
console.warn(e);
}
Expand All @@ -482,7 +482,7 @@ function diffChildren(
rrnodeMirror,
);
try {
oldTree.insertBefore(newNode, referenceNode);
handleInsertBefore(oldTree, newNode, referenceNode);
} catch (e) {
console.warn(e);
}
Expand Down Expand Up @@ -590,3 +590,64 @@ export function nodeMatching(
if (node1Id === -1 || node1Id !== node2Id) return false;
return sameNodeType(node1, node2);
}

/**
* Copies CSSRules and their position from HTML style element which don't exist in it's innerText
*/
function getInsertedStylesFromElement(
styleElement: HTMLStyleElement,
): Array<{ index: number; cssRuleText: string }> | undefined {
const elementCssRules = styleElement.sheet?.cssRules;
if (!elementCssRules || !elementCssRules.length) return;
// style sheet w/ innerText styles to diff with actual and get only inserted styles
const tempStyleSheet = new CSSStyleSheet();
tempStyleSheet.replaceSync(styleElement.innerText);
Copy link
Member

Choose a reason for hiding this comment

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

This could be an issue for some safari users: https://caniuse.com/mdn-api_cssstylesheet_replacesync but I'd leave it in.


const innerTextStylesMap: { [key: string]: CSSRule } = {};

for (let i = 0; i < tempStyleSheet.cssRules.length; i++) {
innerTextStylesMap[tempStyleSheet.cssRules[i].cssText] =
tempStyleSheet.cssRules[i];
}

const insertedStylesStyleSheet = [];

for (let i = 0; i < elementCssRules?.length; i++) {
const cssRuleText = elementCssRules[i].cssText;

if (!innerTextStylesMap[cssRuleText]) {
insertedStylesStyleSheet.push({
index: i,
cssRuleText,
});
}
}

return insertedStylesStyleSheet;
}

/**
* Conditionally copy insertedStyles for STYLE nodes and apply after calling insertBefore'
* For non-STYLE nodes, just insertBefore
*/
export function handleInsertBefore(
oldTree: Node,
nodeToMove: Node,
insertBeforeNode: Node | null,
): void {
let insertedStyles;

if (nodeToMove.nodeName === 'STYLE') {
insertedStyles = getInsertedStylesFromElement(
nodeToMove as HTMLStyleElement,
);
}

oldTree.insertBefore(nodeToMove, insertBeforeNode);

if (insertedStyles && insertedStyles.length) {
insertedStyles.forEach(({ cssRuleText, index }) => {
(nodeToMove as HTMLStyleElement).sheet?.insertRule(cssRuleText, index);
});
}
}
70 changes: 68 additions & 2 deletions packages/rrdom/test/diff.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
ReplayerHandler,
nodeMatching,
sameNodeType,
handleInsertBefore,
} from '../src/diff';
import type { IRRElement, IRRNode } from '../src/document';
import { Replayer } from '@sentry-internal/rrweb';
Expand Down Expand Up @@ -1469,7 +1470,7 @@ describe('diff algorithm for rrdom', () => {
const rrHtmlEl = rrDocument.createElement('html');
rrDocument.mirror.add(rrHtmlEl, rrdom.getDefaultSN(rrHtmlEl, ${htmlElId}));
rrIframeEl.contentDocument.appendChild(rrHtmlEl);

const replayer = {
mirror: rrdom.createMirror(),
applyCanvas: () => {},
Expand All @@ -1478,7 +1479,7 @@ describe('diff algorithm for rrdom', () => {
applyStyleSheetMutation: () => {},
};
rrdom.diff(iframeEl, rrIframeEl, replayer);

iframeEl.contentDocument.documentElement.className =
'${className.toLowerCase()}';
iframeEl.contentDocument.childNodes.length === 2 &&
Expand Down Expand Up @@ -2000,4 +2001,69 @@ describe('diff algorithm for rrdom', () => {
expect(nodeMatching(node1, node2, NodeMirror, rrdomMirror)).toBeFalsy();
});
});

describe('test handleInsertBefore function', () => {
it('should insert nodeToMove before insertBeforeNode in oldTree for non-style elements', () => {
const oldTree = document.createElement('div');
const nodeToMove = document.createElement('div');
const insertBeforeNode = document.createElement('div');
oldTree.appendChild(insertBeforeNode);

expect(oldTree.children.length).toEqual(1);

handleInsertBefore(oldTree, nodeToMove, insertBeforeNode);

expect(oldTree.children.length).toEqual(2);
expect(oldTree.children[0]).toEqual(nodeToMove);
});

it('should not drop inserted styles when moving a style element with inserted styles', async () => {
function MockCSSStyleSheet() {
this.replaceSync = jest.fn();
this.cssRules = [{ cssText: baseStyle }];
}

jest
.spyOn(window, 'CSSStyleSheet')
.mockImplementationOnce(MockCSSStyleSheet as any);

const baseStyle = 'body {margin: 0;}';
const insertedStyle = 'div {display: flex;}';

document.write('<html></html>');

const insertBeforeNode = document.createElement('style');
document.documentElement.appendChild(insertBeforeNode);

const nodeToMove = document.createElement('style');
nodeToMove.appendChild(document.createTextNode(baseStyle));
document.documentElement.appendChild(nodeToMove);
nodeToMove.sheet?.insertRule(insertedStyle);

// validate dom prior to moving element
expect(document.documentElement.children.length).toEqual(4);
expect(document.documentElement.children[2]).toEqual(insertBeforeNode);
expect(document.documentElement.children[3]).toEqual(nodeToMove);
expect(nodeToMove.sheet?.cssRules.length).toEqual(2);
expect(nodeToMove.sheet?.cssRules[0].cssText).toEqual(insertedStyle);
expect(nodeToMove.sheet?.cssRules[1].cssText).toEqual(baseStyle);

// move the node
handleInsertBefore(
document.documentElement,
nodeToMove,
insertBeforeNode,
);

// nodeToMove was inserted before
expect(document.documentElement.children.length).toEqual(4);
expect(document.documentElement.children[2]).toEqual(nodeToMove);
expect(document.documentElement.children[3]).toEqual(insertBeforeNode);
// styles persisted on the moved element
// w/ document.documentElement.insertBefore(nodeToMove, insertBeforeNode) insertedStyle wouldn't be copied
expect(nodeToMove.sheet?.cssRules.length).toEqual(2);
expect(nodeToMove.sheet?.cssRules[0].cssText).toEqual(insertedStyle);
expect(nodeToMove.sheet?.cssRules[1].cssText).toEqual(baseStyle);
});
});
});
Loading
Loading