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

Buffer modifications to virtual stylesheets #618

Merged
merged 21 commits into from
Jul 8, 2021

Conversation

Juice10
Copy link
Contributor

@Juice10 Juice10 commented Jul 6, 2021

Fixes #563 #525 and (probably) fixes #614
Builds on #529. Special thanks to @VladimirMilenko for doing the initial work on this.

Moves all mutations to virtual stylesheets that aren't part of the dom to virtualStyleRulesMap.
When stylesheets get moved to the virtual dom their styles are also backed up to the virtualStyleRulesMap.

This PR also adds replayer snapshot testing, and some replay unit tests for restoring virtual styles.

VladimirMilenko and others added 21 commits April 3, 2021 23:52
Restore skip duration

Use virtualStyleRulesMap to re-populate stylesheet on Flush event

Clear virtualStyleRulesMap after flush applied
…mer was immediately started and reached the snapshot before the setTimeout returned
…se we'll ignore it after scrubbing (restarting play head at a particular time). This is a problem if mutations have altered the player state, and we try to replay those mutations, so we e.g. try to remove an element that has already been removed because we haven't reset the FullSnapshot state
Only applies changes on flush.

`virtualStyleRulesMap` now works with strings instead of CSSRules.
CSSRules can only be via made `.insertRule` on CSSStyleSheet in most browsers.
And `new CSSStyleSheet()` only works in Chrome currently.
@Juice10 Juice10 changed the title Css rules Buffer modifications to virtual stylesheets Jul 6, 2021
@Yuyz0112
Copy link
Member

Yuyz0112 commented Jul 8, 2021

Although it will be a little easier if we can have two PRs for the stylesheet buffer and the new testing infra, I finally finish the review:)

@Juice10 The testing part looks good to me! And huge thanks for picking @VladimirMilenko's work. Nice teamwork.

@Yuyz0112 Yuyz0112 merged commit 39c8ba1 into rrweb-io:master Jul 8, 2021
a.attributes.style = a.attributes.style!.replace(
coordinatesReg,
'$1: Npx',
);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Juice10 I appreciate the wish to apply standardised formatting to things, and I'm going to be doing that now with my own commits whenever I can (now that you've shown me how with prettify!)
however these type of non-functional changes do have a cost in terms of conflict resolution, e.g. when there's a parallel change in an open pull request (or in a custom commit which is to be maintained on top of the master)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, we should include it in a CI step so it gets applied to all

Copy link
Contributor

@eoghanmurray eoghanmurray Jul 12, 2021

Choose a reason for hiding this comment

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

Do you mean like a failing test, so that when you create a pull request you know to make the formatting changes yourself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eoghanmurray Correct!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There might be certain hooks we could use to do this automatically as well, but I'm not sure what would be a good one for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants