rrweb icon indicating copy to clipboard operation
rrweb copied to clipboard

[Bug]: Regression in rrweb style capture for material ui

Open arijit91 opened this issue 3 years ago • 8 comments

Preflight Checklist

  • [X] I have searched the issue tracker for a bug report that matches the one I want to file, without success.

What package is this bug report for?

rrweb

Expected Behavior

rrweb is not capturing styling changes coming from material-ui

Sample app (button color is expected to change from red to blue on click): https://ckn2h.csb.app/#

This was previously reported and fixed in https://github.com/rrweb-io/rrweb/issues/650

It is broken on main

I did a binary search and looks like this was the commit that broke it: https://github.com/rrweb-io/rrweb/commit/a01c97f70da6583400d2759f26a518037d4c2adf

I think the commit is causing duplicate style rules to be created on the DOM node

Actual Behavior

Sample app (button color is expected to change from red to blue on click): https://ckn2h.csb.app/#

Testcase Gist URL

No response

Additional Information

No response

arijit91 avatar Jul 02 '22 13:07 arijit91

Do you think https://github.com/rrweb-io/rrweb/commit/9ed6767adac4e0afab66fc3fb9716ab87bfc81f8 fixed this?

Yuyz0112 avatar Jul 02 '22 14:07 Yuyz0112

@Yuyz0112 I thought so as well actually, I tried that patch but it didn't seem to work :(

The bug exists on main

arijit91 avatar Jul 02 '22 14:07 arijit91

ok, going to debug it

Yuyz0112 avatar Jul 02 '22 14:07 Yuyz0112

From what I could make out, there is a css rule we pick up on with the css style declaration observer, we add it to the node, and then we try to append a node with the same text content from a DOM mutation, so we have 2 (duplicate) styles on the same node

Then while replaying the DOM mutation, only one of the css rules is updated..

arijit91 avatar Jul 02 '22 14:07 arijit91

you are right, the issue was introduced in https://github.com/rrweb-io/rrweb/commit/a01c97f70da6583400d2759f26a518037d4c2adf. And I'm sorry I did not add enough information about what it's trying to fix.

Now let us add comprehensive test cases for the stylesheet recording to ensure we handle things correctly without introducing further regression.

  • remote link element, use href to load a stylesheet
  • style element, apply stylesheet as text content inside it
    • existed when snapshot starting
    • dynamic created and recorded by observers
  • style element, apply stylesheet by using CSS APIs, without text content inside it
    • existed when snapshot starting
    • dynamic created and recorded by observers

I think some duplication of case 2 and case 3 leads these issues, https://github.com/rrweb-io/rrweb/commit/9ed6767adac4e0afab66fc3fb9716ab87bfc81f8 fixed part of it, but not all.

Yuyz0112 avatar Jul 03 '22 05:07 Yuyz0112

@Juice10 are there any cases I'm missing here?

I think maybe we can only use CSS APIs' observers to record style, without serializing style elements' text content.

Yuyz0112 avatar Jul 03 '22 05:07 Yuyz0112

@Yuyz0112 I have seen some combinations of the the last two cases you mentioned.

Also some style element can have multiple text nodes added inside them (via JS), which can lead to some unexpected behavior.

And another thing to keep in mind are some cssRules (like media queries) can have more cssRules inside them. It's possible that serializing isn't accounting for this and those rules aren't correctly captured.

Juice10 avatar Jul 03 '22 08:07 Juice10

@Yuyz0112 We've been seeing this issue for websites using MaterialUI, and I think it's also impacting websites with ionic components too.

Any thoughts on what a fix might look like? I imagine this is pretty wide spread

And another thing to keep in mind are some cssRules (like media queries) can have more cssRules inside them. It's possible that serializing isn't accounting for this and those rules aren't correctly captured.

I can confirm that we're hitting this specific case with MaterialUI. We get a crash in the player when it attempts to add cssRules to a media query. If it's helpful, I can try to pull together an example.

rcmarron avatar Jul 28 '22 01:07 rcmarron

@Yuyz0112 would https://github.com/rrweb-io/rrweb/pull/989 have fixed this issue as well? We are hitting this as we are using MaterialUI web components.

ChetanGoti avatar Nov 28 '22 12:11 ChetanGoti