Single style capture #1437

eoghanmurray · 2024-04-05T16:29:22Z

Prep PR for async <style> serialization via assets: refactor stringifyStylesheet to happen in a single place during initial snapshot.

initial motivation was to change the approach on stringifying <style> elements to do so in a single place
I then learned that we need the style content present on child text elements, in order to support mutation
this is still supported, however the css content is now preferably stored in parent <style> element in that _cssText attribute, rather than in the (natural) position in the child text elements of that element. This is to support the stylesheet-assets PR which will delay population of the _cssText attribute, but the snapshot will otherwise be the same with those blankTextNodes
on rebuild, this content can now be 'spread' out to child text nodes (usually there's only one!) in order to give a correct target for any further text mutations on these children.
there is a new complex but comprehensive test 'can record and replay style text mutations' which covers all of this; it has been designed to ensure that it fails if css text is populated in the wrong way (e.g. putting the entire cssText on the first of many text children and leaving the others blank)

changeset-bot · 2024-04-05T16:29:26Z

🦋 Changeset detected

Latest commit: 938aeea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages

Name	Type
rrweb-snapshot	Patch
rrweb	Patch
rrdom	Patch
rrdom-nodejs	Patch
rrweb-player	Patch
@rrweb/types	Patch
@rrweb/web-extension	Patch
rrvideo	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

eoghanmurray · 2024-04-12T17:48:48Z

packages/rrweb-snapshot/src/snapshot.ts

@@ -463,6 +465,7 @@ function serializeNode(
 recordCanvas,
 keepIframeSrcFn,
 newlyAddedElement = false,
+ blankTextNodes = false,


This addition is to prevent extra 'reading' of the textContent, while still generating an id for the text node.
However, if there's multiple text node children on the <style> we still will have to 'read' the textContent in findCssTextSplits

packages/rrweb-snapshot/src/rebuild.ts

eoghanmurray · 2024-04-16T10:42:24Z

packages/rrweb-snapshot/src/rebuild.ts

+ // support legacy style
+ return doc.createTextNode(adaptCssForReplay(n.textContent, cache));
+ }
+ return doc.createTextNode(n.textContent);


This diff is intended to be functionally the same, but rewritten to show that the isStyle marker is no longer expected

eoghanmurray · 2024-05-07T13:56:12Z

packages/rrweb/test/__snapshots__/record.test.ts.snap

+ \\"attributes\\": {
+ \\"_cssText\\": \\"div { color: red; }section { color: blue; }\\",
+ \\"_cssTextSplits\\": \\"19 43\\"
+ },
 \\"childNodes\\": [


So @Juice10 I think you had a question here about why I am moving things up to _cssText and adding _cssTextSplits rather than just storing everything in the childNodes ...
I suppose the "real reason" is that in the asset version of this code, we don't have the _cssText or _cssTextSplits attributes at all, but instead get these later as part of the asset event. So with the asset version, we are avoiding the inclusion of the css text content twice (in terms of size of total recording).

eoghanmurray · 2024-05-07T14:49:08Z

packages/rrweb-snapshot/src/types.ts

- [key: string]: string | number | true | null;
+type cssTextKeyAttr = {
+ _cssText?: string;
+ _cssTextSplits?: string;


This change is to ensure that if these two are defined that they can't be number/boolean/null (typescript was complaining without this)

… checking the replay took me ages to debug as I thought it was something I introducedg

…rweb-io#1417

… mutations with `absoluteToStylesheet` - I had accidentally removed that with initial work

…yStylesheet to happen in a single place during initial snapshot.

…tyle> need to be tracked

…n't being assigned to correct text node, and provide remedy whereby css text is properly re-split upon rebuild

…extSplits`

…e round

…our there and some explanatory comments

…hat the css hasn't been altered server-side/in-transit

… second text child instead of the first

…ion, even if there is only one text element on the <style>. Recent conversation with Justin has firmed up my thinking on why we look at .sheet in the first place (it's not because we want to get rid of vendor prefixes, which may actually be undesirable where they are useful for accurate replay in the replayer - but rather it's to ensure we don't miss any programmatic style mutations that have happened - have added comments to reflect this)

rrweb:test: test/utils.ts:181:43 - error TS2345: Argument of type 'elementNode & { rootId?: number | undefined; isShadowHost?: boolean | undefined; isShadow?: boolean | undefined; } & { id: number; }' is not assignable to parameter of type '{ attributes: { src?: string | undefined; }; }'. rrweb:test: Types of property 'attributes' are incompatible. rrweb:test: Type 'attributes' has no properties in common with type '{ src?: string | undefined; }'. rrweb:test: rrweb:test: 181 stripBlobURLsFromAttributes(add.node);

eoghanmurray mentioned this pull request Apr 5, 2024

fix: style element corner case #1417

Draft

eoghanmurray force-pushed the single-style-capture branch from 3ea2885 to 1d59339 Compare April 9, 2024 08:39

eoghanmurray mentioned this pull request Apr 11, 2024

Don't serialize all cssRules if multiple text nodes exists #866

Merged

eoghanmurray force-pushed the single-style-capture branch 2 times, most recently from fa37bfa to 0c875be Compare April 12, 2024 17:45

eoghanmurray commented Apr 12, 2024

View reviewed changes

packages/rrweb-snapshot/src/rebuild.ts Show resolved Hide resolved

eoghanmurray force-pushed the single-style-capture branch from 47f542b to 6210356 Compare April 12, 2024 17:57

eoghanmurray commented Apr 16, 2024

View reviewed changes

eoghanmurray force-pushed the single-style-capture branch from f5f1bde to ee6c028 Compare May 7, 2024 11:59

eoghanmurray commented May 7, 2024

View reviewed changes

eoghanmurray force-pushed the single-style-capture branch from 0c7403e to 1b71bc3 Compare May 7, 2024 14:33

eoghanmurray commented May 7, 2024

View reviewed changes

eoghanmurray marked this pull request as ready for review May 7, 2024 14:55

eoghanmurray force-pushed the single-style-capture branch from 58b031d to 746dfbd Compare May 8, 2024 13:55

eoghanmurray force-pushed the single-style-capture branch from 2fcf2fb to 02f4fcb Compare May 22, 2024 11:41

This was referenced May 22, 2024

improvements to <link rel="stylesheet"> + mutations #1483

Draft

assets + stylesheet assets #1475

Draft

eoghanmurray added 11 commits May 23, 2024 11:57

Unrelated, but the presence of a blank <style></style> element when…

8558ee3

… checking the replay took me ages to debug as I thought it was something I introducedg

Add motivating test which can be used against this PR as well as in r…

4a3bd11

…rweb-io#1417

Recognize that snapshot.ts::serializeTextNode does important work for…

c900af2

… mutations with `absoluteToStylesheet` - I had accidentally removed that with initial work

Prep PR for async <style> serialization via assets: refactor stringif…

5e4dc81

…yStylesheet to happen in a single place during initial snapshot.

Add a test to show how mutations on multiple text nodes within the <s…

2fc0c91

…tyle> need to be tracked

Multiple <style> text children: Demonstrate a failing test as CSS was…

358ab1d

…n't being assigned to correct text node, and provide remedy whereby css text is properly re-split upon rebuild

Add test and remedy for the most trivial type of failure of `findCssT…

0a5f38a

…extSplits`

Missed this which is 'happy path' - add a test to catch catch case

80f92be

Create single-style-capture.md

db5fb30

An expected test change as per this PR; must have missed it first tim…

0a54a3d

…e round

More expected test changes I missed when authoring PR

9b9962c

eoghanmurray added 9 commits May 23, 2024 11:57

New function buildStyleNode to better encapsulate the separate behavi…

b295ea9

…our there and some explanatory comments

Revert an accidental reordering

bd4f162

Add the css text length to the splits array as a method of checking t…

91f0259

…hat the css hasn't been altered server-side/in-transit

Refactor out applyCssSplits for clarity and so we can test it

2a1f40d

Add more tests and fix bug that was causing css text to end up in the…

5cb4025

… second text child instead of the first

The _cssTextSplits should only apply to <style> elements

549b410

Highlight that there is a replayer involved in these tests

938aeea

eoghanmurray force-pushed the single-style-capture branch from 54fc502 to 938aeea Compare May 23, 2024 10:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Single style capture #1437

Single style capture #1437

eoghanmurray commented Apr 5, 2024 •

edited

changeset-bot bot commented Apr 5, 2024 •

edited

eoghanmurray Apr 12, 2024

eoghanmurray Apr 16, 2024

eoghanmurray May 7, 2024

eoghanmurray May 7, 2024 •

edited

Single style capture #1437

Are you sure you want to change the base?

Single style capture #1437

Conversation

eoghanmurray commented Apr 5, 2024 • edited

changeset-bot bot commented Apr 5, 2024 • edited

🦋 Changeset detected

eoghanmurray Apr 12, 2024

Choose a reason for hiding this comment

eoghanmurray Apr 16, 2024

Choose a reason for hiding this comment

eoghanmurray May 7, 2024

Choose a reason for hiding this comment

eoghanmurray May 7, 2024 • edited

Choose a reason for hiding this comment

eoghanmurray commented Apr 5, 2024 •

edited

changeset-bot bot commented Apr 5, 2024 •

edited

eoghanmurray May 7, 2024 •

edited