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

isBlocked factors in the selector #894

Merged
merged 16 commits into from
Aug 30, 2022
Merged

isBlocked factors in the selector #894

merged 16 commits into from
Aug 30, 2022

Conversation

dbseel
Copy link
Contributor

@dbseel dbseel commented May 11, 2022

Support a list of blocked classes in a CSS selector (blockSelector) in addition to the blockClass.
Also ensure only node types are passed into contains.
Also support providing a list of CSS properties to ignore on set/remove stylesheet mutations

@IMFIL
Copy link
Contributor

IMFIL commented May 30, 2022

@Yuyz0112 Hey man, we've added a bunch of changes to Rrweb as we are using it on thousands of websites and noticed some flaws. Let us know if certain things are missing for this to get merged in.

@Juice10
Copy link
Contributor

Juice10 commented May 30, 2022

Thanks for the PR @dbseel! When refactoring and speeding up isBlocked for #903 I also noticed this was missing from rrweb. Happy you've created a PR for it.
Because #903 has changed the guts of isBlocked drastically I expect it to create some merge issues for this branch once it gets merged in.

@Juice10
Copy link
Contributor

Juice10 commented Aug 19, 2022

Hey @dbseel, any chance you could fix the merge conflicts?

@Juice10 Juice10 self-assigned this Aug 19, 2022
@dbseel
Copy link
Contributor Author

dbseel commented Aug 19, 2022

@Juice10 Done. Can we get this merged in? It has been sitting here 3 months, don't want to have to rebase again.

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.

Thanks for rebasing @dbseel, apologies that this has been sitting for so long. Thank you for your fixes! I'm going to do my best to get this merged ASAP for you

I noticed ignoreCSSAttributes isn't documented yet in https://github.com/rrweb-io/rrweb/blob/5f59f9171e481c78d08a1008186cff31a40ee199/guide.md#options could you add one to guide.md and a placeholder (or maybe google translated) to the Chinese guide.zh_CN.md

packages/rrweb/src/utils.ts Outdated Show resolved Hide resolved
packages/rrweb/src/utils.ts Outdated Show resolved Hide resolved
packages/rrweb-snapshot/src/snapshot.ts Outdated Show resolved Hide resolved
@dbseel dbseel requested a review from Juice10 August 29, 2022 15:49
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.

Thanks for this @dbseel! Really appreciate your contribution. Apologies that it took so long for us to review it

@dbseel
Copy link
Contributor Author

dbseel commented Aug 29, 2022

Thanks for this @dbseel! Really appreciate your contribution. Apologies that it took so long for us to review it

No problem @Juice10! Are you able to merge? I don't have permissions.

@Juice10
Copy link
Contributor

Juice10 commented Aug 29, 2022

I don't but I've messaged the other members of the core team, so it should get merged soon

Copy link
Member

@Yuyz0112 Yuyz0112 left a comment

Choose a reason for hiding this comment

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

good job!

@Yuyz0112 Yuyz0112 merged commit 5ba933c into rrweb-io:master Aug 30, 2022
@Juice10 Juice10 added the 2.0 label Dec 1, 2022
@eoghanmurray
Copy link
Contributor

Late to the party here!

@dbseel what are the use cases for ignoreCSSAttributes? I see color in the test case but curious as to the others. Is the motivation for this privacy, or performance (like slimDOM options).

Also wondering whether ignoreCSSProperties would have been a better name?

@dbseel
Copy link
Contributor Author

dbseel commented Feb 28, 2023

@dbseel what are the use cases for ignoreCSSAttributes?

We have encountered some websites where certain attributes were constantly being added/removed/updated on the DOM, but they did not affect the replay. So there was a huge performance improvement by ignoring these attributes.

@eoghanmurray
Copy link
Contributor

These are CSS rule attributes rather than DOM attributes though?
I guess just thinking it must be very unusual that rules would be continually added/removed.
Was it a standard part of some framework, or just some once off craziness on a particular website?

@dbseel
Copy link
Contributor Author

dbseel commented Mar 1, 2023

These are CSS rule attributes rather than DOM attributes though? I guess just thinking it must be very unusual that rules would be continually added/removed. Was it a standard part of some framework, or just some once off craziness on a particular website?

In our case a website was using quantum metrics. This plugin was adding/removing attributes thousands of times a second. Attributes like this: --quantum-metric-background-image

@eoghanmurray
Copy link
Contributor

I've just come across this again today; I notice that ignoreCSSAttributes doesn't omit them from capture from the initial snapshot. This wouldn't be significant for your usecase of mutations to --quantum-metric-background-image, but for completeness does anyone think it needs to be added?

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

Successfully merging this pull request may close these issues.

5 participants