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 #1253

Merged
merged 4 commits into from
Jul 7, 2023

Conversation

mydea
Copy link
Contributor

@mydea mydea commented Jul 7, 2023

Safari does not capture some css selectors correctly, so we need to fix them, or else playback fails for them.

Closes #1208

ref getsentry#86

@changeset-bot
Copy link

changeset-bot bot commented Jul 7, 2023

🦋 Changeset detected

Latest commit: d591538

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

@Juice10
Copy link
Contributor

Juice10 commented Jul 7, 2023

@mydea thanks for submitting this, could you add a Changeset then I can merge this issue?

@mydea
Copy link
Contributor Author

mydea commented Jul 7, 2023

@mydea thanks for submitting this, could you add a Changeset then I can merge this issue?

Added a changeset 👍

@Juice10 Juice10 merged commit c6600e7 into rrweb-io:master Jul 7, 2023
eoghanmurray pushed a commit to eoghanmurray/rrweb that referenced this pull request Jul 27, 2023
* fix: Fix CSS rules captured in Safari

* Apply formatting changes

* add changeset

* fix

---------

Co-authored-by: mydea <mydea@users.noreply.github.com>
eoghanmurray pushed a commit to eoghanmurray/rrweb that referenced this pull request Jul 27, 2023
* fix: Fix CSS rules captured in Safari

* Apply formatting changes

* add changeset

* fix

---------

Co-authored-by: mydea <mydea@users.noreply.github.com>
eoghanmurray pushed a commit to eoghanmurray/rrweb that referenced this pull request Jul 27, 2023
* fix: Fix CSS rules captured in Safari

* Apply formatting changes

* add changeset

* fix

---------

Co-authored-by: mydea <mydea@users.noreply.github.com>
eoghanmurray pushed a commit to eoghanmurray/rrweb that referenced this pull request Aug 3, 2023
* fix: Fix CSS rules captured in Safari

* Apply formatting changes

* add changeset

* fix

---------

Co-authored-by: mydea <mydea@users.noreply.github.com>
eoghanmurray pushed a commit to eoghanmurray/rrweb that referenced this pull request Aug 8, 2023
* fix: Fix CSS rules captured in Safari

* Apply formatting changes

* add changeset

* fix

---------

Co-authored-by: mydea <mydea@users.noreply.github.com>
@eoghanmurray
Copy link
Contributor

In light of performance efforts in #1271 I wonder whether it's desirable to run a browser check before employing the validateStringifiedCssRule function?

I believe (correct me if I'm wrong) that the if (cssStringified.includes(':')) { check will succeed for nearly 100% of css strings, so the regex will be needlessly run in non-safari browsers. Is that correct?

@eoghanmurray
Copy link
Contributor

eoghanmurray commented Aug 8, 2023

@mydea I've done a slight rewrite of the includes test in #1280
if you have a chance, could you validate that that still does the right thing in Safari?

eoghanmurray pushed a commit to eoghanmurray/rrweb that referenced this pull request Aug 8, 2023
* fix: Fix CSS rules captured in Safari

* Apply formatting changes

* add changeset

* fix

---------

Co-authored-by: mydea <mydea@users.noreply.github.com>
@JonasBa
Copy link
Contributor

JonasBa commented Aug 8, 2023

In light of performance efforts in #1271 I wonder whether it's desirable to run a browser check before employing the validateStringifiedCssRule function?

I believe (correct me if I'm wrong) that the if (cssStringified.includes(':')) { check will succeed for nearly 100% of css strings, so the regex will be needlessly run in non-safari browsers. Is that correct?

Correct, + regexp.replace will always allocate a new string for you, so you are also performing an unnecessary copy

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.

[Bug]: Safari does not escape : in CSS rule
4 participants