fix: inserted styles lost when moving elements #1357

jaj1014 · 2023-11-20T17:27:59Z

hello 👋 - I created an issue a couple weeks back and had some time to dig in a bit more to see if I could figure out a potential solution.

The issue linked above has more detail, but the tl;dr; is that styles which are inserted using CSSStyleSheet.insertRule() are lost during diff operations that result in that style element getting moved.

In order to better demonstrate the problem, I was finally able to create a recording (linked below): If you skip from 0:04 seconds to 0:15, you'll see the teal div at the top loses it's teal color. If you let it play through from 0:04 seconds to 0:15, you'll notice the teal div is still teal. This is because skipping triggers the style element which had the teal styles inserted gets moved as a result of the diff, so the inserted styles get lost. Letting it play through, they aren't lost because diff isn't run and things get mutated normally (w/o virtual dom).

https://rrwebdebug.com/play/index.html?url=https%3A%2F%2Fgist.github.com%2Fjaj1014%2F6e6658d0f24ea297d31edbcfdd44379e&version=2.0.0-alpha.8&virtual-dom=on&play=on

Because the issue has several things contributing to the issue, I wasn't sure where it made the most sense to apply a fix. I took a stab at applying a fix during the diff process inside buildFromNode to copy CSSRules from the dom node into the rrNode for style elements. Later in the diff flow, there is code that will take those style rules copied from the original and apply them.

changeset-bot · 2023-11-20T17:28:03Z

⚠️ No Changeset found

Latest commit: 74f887e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

jaj1014 · 2023-11-27T16:00:11Z

The original approach I took w/ copying cssRules into the virtual nodes created an issue w/ duplicate styles being applied after the diff. This was because that approach wasn't surgical enough and applied to all style sheets - not just the ones that were being moved. To address that, I moved the fix into the diff work to only copy inserted styles over when insertBefore() is being used.

I also added a test case in replayer.test that fails if you run w/o my changes (demonstrating the issue) and passes w/ my changes.

I'm not sure if the written code satisfies rrweb style/standards, so I haven't written tests for the 'util' functions that were added to diff.ts. Happy to adjust where that code lives based on comments. :)

Juice10 · 2023-12-01T12:27:52Z

packages/rrweb/test/replayer.test.ts

@@ -718,7 +735,7 @@ describe('replayer', function () {
      const replayer = new Replayer(events);
      replayer.play();
    `);
-    await page.waitForTimeout(50);
+    await page.waitForTimeout(150);


This is a more reliable way of forcing a wait

Suggested change

await page.waitForTimeout(150);

await waitForRAF(page);

updated and thank you!

Juice10 · 2023-12-01T12:43:51Z

@YunFeng0817 could you check this one out, see if this is the best way to do this in rrdom? @eoghanmurray and I started reviewing this, tests look good, but we're also not sure if this is the best way to implement this.

@jaj1014 Thanks for taking the time to debug this and create a fix for it!

jaj1014 · 2024-04-19T19:28:09Z

Finally got around to adding unit test coverage for the added functions.

eoghanmurray · 2024-05-08T21:30:34Z

packages/rrweb/src/record/iframe-manager.ts

+    if (
+      iframeSourceWindow == window.parent &&
+      window != window.parent &&
+      message.data.snapshot


Annoyingly, typescript (eslint) is failing here; you could define your own message type (see CrossOriginIframeMessageEventContent) and cast to that message type with the as keyword, or maybe the following will work

Suggested change

message.data.snapshot

'snapshot' in message.data

eoghanmurray · 2024-05-08T21:33:12Z

packages/rrweb/src/record/observer.ts

@@ -93,7 +91,7 @@ export function initMutationObserver(
  // see mutation.ts for details
  mutationBuffer.init(options);
  let mutationObserverCtor =


No idea what all the angularZoneSymbol stuff is, but now that this is no longer reassigned by that code, eslint requires us to change to a const

Suggested change

let mutationObserverCtor =

const mutationObserverCtor =

jaj1014 · 2024-05-08T23:20:22Z

sorry @eoghanmurray my branch got screwed up w/ some other commits. I'll try to get it clean up and back to only the changes pertinent to this PR. or worst case, close and open a clean one. sorry about this.

eoghanmurray · 2024-05-09T09:42:33Z

diff with actual and get only inserted styles

The new getInsertedStylesFromElement looks pretty cool; basically you are comparing raw text node rules to derived css rules and figuring out the diffs? This approach could potentially be useful during snapshotting as we've opted to go directly to .sheet.rules in case there were previous programmatically inserted rules (which doesn't always work; see #1322). I've got some in-the-same-space work in progress in #1437 findCssTextSplits()

For this PR though, I feel like it might be overkill and that it adds too much complexity; I feel like the underlying problem should be fixed instead (that elements don't get moved around after diffChildren). But maybe I'm missing something and this PR is the only way of doing it?

jaj1014 · 2024-05-09T10:18:58Z

cae6981 has the changes pertinent to the originally described problem. all the other commits got mixed in unintentionally. unsure if the complexity you're referring to is contained in this one commit, or if you're referring to the others.

you're correct on the gist of the code: it rectifies the issue caused by insertBefore dropping styles that were added with CSSStyleSheet.insertRule(). It does this by creating a copy of the stylesheet without those inserted rules and diffing the copy against the actual (before being moved) to get a map of the inserted styles it can apply after the element is moved.

It's been a while since I've been in this code, and what I thought I know may not even be right. I was thinking that moving elements was just applying the result of diffing with the virtualDom. When not using the virtualDom optimization, this issue doesn't happen. I'm don't know if it is possible to not move elements after diffChildren

eoghanmurray · 2024-05-09T17:25:41Z

Ah okay; if you wish you could cherry-pick that commit onto an up to date trunk and push here.

Until then I'll leave it to @YunFeng0817 to look at the code and see if this is the best approach or if there is a simpler way of achieving the same thing with rrdom.

fix code for nodejs tests change fix direction to avoid issues with duplicate styles format issues swap waitForTimeout for waitForRAF in test that flaked Add unit tests for new functions Fix broken test causes by file formatting removing spaced

billyvg · 2025-01-06T23:27:26Z

I've been trying this out and I think this causes a perf regression. I'm seeing the differ go from ~300-400 to ~600-700ms. The profiler indicates that getInsertedStylesFromElement is forcing reflow quite often.

jaj1014 · 2025-01-09T16:21:51Z

I've been trying this out and I think this causes a perf regression. I'm seeing the differ go from ~300-400 to ~600-700ms. The profiler indicates that getInsertedStylesFromElement is forcing reflow quite often.

Interesting. It makes sense that it's going to cause a performance hit to diff since it's running code that wasn't there before, but it should only run for style elements since those are the ones it's concerned with - and I'm not sure what in that function would cause a reflow...

Was there something specific you saw w/ getInsertedStylesFromElement that was causing the reflow?

The forEach that goes through the inserted styles found by getInsertedStylesFromElement that inserts onto the sheet... I could see each of those possibly triggering a reflow 🤔. But I'm also not super knowledgeable about profiler and didn't do testing for performance when I originally created this PR over a year ago.

jaj1014 · 2025-01-09T20:14:57Z

couldn't resist and was curious...

maybe because it uses innerText (sources: what forces layout/reflow from Paul Irish and an article that outlines differences)

Based on the article, it looks like .textContent instead would avoid the reflow. So line 627 would be: tempStyleSheet.replaceSync(styleElement.textConent);

jaj1014 force-pushed the ajax-copy-css-rules-from-style branch 3 times, most recently from d455e0b to 9c4d539 Compare November 27, 2023 15:46

Juice10 reviewed Dec 1, 2023

View reviewed changes

jaj1014 force-pushed the ajax-copy-css-rules-from-style branch from 9c4d539 to c6de322 Compare December 1, 2023 12:35

YunFeng0817 self-requested a review December 4, 2023 10:43

jaj1014 force-pushed the ajax-copy-css-rules-from-style branch 2 times, most recently from 404c950 to f4537e6 Compare April 19, 2024 17:46

jaj1014 force-pushed the ajax-copy-css-rules-from-style branch 2 times, most recently from fb5143a to 8b10795 Compare April 22, 2024 19:19

eoghanmurray reviewed May 8, 2024

View reviewed changes

eoghanmurray requested changes May 8, 2024

View reviewed changes

eoghanmurray mentioned this pull request May 9, 2024

fix: grid template is parsed incorrectly by chrome #1396

Closed

jaj1014 force-pushed the ajax-copy-css-rules-from-style branch from ef06778 to e87e9df Compare May 23, 2024 13:26

jaj1014 mentioned this pull request May 23, 2024

[Bug]: Injected styles are cleared when node is added to a <style> element #1460

Closed

1 task

jaj1014 and others added 3 commits October 22, 2024 05:13

Apply formatting changes

471c19e

Update test to use vitest

cf4b340

jaj1014 force-pushed the ajax-copy-css-rules-from-style branch from f890bc9 to cf4b340 Compare October 22, 2024 09:34

Apply formatting changes

74f887e

This was referenced Dec 17, 2024

fix: inserted styles lost when moving elements getsentry/rrweb#230

Merged

fix: inserted styles lost when moving elements getsentry/rrweb#233

Merged

billyvg mentioned this pull request Dec 18, 2024

repro: adding node to <style> clears injected styles #1459

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: inserted styles lost when moving elements #1357

fix: inserted styles lost when moving elements #1357

jaj1014 commented Nov 20, 2023

changeset-bot bot commented Nov 20, 2023 •

edited

Loading

jaj1014 commented Nov 27, 2023

Juice10 Dec 1, 2023

jaj1014 Dec 1, 2023

Juice10 commented Dec 1, 2023

jaj1014 commented Apr 19, 2024 •

edited

Loading

eoghanmurray May 8, 2024 •

edited

Loading

eoghanmurray May 8, 2024

jaj1014 commented May 8, 2024

eoghanmurray commented May 9, 2024 •

edited

Loading

jaj1014 commented May 9, 2024

eoghanmurray commented May 9, 2024

billyvg commented Jan 6, 2025

jaj1014 commented Jan 9, 2025

jaj1014 commented Jan 9, 2025

fix: inserted styles lost when moving elements #1357

Are you sure you want to change the base?

fix: inserted styles lost when moving elements #1357

Conversation

jaj1014 commented Nov 20, 2023

changeset-bot bot commented Nov 20, 2023 • edited Loading

⚠️ No Changeset found

jaj1014 commented Nov 27, 2023

Juice10 Dec 1, 2023

Choose a reason for hiding this comment

jaj1014 Dec 1, 2023

Choose a reason for hiding this comment

Juice10 commented Dec 1, 2023

jaj1014 commented Apr 19, 2024 • edited Loading

eoghanmurray May 8, 2024 • edited Loading

Choose a reason for hiding this comment

eoghanmurray May 8, 2024

Choose a reason for hiding this comment

jaj1014 commented May 8, 2024

eoghanmurray commented May 9, 2024 • edited Loading

jaj1014 commented May 9, 2024

eoghanmurray commented May 9, 2024

billyvg commented Jan 6, 2025

jaj1014 commented Jan 9, 2025

jaj1014 commented Jan 9, 2025

changeset-bot bot commented Nov 20, 2023 •

edited

Loading

jaj1014 commented Apr 19, 2024 •

edited

Loading

eoghanmurray May 8, 2024 •

edited

Loading

eoghanmurray commented May 9, 2024 •

edited

Loading