From a5ce9eb43cafe0898f72109aec0df839ef7fd4cb Mon Sep 17 00:00:00 2001 From: jaj1014 Date: Mon, 20 Nov 2023 11:56:25 -0500 Subject: [PATCH 1/4] 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 5cff5dc724..612fb6685f 100644 --- a/packages/rrdom/src/diff.ts +++ b/packages/rrdom/src/diff.ts @@ -413,7 +413,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); } @@ -424,7 +424,7 @@ function diffChildren( nodeMatching(oldEndNode, newStartNode, replayer.mirror, rrnodeMirror) ) { try { - oldTree.insertBefore(oldEndNode, oldStartNode); + handleInsertBefore(oldTree, oldEndNode, oldStartNode); } catch (e) { console.warn(e); } @@ -449,7 +449,7 @@ function diffChildren( nodeMatching(nodeToMove, newStartNode, replayer.mirror, rrnodeMirror) ) { try { - oldTree.insertBefore(nodeToMove, oldStartNode); + handleInsertBefore(oldTree, nodeToMove, oldStartNode); } catch (e) { console.warn(e); } @@ -483,7 +483,7 @@ function diffChildren( } try { - oldTree.insertBefore(newNode, oldStartNode || null); + handleInsertBefore(oldTree, newNode, oldStartNode || null); } catch (e) { console.warn(e); } @@ -505,7 +505,7 @@ function diffChildren( rrnodeMirror, ); try { - oldTree.insertBefore(newNode, referenceNode); + handleInsertBefore(oldTree, newNode, referenceNode); } catch (e) { console.warn(e); } @@ -613,3 +613,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 e250ef8f06..cd5c6521dc 100644 --- a/packages/rrdom/test/diff.test.ts +++ b/packages/rrdom/test/diff.test.ts @@ -25,6 +25,7 @@ import { ReplayerHandler, nodeMatching, sameNodeType, + handleInsertBefore, } from '../src/diff'; import type { IRRElement, IRRNode } from '../src/document'; import type { canvasMutationData, styleSheetRuleData } from '@rrweb/types'; @@ -1467,7 +1468,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: () => {}, @@ -1476,7 +1477,7 @@ describe('diff algorithm for rrdom', () => { applyStyleSheetMutation: () => {}, }; rrdom.diff(iframeEl, rrIframeEl, replayer); - + iframeEl.contentDocument.documentElement.className = '${className.toLowerCase()}'; iframeEl.contentDocument.childNodes.length === 2 && @@ -1838,4 +1839,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 96e2012b15..6067330266 100644 --- a/packages/rrweb/test/replayer.test.ts +++ b/packages/rrweb/test/replayer.test.ts @@ -11,6 +11,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'; @@ -177,6 +178,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(` @@ -228,7 +245,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]; @@ -705,7 +722,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()!; @@ -767,7 +784,7 @@ describe('replayer', function () { const replayer = new Replayer(events); replayer.play(); `); - await page.waitForTimeout(50); + await waitForRAF(page); await assertDomSnapshot(page); }); @@ -791,7 +808,7 @@ describe('replayer', function () { await page.evaluate(` const { Replayer } = rrweb; let replayer = new Replayer(events); - replayer.play(); + replayer.play(); `); const replayerWrapperClassName = 'replayer-wrapper'; From 471c19ef8e18bf8cf64e87b1f159d6a3fea74888 Mon Sep 17 00:00:00 2001 From: jaj1014 Date: Thu, 23 May 2024 13:27:00 +0000 Subject: [PATCH 2/4] Apply formatting changes --- packages/rrdom/test/diff.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rrdom/test/diff.test.ts b/packages/rrdom/test/diff.test.ts index cd5c6521dc..3be05c463a 100644 --- a/packages/rrdom/test/diff.test.ts +++ b/packages/rrdom/test/diff.test.ts @@ -1890,7 +1890,7 @@ describe('diff algorithm for rrdom', () => { handleInsertBefore( document.documentElement, nodeToMove, - insertBeforeNode + insertBeforeNode, ); // nodeToMove was inserted before From cf4b340aa024d458c477a28c8cdc0643c890ef15 Mon Sep 17 00:00:00 2001 From: jaj1014 Date: Tue, 22 Oct 2024 05:26:46 -0400 Subject: [PATCH 3/4] Update test to use vitest --- packages/rrdom/test/diff.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/rrdom/test/diff.test.ts b/packages/rrdom/test/diff.test.ts index 3be05c463a..0ed95cb8a2 100644 --- a/packages/rrdom/test/diff.test.ts +++ b/packages/rrdom/test/diff.test.ts @@ -1857,11 +1857,11 @@ describe('diff algorithm for rrdom', () => { it('should not drop inserted styles when moving a style element with inserted styles', async () => { function MockCSSStyleSheet() { - this.replaceSync = jest.fn(); + this.replaceSync = vi.fn(); this.cssRules = [{ cssText: baseStyle }]; } - jest + vi .spyOn(window, 'CSSStyleSheet') .mockImplementationOnce(MockCSSStyleSheet as any); From 74f887e6eeeb57d0662e3d6692a8ec768602dd85 Mon Sep 17 00:00:00 2001 From: jaj1014 Date: Tue, 22 Oct 2024 09:40:20 +0000 Subject: [PATCH 4/4] Apply formatting changes --- packages/rrdom/test/diff.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/rrdom/test/diff.test.ts b/packages/rrdom/test/diff.test.ts index 0ed95cb8a2..407186b2f8 100644 --- a/packages/rrdom/test/diff.test.ts +++ b/packages/rrdom/test/diff.test.ts @@ -1861,9 +1861,9 @@ describe('diff algorithm for rrdom', () => { this.cssRules = [{ cssText: baseStyle }]; } - vi - .spyOn(window, 'CSSStyleSheet') - .mockImplementationOnce(MockCSSStyleSheet as any); + vi.spyOn(window, 'CSSStyleSheet').mockImplementationOnce( + MockCSSStyleSheet as any, + ); const baseStyle = 'body {margin: 0;}'; const insertedStyle = 'div {display: flex;}';