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

Single style capture #1437

Merged
merged 44 commits into from
Aug 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
ea9f096
Unrelated, but the presence of a blank `<style></style>` element when…
eoghanmurray Apr 5, 2024
385dee8
Default to headless on retest
eoghanmurray Mar 28, 2024
945bde3
Add motivating test which can be used against this PR as well as in #…
eoghanmurray Apr 5, 2024
a6ef9c4
Recognize that snapshot.ts::serializeTextNode does important work for…
eoghanmurray Apr 12, 2024
bc769aa
Prep PR for async <style> serialization via assets: refactor stringif…
eoghanmurray Apr 12, 2024
b98c062
Add a test to show how mutations on multiple text nodes within the <s…
eoghanmurray Apr 12, 2024
6df62e1
Multiple <style> text children: Demonstrate a failing test as CSS was…
eoghanmurray Apr 12, 2024
899911d
Add test and remedy for the most trivial type of failure of `findCssT…
eoghanmurray Apr 12, 2024
173cd0f
Missed this which is 'happy path' - add a test to catch catch case
eoghanmurray Apr 12, 2024
2983a19
Create single-style-capture.md
eoghanmurray Apr 12, 2024
ee0fab5
An expected test change as per this PR; must have missed it first tim…
eoghanmurray Apr 15, 2024
3f4e0b7
More expected test changes I missed when authoring PR
eoghanmurray Apr 15, 2024
8d440c3
New function buildStyleNode to better encapsulate the separate behavi…
eoghanmurray Apr 16, 2024
1d31ad7
Revert an accidental reordering
eoghanmurray Apr 16, 2024
0067f5d
Add the css text length to the splits array as a method of checking t…
eoghanmurray May 3, 2024
7b5dda7
Refactor out `applyCssSplits` for clarity and so we can test it
eoghanmurray May 7, 2024
27832fe
Add more tests and fix bug that was causing css text to end up in the…
eoghanmurray May 3, 2024
7099d29
The _cssTextSplits should only apply to <style> elements
eoghanmurray May 7, 2024
2a7560a
Realize we don't actually need to look at the `.sheet` during a mutat…
eoghanmurray May 7, 2024
7a3d21e
Fix the following typing problem in the tests:
eoghanmurray May 7, 2024
b357b43
Highlight that there is a replayer involved in this test (as well as …
eoghanmurray May 2, 2024
91d29a5
Think this is how this should have been authored
eoghanmurray Jul 3, 2024
45f2953
Rather than recording a new separate error-prone _cssTextSplits attri…
eoghanmurray Jul 6, 2024
9fe426d
The `blankTextNodes` config option proved very confusing for both Jus…
eoghanmurray Jul 6, 2024
c5d1ef0
Simplify as I don't think this warning is actually needed, and tests …
eoghanmurray Jul 6, 2024
dd1bfee
Caught a case where a text mutation, which bypasses `_cssText`, wasn'…
eoghanmurray Jul 6, 2024
46aa8f2
Don't record css content twice when a <style> element is added in a m…
eoghanmurray Jul 8, 2024
8b9cc06
I suspected this style of mutation was going to result in duplicate t…
eoghanmurray Jul 8, 2024
3e9edbc
Include another type of <style> addition to ensure it doesn't duplica…
eoghanmurray Jul 8, 2024
b9622d0
Fix case where we wouldn't have been able to mutate a <style> text no…
eoghanmurray Jul 8, 2024
a9d14ea
Prefer `waitForRAF`
eoghanmurray Jul 8, 2024
3156575
feat: add test for recording and replaying style mutations with multi…
Juice10 Jul 3, 2024
b358827
This is the effect of the following two changesets:
eoghanmurray Jul 9, 2024
217e149
A bit more explicit with which style elements we're referring to in test
eoghanmurray Jul 9, 2024
b932c98
Generate two text content entries in the existing style element
eoghanmurray Jul 9, 2024
876b109
Fixup eslint
eoghanmurray Jul 9, 2024
39007f1
Some extra reminder on these tests as I believe 'can record and repla…
eoghanmurray Jul 9, 2024
34def57
Fix regression on test [html file]: with-style-sheet-with-import.html
eoghanmurray Jul 26, 2024
f40a053
Make algorithm easier to understand while still ensuring the 'maintai…
eoghanmurray Aug 2, 2024
7a93a1d
add 'type' keyword in css.test.ts
eoghanmurray Aug 1, 2024
83b3482
add cross-env to retest command in packages/rrweb/package.json
eoghanmurray Aug 1, 2024
63d79fb
Add break to short circuit when normalized string found
eoghanmurray Aug 2, 2024
9770a16
Convert one test over to happy-dom as unlike JSDOM, that can access s…
eoghanmurray Aug 2, 2024
58bfecc
Add stripping of css comments and also semicolons to the `normalizeCs…
eoghanmurray Aug 2, 2024
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
6 changes: 6 additions & 0 deletions .changeset/single-style-capture.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"rrweb-snapshot": patch
"rrweb": patch
---

Edge case: Provide support for mutations on a <style> element which (unusually) has multiple text nodes
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"eslint-plugin-compat": "^5.0.0",
"eslint-plugin-jest": "^27.6.0",
"eslint-plugin-tsdoc": "^0.2.17",
"happy-dom": "^14.12.0",
"markdownlint": "^0.25.1",
"markdownlint-cli": "^0.31.1",
"prettier": "2.8.4",
Expand Down
1 change: 0 additions & 1 deletion packages/rrdom/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
"@typescript-eslint/eslint-plugin": "^5.23.0",
"@typescript-eslint/parser": "^5.23.0",
"eslint": "^8.15.0",
"happy-dom": "^14.12.0",
"puppeteer": "^17.1.3",
"typescript": "^5.4.5",
"vite": "^5.3.1",
Expand Down
98 changes: 85 additions & 13 deletions packages/rrweb-snapshot/src/rebuild.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { mediaSelectorPlugin, pseudoClassPlugin } from './css';
import {
type serializedNodeWithId,
type serializedElementNodeWithId,
type serializedTextNodeWithId,
NodeType,
type tagMap,
type elementNode,
Expand Down Expand Up @@ -78,6 +80,77 @@ export function createCache(): BuildCache {
};
}

/**
* undo splitCssText/markCssSplits
* (would move to utils.ts but uses `adaptCssForReplay`)
*/
export function applyCssSplits(
n: serializedElementNodeWithId,
cssText: string,
hackCss: boolean,
cache: BuildCache,
): void {
const childTextNodes: serializedTextNodeWithId[] = [];
for (const scn of n.childNodes) {
if (scn.type === NodeType.Text) {
childTextNodes.push(scn);
}
}
const cssTextSplits = cssText.split('/* rr_split */');
while (
cssTextSplits.length > 1 &&
cssTextSplits.length > childTextNodes.length
) {
// unexpected: remerge the last two so that we don't discard any css
cssTextSplits.splice(-2, 2, cssTextSplits.slice(-2).join(''));
}
for (let i = 0; i < childTextNodes.length; i++) {
const childTextNode = childTextNodes[i];
const cssTextSection = cssTextSplits[i];
if (childTextNode && cssTextSection) {
// id will be assigned when these child nodes are
// iterated over in buildNodeWithSN
childTextNode.textContent = hackCss
? adaptCssForReplay(cssTextSection, cache)
: cssTextSection;
}
}
}

/**
* Normally a <style> element has a single textNode containing the rules.
* During serialization, we bypass this (`styleEl.sheet`) to get the rules the
* browser sees and serialize this to a special _cssText attribute, blanking
* out any text nodes. This function reverses that and also handles cases where
* there were no textNode children present (dynamic css/or a <link> element) as
* well as multiple textNodes, which need to be repopulated (based on presence of
* a special `rr_split` marker in case they are modified by subsequent mutations.
*/
export function buildStyleNode(
n: serializedElementNodeWithId,
styleEl: HTMLStyleElement, // when inlined, a <link type="stylesheet"> also gets rebuilt as a <style>
cssText: string,
options: {
doc: Document;
hackCss: boolean;
cache: BuildCache;
},
) {
const { doc, hackCss, cache } = options;
if (n.childNodes.length) {
applyCssSplits(n, cssText, hackCss, cache);
} else {
if (hackCss) {
cssText = adaptCssForReplay(cssText, cache);
}
/**
<link> element or dynamic <style> are serialized without any child nodes
we create the text node without an ID or presence in mirror as it can't
*/
styleEl.appendChild(doc.createTextNode(cssText));
}
}

function buildNode(
n: serializedNodeWithId,
options: {
Expand Down Expand Up @@ -154,14 +227,13 @@ function buildNode(
continue;
}

const isTextarea = tagName === 'textarea' && name === 'value';
const isRemoteOrDynamicCss = tagName === 'style' && name === '_cssText';
if (isRemoteOrDynamicCss && hackCss && typeof value === 'string') {
value = adaptCssForReplay(value, cache);
}
if ((isTextarea || isRemoteOrDynamicCss) && typeof value === 'string') {
// https://github.com/rrweb-io/rrweb/issues/112
// https://github.com/rrweb-io/rrweb/pull/1351
if (typeof value !== 'string') {
// pass
} else if (tagName === 'style' && name === '_cssText') {
buildStyleNode(n, node as HTMLStyleElement, value, options);
continue; // no need to set _cssText as attribute
} else if (tagName === 'textarea' && name === 'value') {
// create without an ID or presence in mirror
node.appendChild(doc.createTextNode(value));
n.childNodes = []; // value overrides childNodes
continue;
Expand Down Expand Up @@ -317,11 +389,11 @@ function buildNode(
return node;
}
case NodeType.Text:
return doc.createTextNode(
n.isStyle && hackCss
? adaptCssForReplay(n.textContent, cache)
: n.textContent,
);
if (n.isStyle && hackCss) {
// support legacy style
return doc.createTextNode(adaptCssForReplay(n.textContent, cache));
eoghanmurray marked this conversation as resolved.
Show resolved Hide resolved
}
return doc.createTextNode(n.textContent);
eoghanmurray marked this conversation as resolved.
Show resolved Hide resolved
Juice10 marked this conversation as resolved.
Show resolved Hide resolved
case NodeType.CDATA:
return doc.createCDATASection(n.textContent);
case NodeType.Comment:
Expand Down
75 changes: 38 additions & 37 deletions packages/rrweb-snapshot/src/snapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
toLowerCase,
extractFileExtension,
absolutifyURLs,
markCssSplits,
} from './utils';
import dom from '@rrweb/utils';

Expand Down Expand Up @@ -279,7 +280,7 @@
// should warn? maybe a text node isn't attached to a parent node yet?
return false;
} else {
el = dom.parentElement(node)!;

Check warning on line 283 in packages/rrweb-snapshot/src/snapshot.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb-snapshot/src/snapshot.ts#L283

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
}
try {
if (typeof maskTextClass === 'string') {
Expand Down Expand Up @@ -403,6 +404,7 @@
* `newlyAddedElement: true` skips scrollTop and scrollLeft check
*/
newlyAddedElement?: boolean;
cssCaptured?: boolean;
},
): serializedNode | false {
const {
Expand All @@ -420,6 +422,7 @@
recordCanvas,
keepIframeSrcFn,
newlyAddedElement = false,
cssCaptured = false,
} = options;
// Only record root id when document object is not the base document
const rootId = getRootId(doc, mirror);
Expand Down Expand Up @@ -466,6 +469,7 @@
needsMask,
maskTextFn,
rootId,
cssCaptured,
});
case n.CDATA_SECTION_NODE:
return {
Expand Down Expand Up @@ -497,48 +501,38 @@
needsMask: boolean;
maskTextFn: MaskTextFn | undefined;
rootId: number | undefined;
cssCaptured?: boolean;
},
): serializedNode {
const { needsMask, maskTextFn, rootId } = options;
const { needsMask, maskTextFn, rootId, cssCaptured } = options;
// The parent node may not be a html element which has a tagName attribute.
// So just let it be undefined which is ok in this use case.
const parent = dom.parentNode(n);
const parentTagName = parent && (parent as HTMLElement).tagName;
let text = dom.textContent(n);
let textContent: string | null = '';
const isStyle = parentTagName === 'STYLE' ? true : undefined;
const isScript = parentTagName === 'SCRIPT' ? true : undefined;
if (isStyle && text) {
try {
// try to read style sheet
if (n.nextSibling || n.previousSibling) {
// This is not the only child of the stylesheet.
// We can't read all of the sheet's .cssRules and expect them
// to _only_ include the current rule(s) added by the text node.
// So we'll be conservative and keep textContent as-is.
} else if ((parent as HTMLStyleElement).sheet?.cssRules) {
text = stringifyStylesheet((parent as HTMLStyleElement).sheet!);
}
} catch (err) {
console.warn(
`Cannot get CSS styles from text's parentNode. Error: ${err as string}`,
n,
);
}
text = absolutifyURLs(text, getHref(options.doc));
}
if (isScript) {
text = 'SCRIPT_PLACEHOLDER';
textContent = 'SCRIPT_PLACEHOLDER';
} else if (!cssCaptured) {
textContent = dom.textContent(n);
if (isStyle && textContent) {
// mutation only: we don't need to use stringifyStylesheet
// as a <style> text node mutation obliterates any previous
// programmatic rule manipulation (.insertRule etc.)
// so the current textContent represents the most up to date state
textContent = absolutifyURLs(textContent, getHref(options.doc));
}
}
if (!isStyle && !isScript && text && needsMask) {
text = maskTextFn
? maskTextFn(text, dom.parentElement(n))
: text.replace(/[\S]/g, '*');
if (!isStyle && !isScript && textContent && needsMask) {
textContent = maskTextFn
? maskTextFn(textContent, dom.parentElement(n))
: textContent.replace(/[\S]/g, '*');
}

return {
type: NodeType.Text,
textContent: text || '',
isStyle,
textContent: textContent || '',
rootId,
};
}
Expand Down Expand Up @@ -608,17 +602,14 @@
attributes._cssText = cssText;
}
}
// dynamic stylesheet
if (
tagName === 'style' &&
(n as HTMLStyleElement).sheet &&
// TODO: Currently we only try to get dynamic stylesheet when it is an empty style element
!(n.innerText || dom.textContent(n) || '').trim().length
) {
const cssText = stringifyStylesheet(
if (tagName === 'style' && (n as HTMLStyleElement).sheet) {
let cssText = stringifyStylesheet(
(n as HTMLStyleElement).sheet as CSSStyleSheet,
);
if (cssText) {
if (n.childNodes.length > 1) {
cssText = markCssSplits(cssText, n as HTMLStyleElement);
}
attributes._cssText = cssText;
}
}
Expand Down Expand Up @@ -709,10 +700,10 @@
const recordInlineImage = () => {
image.removeEventListener('load', recordInlineImage);
try {
canvasService!.width = image.naturalWidth;

Check warning on line 703 in packages/rrweb-snapshot/src/snapshot.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb-snapshot/src/snapshot.ts#L703

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
canvasService!.height = image.naturalHeight;

Check warning on line 704 in packages/rrweb-snapshot/src/snapshot.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb-snapshot/src/snapshot.ts#L704

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
canvasCtx!.drawImage(image, 0, 0);

Check warning on line 705 in packages/rrweb-snapshot/src/snapshot.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb-snapshot/src/snapshot.ts#L705

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
attributes.rr_dataURL = canvasService!.toDataURL(

Check warning on line 706 in packages/rrweb-snapshot/src/snapshot.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb-snapshot/src/snapshot.ts#L706

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
dataURLOptions.type,
dataURLOptions.quality,
);
Expand Down Expand Up @@ -937,6 +928,7 @@
node: serializedElementNodeWithId,
) => unknown;
stylesheetLoadTimeout?: number;
cssCaptured?: boolean;
},
): serializedNodeWithId | null {
const {
Expand All @@ -962,6 +954,7 @@
stylesheetLoadTimeout = 5000,
keepIframeSrcFn = () => false,
newlyAddedElement = false,
cssCaptured = false,
} = options;
let { needsMask } = options;
let { preserveWhiteSpace = true } = options;
Expand Down Expand Up @@ -992,6 +985,7 @@
recordCanvas,
keepIframeSrcFn,
newlyAddedElement,
cssCaptured,
});
if (!_serializedNode) {
// TODO: dev only
Expand All @@ -1007,7 +1001,6 @@
slimDOMExcluded(_serializedNode, slimDOMOptions) ||
(!preserveWhiteSpace &&
_serializedNode.type === NodeType.Text &&
!_serializedNode.isStyle &&
!_serializedNode.textContent.replace(/^\s+|\s+$/gm, '').length)
) {
id = IGNORED_NODE;
Expand Down Expand Up @@ -1072,6 +1065,7 @@
onStylesheetLoad,
stylesheetLoadTimeout,
keepIframeSrcFn,
cssCaptured: false,
};

if (
Expand All @@ -1081,6 +1075,13 @@
) {
// value parameter in DOM reflects the correct value, so ignore childNode
} else {
if (
serializedNode.type === NodeType.Element &&
(serializedNode as elementNode).attributes._cssText !== undefined &&
typeof serializedNode.attributes._cssText === 'string'
) {
bypassOptions.cssCaptured = true;
}
for (const childN of Array.from(dom.childNodes(n))) {
const serializedChildNode = serializeNodeWithId(childN, bypassOptions);
if (serializedChildNode) {
Expand Down
22 changes: 20 additions & 2 deletions packages/rrweb-snapshot/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,18 @@ export type documentTypeNode = {
systemId: string;
};

export type attributes = {
[key: string]: string | number | true | null;
type cssTextKeyAttr = {
_cssText?: string;
};

export type attributes = cssTextKeyAttr & {
[key: string]:
| string
| number // properties e.g. rr_scrollLeft or rr_mediaCurrentTime
| true // e.g. checked on <input type="radio">
| null; // an indication that an attribute was removed (during a mutation)
};

export type legacyAttributes = {
/**
* @deprecated old bug in rrweb was causing these to always be set
Expand All @@ -45,6 +54,10 @@ export type elementNode = {
export type textNode = {
type: NodeType.Text;
textContent: string;
/**
* @deprecated styles are now always snapshotted against parent <style> element
* style mutations can still happen via an added textNode, but they don't need this attribute for correct replay
*/
isStyle?: true;
};

Expand Down Expand Up @@ -78,6 +91,11 @@ export type serializedElementNodeWithId = Extract<
Record<'type', NodeType.Element>
>;

export type serializedTextNodeWithId = Extract<
serializedNodeWithId,
Record<'type', NodeType.Text>
>;
eoghanmurray marked this conversation as resolved.
Show resolved Hide resolved

export type tagMap = {
[key: string]: string;
};
Expand Down
Loading
Loading