fix: nested stylesheets should have absolute URLs #1533

jeffdnguyen · 2024-07-11T22:33:47Z

hello 👋 - I identified an issue with nested stylesheets while looking into the asset manager PR.

The PR has a comment that outlines the problem in more detail

The tl;dr is that for style tags, we are assuming any URLs inside of a style tag must share the same file path as the document the style tag is on. This causes nested stylesheets (think style tag that imports another stylesheet) to have incorrect urls in the snapshot. In order to fix this, I decided the best route would be to determine those absolute paths sooner when stringifying the stylesheets.

changeset-bot · 2024-07-11T22:33:50Z

🦋 Changeset detected

Latest commit: acf1cb9

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

This PR includes changesets to release 18 packages

Name	Type
rrweb-snapshot	Patch
rrweb	Patch
rrdom	Patch
rrdom-nodejs	Patch
rrweb-player	Patch
@rrweb/all	Patch
@rrweb/replay	Patch
@rrweb/record	Patch
@rrweb/types	Patch
@rrweb/packer	Patch
@rrweb/web-extension	Patch
rrvideo	Patch
@rrweb/rrweb-plugin-console-record	Patch
@rrweb/rrweb-plugin-console-replay	Patch
@rrweb/rrweb-plugin-sequential-id-record	Patch
@rrweb/rrweb-plugin-sequential-id-replay	Patch
@rrweb/rrweb-plugin-canvas-webrtc-record	Patch
@rrweb/rrweb-plugin-canvas-webrtc-replay	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

…ate directory

packages/rrweb-snapshot/src/utils.ts

eoghanmurray

I think we just need to avoid the extra .map call as per my other comment; I can make the other changes at a later date.

…ter stringifying imported stylesheet

packages/rrweb-snapshot/test/__snapshots__/integration.test.ts.snap

eoghanmurray · 2024-07-17T13:17:45Z

So something in 27432e2 has caused the test to fail; unfortunately they didn't run automatically after you pushed to the PR, not sure why

eoghanmurray · 2024-07-17T13:18:55Z

I've also got 3 commits in https://github.com/eoghanmurray/rrweb/tree/jn-correct-absolute-urls-for-nested-stylesheets that I'd like you to cherry-pick / pull in here first ... one of them adds a new test.
I'd need some sort of access credentials for pendo-io to be able to commit directly, not sure why that's not the case; maybe it's the 'allow access to maintainers' checkbox in your right hand column for this PR?

jeffdnguyen · 2024-07-17T13:24:14Z

So something in 27432e2 has caused the test to fail; unfortunately they didn't run automatically after you pushed to the PR, not sure why

Good catch! I think I know why, moving the absolultifyURLs call into stringifyRule means I need to use the import rule's stylesheet's href and not the parent stylesheet href. Also need to call it not just after importStringified is populated in the case of a stylesheet with urls that wasn't imported (in this case use the parent href).

jeffdnguyen · 2024-07-17T13:25:26Z

I've also got 3 commits in https://github.com/eoghanmurray/rrweb/tree/jn-correct-absolute-urls-for-nested-stylesheets that I'd like you to cherry-pick / pull in here first ... one of them adds a new test. I'd need some sort of access credentials for pendo-io to be able to commit directly, not sure why that's not the case; maybe it's the 'allow access to maintainers' checkbox in your right hand column for this PR?

Weird...I'm not seeing that checkbox but will look into that. In the meantime happy to cherry pick!

eoghanmurray · 2024-07-17T14:29:03Z

Also I'm urging the rest of the core team to merge in the following soon:
#1437

I imagine there would be extensive conflicts with this one, so if you are at all going back to the drawing board, then we could keep the tests from this PR and build upon that first. If that's too much let's go ahead with this one and I'll handle the rebase on the other.

… to be absolutified

…ied urls

jeffdnguyen · 2024-07-17T15:05:57Z

Update

Cherry picked your commits (thanks for making those!)
Handled that non import case
Clarified with the jpg names as you suggested

packages/rrweb-snapshot/src/utils.ts

eoghanmurray

Just the moving of code out of the try/catch and I think we'll be good to merge

jeffdnguyen · 2024-07-18T13:14:48Z

Made the change! 🤞

eoghanmurray · 2024-07-19T17:38:05Z

I've made a few more changes on top, including playing around with a benchmark as I was worried about the now-per-rule calls to absolutifyURL (which turned out fine)

pendo-io/rrweb@jn-correct-absolute-urls-for-nested-stylesheets...eoghanmurray:rrweb:jn-correct-absolute-urls-for-nested-stylesheets

jeffdnguyen · 2024-07-19T18:16:27Z

Cherry-picked your commits in. Nice job with deduping the fallback behavior, that feels much better! Glad the benchmark turned out fine, did not think to do that 😅

jeffdnguyen · 2024-07-22T20:40:31Z

@eoghanmurray Please let me know if there is anything left I need to do to or can help with to get this merged 😄

Juice10 · 2024-07-23T08:45:57Z

Can we use postcss instead of regex to replace/absolutify urls in the css? We've had a lot of issues with regular expressions not being the same as the spec and having some buggy edge cases. We've added postcss recently which should be much more stable.

jeffdnguyen · 2024-07-23T12:50:42Z

Can we use postcss instead of regex to replace/absolutify urls in the css? We've had a lot of issues with regular expressions not being the same as the spec and having some buggy edge cases. We've added postcss recently which should be much more stable.

@Juice10 I love the idea! Would you be open to me opening a seperate/followup PR to explore that instead? Mainly since I wanted to keep the scope of this PR to changing when we try to absolutify and not the how/implementation of the function.

eoghanmurray · 2024-07-23T22:57:22Z

@Juice10 this PR is for the record side; postcss is used on the replay side, so big -1 from me for introducing postcss dependency for recording. We've been using the absoluteURLs regex for ages already (this doesn't change that) and nobody has reported any issues.

I did explore a couple of further commits on top of this PR which isolate the url regex replace to just the rule bit of the css (and not the selector bit). I had performance rather than correctness in mind with that, as after this PR we are applying the regex to each rule, rather than to the entire block of css. I figured this PR was complete enough so will introduce those changes properly in a new PR if this is accepted.

Side note: postcss could be made redundant at replay time if I complete #1480 successfully.

Juice10 · 2024-07-25T14:24:17Z

@Juice10 this PR is for the record side; postcss is used on the replay side, so big -1 from me for introducing postcss dependency for recording. We've been using the absoluteURLs regex for ages already (this doesn't change that) and nobody has reported any issues.

Fair point @eoghanmurray! Merging this as is, thanks for contributing @jeffdnguyen

The prior 'dynamic stylesheet' route is now the main route for serializing a stylesheet; dynamic stylesheet were missed out in rrweb-io#1533 but are caught in this PR by the tests added in that PR as the stylesheet handling is simplified/centralised

* Replace relative URLs with absolute URLs when stringifying stylesheets * Add test to show desired behavior for imported stylesheets from seperate directory * Rename `absoluteToStylesheet` to `absolutifyURLs` and call it once after stringifying imported stylesheet * Don't create the intermediary array of the spread operator * Formalize that `stringifyRule` should expect a sheet href * Ensure a <style> element can also import and gets it's url absolutized * Handle case where non imported stylesheet has relative urls that need to be absolutified * Clarify in test files where jpegs are expected to appear in absolutified urls * Move absolutifyURLs call for import rules out of trycatch * Add a benchmarking test for stringifyStylesheet * Avoid the duplication on how to fall back --------- Co-authored-by: Eoghan Murray <eoghan@getthere.ie> Co-authored-by: eoghanmurray <eoghanmurray@users.noreply.github.com>

The prior 'dynamic stylesheet' route is now the main route for serializing a stylesheet; dynamic stylesheet were missed out in rrweb-io#1533 but are caught in this PR by the tests added in that PR as the stylesheet handling is simplified/centralised

Support a contrived/rare case where a <style> element has multiple text node children (this is usually only possible to recreate via javascript append) ... this PR fixes cases where there are subsequent text mutations to these nodes; previously these would have been lost * In this scenario, a new CSS comment may now be inserted into the captured `_cssText` for a <style> element to show where it should be broken up into text elements upon replay: `/* rr_split */` * The new 'can record and replay style mutations' test is the principal way to the problematic scenarios, and is a detailed 'catch-all' test with many checks to cover most of the ways things can fail * There are new tests for splitting/rebuilding the css using the rr_split marker * The prior 'dynamic stylesheet' route is now the main route for serializing a stylesheet; dynamic stylesheet were missed out in #1533 but that case is now covered with this PR This PR was originally extracted from #1475 so the initial motivation was to change the approach on stringifying <style> elements to do so in a single place. This is also the motivating factor for always serializing <style> elements via the `_cssText` attribute rather than in it's childNodes; in #1475 we will be delaying populating `_cssText` for performance and instead recorrding them as assets. Thanks for the detailed review to Justin Halsall <Juice10@users.noreply.github.com> & Yun Feng <https://github.com/YunFeng0817>

jeffdnguyen marked this pull request as ready for review July 11, 2024 22:48

jeffdnguyen mentioned this pull request Jul 12, 2024

assets + stylesheet assets #1475

Open

jeffdnguyen force-pushed the jn-correct-absolute-urls-for-nested-stylesheets branch 2 times, most recently from 7c329f9 to 8f6498d Compare July 16, 2024 02:21

jeffdnguyen added 2 commits July 15, 2024 22:30

Replace relative URLs with absolute URLs when stringifying stylesheets

d685bc6

Add test to show desired behavior for imported stylesheets from seper…

c6c0ee3

…ate directory

jeffdnguyen force-pushed the jn-correct-absolute-urls-for-nested-stylesheets branch from 88fbb74 to c6c0ee3 Compare July 16, 2024 02:31

Add changeset

f69e8f3

eoghanmurray reviewed Jul 16, 2024

View reviewed changes

packages/rrweb-snapshot/src/utils.ts Outdated Show resolved Hide resolved

eoghanmurray reviewed Jul 16, 2024

View reviewed changes

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

eoghanmurray reviewed Jul 16, 2024

View reviewed changes

packages/rrweb-snapshot/src/utils.ts Outdated Show resolved Hide resolved

eoghanmurray requested changes Jul 16, 2024

View reviewed changes

Rename absoluteToStylesheet to absolutifyURLs and call it once af…

27432e2

…ter stringifying imported stylesheet

jeffdnguyen force-pushed the jn-correct-absolute-urls-for-nested-stylesheets branch from f26eacf to 27432e2 Compare July 16, 2024 16:51

jeffdnguyen requested a review from eoghanmurray July 16, 2024 16:56

eoghanmurray reviewed Jul 17, 2024

View reviewed changes

packages/rrweb-snapshot/test/__snapshots__/integration.test.ts.snap Outdated Show resolved Hide resolved

eoghanmurray and others added 4 commits July 17, 2024 10:01

Don't create the intermediary array of the spread operator

6ca68b8

Formalize that stringifyRule should expect a sheet href

ddebff5

Ensure a <style> element can also import and gets it's url absolutized

04b3fc6

Apply formatting changes

0c0ceb9

jeffdnguyen added 2 commits July 17, 2024 10:57

Handle case where non imported stylesheet has relative urls that need…

f4bfd73

… to be absolutified

Clarify in test files where jpegs are expected to appear in absolutif…

72f6a68

…ied urls

jeffdnguyen requested a review from eoghanmurray July 17, 2024 15:04

eoghanmurray reviewed Jul 17, 2024

View reviewed changes

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

eoghanmurray reviewed Jul 17, 2024

View reviewed changes

packages/rrweb-snapshot/src/utils.ts Outdated Show resolved Hide resolved

eoghanmurray requested changes Jul 17, 2024

View reviewed changes

Movee absolutifyURLs call for import rules out of trycatch

772f64c

jeffdnguyen requested a review from eoghanmurray July 18, 2024 13:13

eoghanmurray approved these changes Jul 18, 2024

View reviewed changes

eoghanmurray added 4 commits July 19, 2024 14:12

Add a benchmarking test for stringifyStylesheet

8edb9fa

Avoid the duplication on how to fall back

e4d1861

Add explanatory comment

74f3758

Code style: more compact

acf1cb9

Juice10 approved these changes Jul 25, 2024

View reviewed changes

Juice10 merged commit d350da8 into rrweb-io:master Jul 25, 2024
6 checks passed

This was referenced Jul 25, 2024

Version Packages (alpha) #1526

Merged

Version Packages (alpha) pendo-io/rrweb#10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: nested stylesheets should have absolute URLs #1533

fix: nested stylesheets should have absolute URLs #1533

jeffdnguyen commented Jul 11, 2024

changeset-bot bot commented Jul 11, 2024 •

edited

Loading

eoghanmurray left a comment

eoghanmurray commented Jul 17, 2024

eoghanmurray commented Jul 17, 2024 •

edited

Loading

jeffdnguyen commented Jul 17, 2024 •

edited

Loading

jeffdnguyen commented Jul 17, 2024

eoghanmurray commented Jul 17, 2024

jeffdnguyen commented Jul 17, 2024

eoghanmurray left a comment

jeffdnguyen commented Jul 18, 2024

eoghanmurray commented Jul 19, 2024

jeffdnguyen commented Jul 19, 2024

jeffdnguyen commented Jul 22, 2024

Juice10 commented Jul 23, 2024 •

edited

Loading

jeffdnguyen commented Jul 23, 2024

eoghanmurray commented Jul 23, 2024 •

edited

Loading

Juice10 commented Jul 25, 2024

fix: nested stylesheets should have absolute URLs #1533

fix: nested stylesheets should have absolute URLs #1533

Conversation

jeffdnguyen commented Jul 11, 2024

changeset-bot bot commented Jul 11, 2024 • edited Loading

🦋 Changeset detected

eoghanmurray left a comment

Choose a reason for hiding this comment

eoghanmurray commented Jul 17, 2024

eoghanmurray commented Jul 17, 2024 • edited Loading

jeffdnguyen commented Jul 17, 2024 • edited Loading

jeffdnguyen commented Jul 17, 2024

eoghanmurray commented Jul 17, 2024

jeffdnguyen commented Jul 17, 2024

eoghanmurray left a comment

Choose a reason for hiding this comment

jeffdnguyen commented Jul 18, 2024

eoghanmurray commented Jul 19, 2024

jeffdnguyen commented Jul 19, 2024

jeffdnguyen commented Jul 22, 2024

Juice10 commented Jul 23, 2024 • edited Loading

jeffdnguyen commented Jul 23, 2024

eoghanmurray commented Jul 23, 2024 • edited Loading

Juice10 commented Jul 25, 2024

changeset-bot bot commented Jul 11, 2024 •

edited

Loading

eoghanmurray commented Jul 17, 2024 •

edited

Loading

jeffdnguyen commented Jul 17, 2024 •

edited

Loading

Juice10 commented Jul 23, 2024 •

edited

Loading

eoghanmurray commented Jul 23, 2024 •

edited

Loading