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: Incorrect parsing of functional pseudo class css selector #169

Merged
merged 8 commits into from
Mar 25, 2024

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Mar 21, 2024

This fixes a parsing issue of CSS selectors that use a functional pseudo class with multiple arguments.

For example,

.foo:has(button,div) {}

Would get parsed as 2 selectors: .foo:has(button and div) - this results in an invalid stylesheet and looks like the replay is broken.

This fixes a parsing issue of CSS selectors that use a functional pseudo class with multiple arguments. See the test cases
@billyvg billyvg changed the title fix: Fix incorrect parsing of functional psuedo class css selector fix: Incorrect parsing of functional pseudo class css selector Mar 21, 2024
@billyvg billyvg marked this pull request as ready for review March 21, 2024 21:32
@billyvg billyvg requested review from ryan953 and a team March 21, 2024 21:33
let j = 0;
const len = splitSelectors.length;
const finalSelectors = [];
while (i < len) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The logic is as follows:

  • Iterate through the split selectors and look for the first selector with an opening parenthesis
  • If opening paren is found, look through the rest of the selectors to look for a matching number of closing parentheses
    • If closing paren is found, merge together all selectors between the opening and closing
      • Move iterator index to the next selector and continue
    • If no closing paren is found, assume that selectors were correctly split

Copy link
Member

@c298lee c298lee left a comment

Choose a reason for hiding this comment

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

Tests are passing and comment on logic of the code makes sense

Copy link
Member

@ryan953 ryan953 left a comment

Choose a reason for hiding this comment

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

I'm trying to break it 🗡️

edit: i will check it out, and run these test suggestions to see what happens. Maybe think of more tests
edit2: huzzah! 2/3 of my suggestions break it. The first and the last .bar:has(div, input:is(:disabled), button) {} and .bar:has(input:is(:disabled),.foo,button:is(:disabled)), .foo:has(input, button), .baz, {}

packages/rrweb-snapshot/src/css.ts Outdated Show resolved Hide resolved
packages/rrweb-snapshot/test/css.test.ts Show resolved Hide resolved
packages/rrweb-snapshot/test/css.test.ts Show resolved Hide resolved
packages/rrweb-snapshot/test/css.test.ts Outdated Show resolved Hide resolved
@billyvg
Copy link
Member Author

billyvg commented Mar 22, 2024

@ryan953 good test cases! I've updated to track the net unbalanced across each selector - however this does add another regex

@billyvg billyvg merged commit 810b39f into sentry-v2 Mar 25, 2024
14 checks passed
@billyvg billyvg deleted the fix-incorrect-css-serialization-of-has branch March 25, 2024 17:19
billyvg added a commit that referenced this pull request Apr 19, 2024
billyvg added a commit that referenced this pull request Apr 19, 2024
#169)" (#182)

This reverts commit 810b39f.


Reverting this in favor of rrweb-io#1401
and rrweb-io#1440 which is better than
whatever it is I wrote. I kept our tests just to ensure the fix is
compatible with our previous patch.
billyvg added a commit that referenced this pull request Apr 26, 2024
This fixes a parsing issue of CSS selectors that use a functional pseudo
class with multiple arguments.

For example,

```
.foo:has(button,div) {}
```

Would get parsed as 2 selectors: `.foo:has(button` and `div)` - this
results in an invalid stylesheet and looks like the replay is broken.

---------

Co-authored-by: Ryan Albrecht <[email protected]>
billyvg added a commit that referenced this pull request Jun 5, 2024
…or (#169)" (#182)

This reverts commit de6cd2b, reversing
changes made to 9e07245.
billyvg added a commit that referenced this pull request Jun 7, 2024
This reverts #182 (i.e. we are
going back to #169) as we had
some performance regressions with the CSS changes.

I tested these against the replays that had crashed with the upstream
changes and they seem to work ok.
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.

3 participants