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

Replay fix for Firefox — add <head> and <body> separately #1133

Merged
merged 8 commits into from
Feb 22, 2023

Conversation

eoghanmurray
Copy link
Contributor

This PR solves a problem in Firefox where css transitions are incorrectly being applied upon rebuild. Presumably FF doesn't finished parsing the styles in time, and applies e.g. a default margin:0 to elements which have a non-zero margin set in CSS, along with a transition on them.

I've included a test file which can't be automated at the moment as we use Chrome for repl and live-stream.

To recreate, you'd need to make a repl recording against the transition.html file, save the events to html and then open the file in Firefox (without this PR applied). You'll see the red box very slowly move across the screen, which is incorrect as the styles never had a transition from margin-left:0 to margin-left:400px — it was at the latter from initial page load.

eoghanmurray and others added 3 commits February 14, 2023 12:10
… that (presumably) stylesheet rules are ready to be applied when the body appears

The css which triggered the bug was simply

{
  margin-left: 220px;
  transition: margin-left .448s;
}
…his file://, save the events to a html file, and then open the file in Firefox (without this PR applied)
@changeset-bot
Copy link

changeset-bot bot commented Feb 14, 2023

🦋 Changeset detected

Latest commit: c7a8cd0

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

This PR includes changesets to release 1 package
Name Type
rrweb-snapshot 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

@eoghanmurray eoghanmurray changed the title Firefox replay head style Replay fix for Firefox — add <head> and <body> separately Feb 14, 2023
@YunFeng0817
Copy link
Member

YunFeng0817 commented Feb 14, 2023

Repro: open this link in a firefox browser.
https://rrwebdebug.com/play/index.html?url=https%3A%2F%2Fgist.githubusercontent.com%2FYunFeng0817%2F66c6c742c619812d00cabd10f0f3de6a%2Fraw%2F5a09ceafa3d13baec07d944290c10f9245fd6a8d%2Frepro-firefox-bug-%2525231133.json&version=1.0.0-alpha.4&play=on

Source HTML for reproduction.

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>transition</title>
    <style>
      div {
          width:400px;
          height:400px;
          margin-left:400px;
          background-color:red;
          transition:margin-left 20s;
      }
    </style>
  </head>
  <body>
    <!-- transition shouldn't apply upon rebuild -->
    <div></div>
  </body>
</html>

@eoghanmurray
Copy link
Contributor Author

I've no idea how this fixes the problem.

I've created a bug report for Firefox here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1816672

@eoghanmurray
Copy link
Contributor Author

@YunFeng0817 wondering whether we need to also apply this fix when rebuilding a diff via rrdom, but I can't pinpoint the bit of code in rrdom that applies the diff... do you have any pointers?

Copy link
Member

@YunFeng0817 YunFeng0817 left a comment

Choose a reason for hiding this comment

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

I rewrite your code to make it fit the rrdom and also make it easier to understand.
It will be better to add a change log for this.

packages/rrweb-snapshot/src/rebuild.ts Outdated Show resolved Hide resolved
Also easier to understand

Co-authored-by: Yun Feng <[email protected]>
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