From cae6981afab28400183db196eb9cd72393aff5c7 Mon Sep 17 00:00:00 2001 From: jaj1014 Date: Mon, 20 Nov 2023 11:56:25 -0500 Subject: [PATCH] fix: inserted styles lost when moving elements 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 --- packages/rrdom/src/diff.ts | 71 +++++- packages/rrdom/test/diff.test.ts | 70 +++++- .../test/events/moving-style-sheet-on-diff.ts | 203 ++++++++++++++++++ packages/rrweb/test/replayer.test.ts | 25 ++- 4 files changed, 358 insertions(+), 11 deletions(-) create mode 100644 packages/rrweb/test/events/moving-style-sheet-on-diff.ts diff --git a/packages/rrdom/src/diff.ts b/packages/rrdom/src/diff.ts index 8ccf81f8c6..dc82b97893 100644 --- a/packages/rrdom/src/diff.ts +++ b/packages/rrdom/src/diff.ts @@ -381,7 +381,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); } @@ -392,7 +392,7 @@ function diffChildren( nodeMatching(oldEndNode, newStartNode, replayer.mirror, rrnodeMirror) ) { try { - oldTree.insertBefore(oldEndNode, oldStartNode); + handleInsertBefore(oldTree, oldEndNode, oldStartNode); } catch (e) { console.warn(e); } @@ -417,7 +417,7 @@ function diffChildren( nodeMatching(nodeToMove, newStartNode, replayer.mirror, rrnodeMirror) ) { try { - oldTree.insertBefore(nodeToMove, oldStartNode); + handleInsertBefore(oldTree, nodeToMove, oldStartNode); } catch (e) { console.warn(e); } @@ -451,7 +451,7 @@ function diffChildren( } try { - oldTree.insertBefore(newNode, oldStartNode || null); + handleInsertBefore(oldTree, newNode, oldStartNode || null); } catch (e) { console.warn(e); } @@ -473,7 +473,7 @@ function diffChildren( rrnodeMirror, ); try { - oldTree.insertBefore(newNode, referenceNode); + handleInsertBefore(oldTree, newNode, referenceNode); } catch (e) { console.warn(e); } @@ -581,3 +581,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); + }); + } +} diff --git a/packages/rrdom/test/diff.test.ts b/packages/rrdom/test/diff.test.ts index fc76f48bd9..4845192d55 100644 --- a/packages/rrdom/test/diff.test.ts +++ b/packages/rrdom/test/diff.test.ts @@ -23,6 +23,7 @@ import { ReplayerHandler, nodeMatching, sameNodeType, + handleInsertBefore, } from '../src/diff'; import type { IRRElement, IRRNode } from '../src/document'; import { Replayer } from 'rrweb'; @@ -1443,7 +1444,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: () => {}, @@ -1452,7 +1453,7 @@ describe('diff algorithm for rrdom', () => { applyStyleSheetMutation: () => {}, }; rrdom.diff(iframeEl, rrIframeEl, replayer); - + iframeEl.contentDocument.documentElement.className = '${className.toLowerCase()}'; iframeEl.contentDocument.childNodes.length === 2 && @@ -1974,4 +1975,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(''); + + 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); + }); + }); }); diff --git a/packages/rrweb/test/events/moving-style-sheet-on-diff.ts b/packages/rrweb/test/events/moving-style-sheet-on-diff.ts new file mode 100644 index 0000000000..af80da3e17 --- /dev/null +++ b/packages/rrweb/test/events/moving-style-sheet-on-diff.ts @@ -0,0 +1,203 @@ +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 + 10, + }, + { + type: EventType.Meta, + data: { + href: 'http://localhost', + width: 1000, + height: 800, + }, + timestamp: now + 10, + }, + // full snapshot: + { + 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: 2, + tagName: 'style', + attributes: {}, + childNodes: [ + { + type: 3, + textContent: + '#wrapper { width: 200px; margin: 50px auto; background-color: gainsboro; padding: 20px; }.target-element { padding: 12px; margin-top: 12px; }', + isStyle: true, + id: 6, + }, + ], + id: 5, + }, + { + type: 2, + tagName: 'style', + attributes: {}, + childNodes: [ + { + type: 3, + textContent: + '.new-element-class { font-size: 32px; color: tomato; }', + isStyle: true, + id: 8, + }, + ], + id: 7, + }, + ], + id: 4, + }, + { + type: 2, + tagName: 'body', + attributes: {}, + childNodes: [ + { + type: 2, + tagName: 'div', + attributes: { + id: 'wrapper', + }, + childNodes: [ + { + type: 2, + tagName: 'div', + attributes: { + class: 'target-element', + }, + childNodes: [ + { + type: 2, + tagName: 'p', + attributes: { + class: 'target-element-child', + }, + childNodes: [ + { + type: 3, + textContent: 'Element to style', + id: 113, + }, + ], + id: 12, + }, + ], + id: 11, + }, + ], + id: 10, + }, + ], + id: 9, + }, + ], + id: 3, + }, + ], + id: 1, + }, + initialOffset: { + left: 0, + top: 0, + }, + }, + type: EventType.FullSnapshot, + timestamp: now + 20, + }, + // 1st mutation that applies StyleSheetRule + { + type: EventType.IncrementalSnapshot, + data: { + source: IncrementalSource.StyleSheetRule, + id: 5, + adds: [ + { + rule: '.target-element{background-color:teal;}', + }, + ], + }, + timestamp: now + 30, + }, + // 2nd mutation inserts new style element to trigger other style element to get moved in diff + { + type: EventType.IncrementalSnapshot, + data: { + source: IncrementalSource.Mutation, + texts: [], + attributes: [], + removes: [{ parentId: 4, id: 7 }], + adds: [ + { + parentId: 4, + nextId: 5, + node: { + type: 2, + tagName: 'style', + attributes: {}, + childNodes: [], + id: 98, + }, + }, + { + parentId: 98, + nextId: null, + node: { + type: 3, + textContent: + '.new-element-class { font-size: 32px; color: tomato; }', + isStyle: true, + id: 99, + }, + }, + ], + }, + timestamp: now + 2000, + }, + // dummy event to have somewhere to skip + { + data: { + adds: [], + texts: [], + source: IncrementalSource.Mutation, + removes: [], + attributes: [], + }, + type: EventType.IncrementalSnapshot, + timestamp: now + 3000, + }, +]; + +export default events; diff --git a/packages/rrweb/test/replayer.test.ts b/packages/rrweb/test/replayer.test.ts index b54d360a6b..46adbcf212 100644 --- a/packages/rrweb/test/replayer.test.ts +++ b/packages/rrweb/test/replayer.test.ts @@ -10,6 +10,7 @@ import { waitForRAF, } from './utils'; import styleSheetRuleEvents from './events/style-sheet-rule-events'; +import movingStyleSheetOnDiff from './events/moving-style-sheet-on-diff'; import orderingEvents from './events/ordering'; import scrollEvents from './events/scroll'; import scrollWithParentStylesEvents from './events/scroll-with-parent-styles'; @@ -175,6 +176,22 @@ describe('replayer', function () { await assertDomSnapshot(page); }); + it('should persist StyleSheetRule changes when skipping triggers parent style element to move in diff', async () => { + await page.evaluate(`events = ${JSON.stringify(movingStyleSheetOnDiff)}`); + + const result = await page.evaluate(` + const { Replayer } = rrweb; + const replayer = new Replayer(events); + replayer.pause(3000); + const rules = [...replayer.iframe.contentDocument.styleSheets].map( + (sheet) => [...sheet.rules], + ).flat(); + rules.some((x) => x.cssText === '.target-element { background-color: teal; }'); + `); + + expect(result).toEqual(true); + }); + it('should apply fast forwarded StyleSheetRules that where added', async () => { await page.evaluate(`events = ${JSON.stringify(styleSheetRuleEvents)}`); const result = await page.evaluate(` @@ -226,7 +243,7 @@ describe('replayer', function () { await waitForRAF(page); /** check the second selection event */ - [startOffset, endOffset] = (await page.evaluate(` + [startOffset, endOffset] = (await page.evaluate(` replayer.pause(410); var range = replayer.iframe.contentDocument.getSelection().getRangeAt(0); [range.startOffset, range.endOffset]; @@ -703,7 +720,7 @@ describe('replayer', function () { events = ${JSON.stringify(canvasInIframe)}; const { Replayer } = rrweb; var replayer = new Replayer(events,{showDebug:true}); - replayer.pause(550); + replayer.pause(550); `); const replayerIframe = await page.$('iframe'); const contentDocument = await replayerIframe!.contentFrame()!; @@ -765,7 +782,7 @@ describe('replayer', function () { const replayer = new Replayer(events); replayer.play(); `); - await page.waitForTimeout(50); + await waitForRAF(page); await assertDomSnapshot(page); }); @@ -789,7 +806,7 @@ describe('replayer', function () { await page.evaluate(` const { Replayer } = rrweb; let replayer = new Replayer(events); - replayer.play(); + replayer.play(); `); const replayerWrapperClassName = 'replayer-wrapper';