Skip to content

Add workaround for Chrome/Edge css import escaping bug#1287

Merged
Juice10 merged 18 commits intomasterfrom
juice10/chromium-import-escape-bug
Aug 11, 2023
Merged

Add workaround for Chrome/Edge css import escaping bug#1287
Juice10 merged 18 commits intomasterfrom
juice10/chromium-import-escape-bug

Conversation

@Juice10
Copy link
Member

@Juice10 Juice10 commented Aug 11, 2023

Edge and Chrome currently have a bug when escaping import statements which can break recording (and playback): https://bugs.chromium.org/p/chromium/issues/detail?id=1472259

This PR adds a workaround for this, and upgrades rrweb to typescript 4.9.5

@changeset-bot
Copy link

changeset-bot bot commented Aug 11, 2023

🦋 Changeset detected

Latest commit: 37d6c8c

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

This PR includes changesets to release 8 packages
Name Type
rrweb-player Patch
rrweb-snapshot Patch
rrweb Patch
@rrweb/types Patch
rrdom Patch
rrdom-nodejs 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

@Juice10 Juice10 changed the title [draft] fix https://bugs.chromium.org/p/chromium/issues/detail?id=1472259 [draft] add workaround for Chrome/Edge css import escaping bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1472259 Aug 11, 2023
@Juice10 Juice10 changed the title [draft] add workaround for Chrome/Edge css import escaping bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1472259 [draft] add workaround for Chrome/Edge css import escaping bug Aug 11, 2023
@Juice10 Juice10 changed the title [draft] add workaround for Chrome/Edge css import escaping bug Add workaround for Chrome/Edge css import escaping bug Aug 11, 2023
@Juice10 Juice10 requested a review from eoghanmurray August 11, 2023 14:36
// browser import didn't work due to CORS
// so we try to construct the import statement ourselves
// and check for browser compatibility issues
fixBrowserCompatibilityIssuesInCSSImports(rule) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why isn't the new function in the catch (error) { return null; } branch of getCssRulesString?
also if there's a particular known CORS exception, we should be testing for it

Copy link
Member Author

@Juice10 Juice10 Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getCssRulesString(rule.styleSheet) (on line 115) contains a catch (error) { return null; } which catches the CORS exception.

To be completely honest I'm not sure if the extra try/catch is necessary in this function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm failing to follow which null it is detecting out of the two possible code paths that return null in getCssRulesString.. I think the fixBrowserCompatibilityIssuesInCSS should be moved closer to the bit that triggers it

@Juice10 Juice10 merged commit efdc167 into master Aug 11, 2023
@Juice10 Juice10 deleted the juice10/chromium-import-escape-bug branch August 11, 2023 15:58
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.

2 participants