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

Apply CSSRule insertion/deltetion on Flush event #529

Conversation

VladimirMilenko
Copy link
Contributor

@VladimirMilenko VladimirMilenko commented Apr 2, 2021

Currently, during the node replacement, the sheet property is being automatically assigned to null.
In order to support virtual sheet mutation, we need to store that sheet in another structure.
We also cannot reuse the existing reference to sheet, as the value at this reference would be removed at the replaceChild call.

To avoid that, we will create:

  private virtualStyleRulesMap!: Map<INode, CSSRule[]>;

This rule array map would keep the latest CSSRule array for each node during the virtual processing of events and would ensure, that at the Flush event, style node has exact same array of css rules, as the one in memory.

I've also added a test which is able to check, whether the stylesheet rule exists in the sheet after insertion.

Fixes #525

Restore skip duration

Use virtualStyleRulesMap to re-populate stylesheet on Flush event

Clear virtualStyleRulesMap after flush applied
@VladimirMilenko VladimirMilenko force-pushed the incremental-snapshot-css-stylesheet-insert-rule branch from efce46f to b3aff90 Compare April 3, 2021 21:52
@VladimirMilenko VladimirMilenko changed the title Fix CSSStyleSheet applying when node doesn't exist in DOM Apply CSSRule insertion/deltetion on Flush event Apr 3, 2021
@Yuyz0112
Copy link
Member

Yuyz0112 commented Apr 8, 2021

LGTM

@Mark-Fenng PTAL

@YunFeng0817
Copy link
Member

A great way to fix the issue.
BTW, is placeholderNode still necessary to exist?

Copy link
Contributor

@Juice10 Juice10 left a comment

Choose a reason for hiding this comment

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

Really loving this, great work @VladimirMilenko!

I found a couple spots where we could make the tests a little more readable

Comment on lines +151 to +158
const rules = Array.from(replayer.iframe.contentDocument.head.children)
.filter(x=>x.nodeName === 'STYLE')
.reduce((acc, node) => {
acc.push(...node.sheet.rules);
return acc;
}, []);

rules.some(x=>x.selectorText === ".css-1fbxx79")
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if we could simplify this a little

Suggested change
const rules = Array.from(replayer.iframe.contentDocument.head.children)
.filter(x=>x.nodeName === 'STYLE')
.reduce((acc, node) => {
acc.push(...node.sheet.rules);
return acc;
}, []);
rules.some(x=>x.selectorText === ".css-1fbxx79")
const rules = [...replayer.iframe.contentDocument.styleSheets].map((sheet)=> sheet.rules);
rules.some(x=>x.selectorText === ".css-1fbxx79")

Comment on lines +198 to +205
const rules = Array.from(replayer.iframe.contentDocument.head.children)
.filter(x=>x.nodeName === 'STYLE')
.reduce((acc, node) => {
acc.push(...node.sheet.rules);
return acc;
}, []);

rules.some(x=>x.selectorText === ".css-1fbxx79")
Copy link
Contributor

Choose a reason for hiding this comment

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

Same goes for here

Suggested change
const rules = Array.from(replayer.iframe.contentDocument.head.children)
.filter(x=>x.nodeName === 'STYLE')
.reduce((acc, node) => {
acc.push(...node.sheet.rules);
return acc;
}, []);
rules.some(x=>x.selectorText === ".css-1fbxx79")
const rules = [...replayer.iframe.contentDocument.styleSheets].map((sheet)=> sheet.rules);
rules.some(x=>x.selectorText === ".css-1fbxx79")

@Juice10
Copy link
Contributor

Juice10 commented Apr 8, 2021

A great way to fix the issue.
BTW, is placeholderNode still necessary to exist?

As far as I can remember it is necessary the first time the .sheet is accessed.
But for performance reasons it would be advisable skip the placeholder/adding-stylesheet-to-dom part when the node already exists in the virtualStyleRulesMap.

@Juice10
Copy link
Contributor

Juice10 commented Jul 3, 2021

It looks like constructible stylesheets (new CSSStyleSheet) is still in the proposal phase and is only implemented in Chrome, and Firefox (behind experimental flag).
In Safari I couldn't get it working directly but I was able to access it via a workaround: Object.create(CSSStyleSheet.prototype)

We might be able to get it working by using this polyfill: https://github.com/calebdwilliams/construct-style-sheets

@Juice10
Copy link
Contributor

Juice10 commented Jul 8, 2021

@VladimirMilenko feel free to close this PR. I ended up incorporating most of it in #618 which fixes this issue.

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.

RRWeb replayer issue with CSSStyleSheetRule insertion replay
4 participants