From d4f2da1244308e4c0ad0eaf1abc4febb6dc8714a Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 17 Dec 2024 14:54:17 -0500 Subject: [PATCH 1/2] fix: inserted styles lost when moving elements (#232) 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 Co-authored-by: jaj1014 --- 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 | 23 +- 4 files changed, 357 insertions(+), 10 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 a927832235..31b6525093 100644 --- a/packages/rrdom/src/diff.ts +++ b/packages/rrdom/src/diff.ts @@ -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); } @@ -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); } @@ -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); } @@ -460,7 +460,7 @@ function diffChildren( } try { - oldTree.insertBefore(newNode, oldStartNode || null); + handleInsertBefore(oldTree, newNode, oldStartNode || null); } catch (e) { console.warn(e); } @@ -482,7 +482,7 @@ function diffChildren( rrnodeMirror, ); try { - oldTree.insertBefore(newNode, referenceNode); + handleInsertBefore(oldTree, newNode, referenceNode); } catch (e) { console.warn(e); } @@ -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); + }); + } +} diff --git a/packages/rrdom/test/diff.test.ts b/packages/rrdom/test/diff.test.ts index fa5a8ccb8a..8a3207e704 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 '@sentry-internal/rrweb'; @@ -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: () => {}, @@ -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 && @@ -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(''); + + 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 723988638a..443fe34dfe 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 touchAllPointerEvents from './events/touch-all-pointer-id'; import touchSomePointerEvents from './events/touch-some-pointer-id'; @@ -179,6 +180,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(` @@ -247,7 +264,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]; @@ -724,7 +741,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()!; @@ -966,7 +983,7 @@ describe('replayer', function () { await page.evaluate(` const { Replayer } = rrweb; let replayer = new Replayer(events); - replayer.play(); + replayer.play(); `); const replayerWrapperClassName = 'replayer-wrapper'; From ed9f888729c1c47e10a71d6e95c3820d4a6ce1a9 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 17 Dec 2024 14:56:51 -0500 Subject: [PATCH 2/2] update rrweb/types import --- packages/rrweb/test/events/moving-style-sheet-on-diff.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/rrweb/test/events/moving-style-sheet-on-diff.ts b/packages/rrweb/test/events/moving-style-sheet-on-diff.ts index af80da3e17..5c2acb5270 100644 --- a/packages/rrweb/test/events/moving-style-sheet-on-diff.ts +++ b/packages/rrweb/test/events/moving-style-sheet-on-diff.ts @@ -1,5 +1,5 @@ -import { EventType, IncrementalSource } from '@rrweb/types'; -import type { eventWithTime } from '@rrweb/types'; +import { EventType, IncrementalSource } from '@sentry-internal/rrweb-types'; +import type { eventWithTime } from '@sentry-internal/rrweb-types'; const now = Date.now(); const events: eventWithTime[] = [