Skip to content

Commit

Permalink
fix: inserted styles lost when moving elements (#233)
Browse files Browse the repository at this point in the history
fix code for nodejs tests
change fix direction to avoid issues with duplicate styles
format issues
swap waitForTimeout for waitForRAF in test that flaked
Add unit tests for new functions
Fix broken test causes by file formatting removing spaced

---------

Co-authored-by: jaj1014 <ajax@pendo.io>
Co-authored-by: jaj1014 <jaj1014@users.noreply.github.com>
  • Loading branch information
3 people authored Dec 18, 2024
1 parent dd1f591 commit 8e9b645
Show file tree
Hide file tree
Showing 4 changed files with 357 additions and 10 deletions.
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);

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

0 comments on commit 8e9b645

Please sign in to comment.