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

fix: nested css selector splitting #21427

Merged
merged 1 commit into from
Apr 9, 2024
Merged

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Apr 9, 2024

Problem

Ticket https://posthoghelp.zendesk.com/agent/tickets/11767

Customers website was not rendering correctly due to a CSS rule of the form

body > ul :is(li:not(:first-of-type) a:hover, li:not(:first-of-type).active a) {
    background: 'red';
}

The CSS parser included in RRWeb attempts to find the selector for a given rule. It uses these selectors to generate hover classes necessary during playback.

However the parser was misinterpreting the , in the above rule and splitting it into two separate selectors e.g. body > ul :is(li:not(:first-of-type) a:hover and li:not(:first-of-type).active a). Both of which were malformed. This broke the rest of the stylesheet and meant that the rules were not being loaded.

Changes

The problem arises from the split regex

.split(/\s*(?![^(]*\)),\s*/)

It does not account for nesting. An arbitrary level of nesting cannot be handled in CSS so I opted to create a new JS function in the parser called splitRootSelectors

The customers website playback now looks perfect!

Does this work well for both Cloud and self-hosted?

Yes

@daibhin daibhin requested a review from a team April 9, 2024 08:56
Copy link
Member

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

Nice debugging and a very detailed PR desc.

eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request Apr 9, 2024
eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request Apr 9, 2024
eoghanmurray added a commit to daibhin/rrweb that referenced this pull request Apr 9, 2024
@daibhin daibhin merged commit cf1234e into master Apr 9, 2024
99 checks passed
@daibhin daibhin deleted the dn-fix/css-selector-split branch April 9, 2024 10:25
eoghanmurray added a commit to rrweb-io/rrweb that referenced this pull request Apr 18, 2024
* better splitting of selectors - overlapping with #1401 
* Add test from example at PostHog/posthog#21427
* ignore brackets inside selector strings
* Add another test as noticed that it's possible to escape strings
* Ensure we are ignoring commas within strings

Co-authored-by: Eoghan Murray <eoghan@getthere.ie>
billyvg pushed a commit to getsentry/rrweb that referenced this pull request Apr 19, 2024
* better splitting of selectors - overlapping with rrweb-io#1401
* Add test from example at PostHog/posthog#21427
* ignore brackets inside selector strings
* Add another test as noticed that it's possible to escape strings
* Ensure we are ignoring commas within strings

Co-authored-by: Eoghan Murray <eoghan@getthere.ie>
billyvg pushed a commit to getsentry/rrweb that referenced this pull request Apr 19, 2024
* better splitting of selectors - overlapping with rrweb-io#1401
* Add test from example at PostHog/posthog#21427
* ignore brackets inside selector strings
* Add another test as noticed that it's possible to escape strings
* Ensure we are ignoring commas within strings

Co-authored-by: Eoghan Murray <eoghan@getthere.ie>
billyvg pushed a commit to getsentry/rrweb that referenced this pull request Apr 19, 2024
* better splitting of selectors - overlapping with rrweb-io#1401 
* Add test from example at PostHog/posthog#21427
* ignore brackets inside selector strings
* Add another test as noticed that it's possible to escape strings
* Ensure we are ignoring commas within strings

Co-authored-by: Eoghan Murray <eoghan@getthere.ie>
jaj1014 pushed a commit to pendo-io/rrweb that referenced this pull request Apr 30, 2024
* better splitting of selectors - overlapping with rrweb-io#1401
* Add test from example at PostHog/posthog#21427
* ignore brackets inside selector strings
* Add another test as noticed that it's possible to escape strings
* Ensure we are ignoring commas within strings

Co-authored-by: Eoghan Murray <eoghan@getthere.ie>
jxiwang pushed a commit to amplitude/rrweb that referenced this pull request Jul 30, 2024
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.

2 participants