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

fix: Fix CSS rules captured in Safari #86

Merged
merged 4 commits into from
May 1, 2023
Merged

Conversation

mydea
Copy link
Member

@mydea mydea commented Apr 19, 2023

Safari does not escape : in attribute selectors correctly, so we need to do that instead.

See rrweb-io#1208

ref getsentry/sentry-javascript#7703

Safari does not escape `:` in attribute selectors correctly, so we need to do that instead.
@mydea mydea requested a review from billyvg April 19, 2023 11:27
@mydea mydea self-assigned this Apr 19, 2023
Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

I know there's a unit test, but can we verify this doesn't break for Chrome? I can test it when I'm back next week if you're busy with other stuff.

@mydea
Copy link
Member Author

mydea commented Apr 24, 2023

OK, so I tried this, and a problem is that getCssRulesString is currently only called for CSS files & empty (??!) <style> tags, not for stuff in style tags. not sure if it is safe to change this...

@billyvg
Copy link
Member

billyvg commented Apr 24, 2023

Ah right, it might be

function stringifyStyleSheet(sheet: CSSStyleSheet): string {
that we need as well

@bruno-garcia
Copy link
Member

Is it possible this can fix the issue? If not I wonder if there's an alternative work around then so we can unblock folks

@@ -78,7 +91,7 @@ function isCSSImportRule(rule: CSSRule): rule is CSSImportRule {
function stringifyStyleSheet(sheet: CSSStyleSheet): string {
return sheet.cssRules
? Array.from(sheet.cssRules)
.map((rule) => rule.cssText || '')
.map((rule) => rule.cssText ? validateStringifiedCssRule(rule.cssText) : '')
Copy link
Member

Choose a reason for hiding this comment

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

This should work now for inline css

Copy link
Member Author

Choose a reason for hiding this comment

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

nice, could you verify this with an actual example app?

Copy link
Member

Choose a reason for hiding this comment

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

@mydea Tested this out and it seems to work

Copy link
Member Author

Choose a reason for hiding this comment

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

nice, feel free to merge then!

@bruno-garcia
Copy link
Member

Thanks guys!

@billyvg billyvg merged commit 7551bb1 into sentry-v1 May 1, 2023
@billyvg billyvg deleted the fn/fix-safari-capture branch May 1, 2023 15:37
mydea pushed a commit to getsentry/sentry-javascript that referenced this pull request May 8, 2023
- fix: Fix some input masking (esp for radio buttons) (getsentry/rrweb#85)
- fix: Unescaped `:` in CSS rule from Safari (getsentry/rrweb#86)
- feat: Define custom elements (web components) (getsentry/rrweb#87)
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.

3 participants