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

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

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

eoghanmurray
Copy link
Contributor

@eoghanmurray eoghanmurray commented May 22, 2024

Note: This PR (by necessity) builds on top of #1437

existing code special cased rel="preload" so that a subsequent attribute mutation from rel="preload" -> rel="stylesheet" would get picked up.

New code deals with any attribute mutation which results in us 'now having a stylesheet link'

as well as completing the TODO for when a stylesheet link href gets updatd

Copy link

changeset-bot bot commented May 22, 2024

🦋 Changeset detected

Latest commit: def3730

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

… checking the replay took me ages to debug as I thought it was something I introducedg
… mutations with `absoluteToStylesheet` - I had accidentally removed that with initial work
…yStylesheet to happen in a single place during initial snapshot.
…n't being assigned to correct text node, and provide remedy whereby css text is properly re-split upon rebuild
…hat the css hasn't been altered server-side/in-transit
…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);
…red twice after a mutation to rel="stylesheet" from rel="preload"
…o rrweb/src/record/asset-manager.ts

 - no need to re-serialize the <link> element; we are only interested in the css contents after it loads
 - add a mutation event with the new css contents if the href is updated and a new stylesheet loads in
 - don't emit two mutation events for the rel="preload" case; we only need the onload event after it becomes rel="stylesheet"
…oad event ... what if the load event has already fired in some browsers (or if the mutation event is delayed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant