posthog icon indicating copy to clipboard operation
posthog copied to clipboard

Improve CSS-in-js support for recordings

Open pauldambra opened this issue 2 years ago • 4 comments

Bug description

We've recently had a few reports of improper rendering on playback and in both cases it was css-in-js not being inlined and then the sources either having changed between recording and playback or not being available to us at playback (e.g. cors issues loading JS assets)

see

  • https://posthoghelp.zendesk.com/agent/tickets/7150
  • https://posthoghelp.zendesk.com/agent/tickets/7108
  • https://posthoghelp.zendesk.com/agent/tickets/11285
  • https://posthoghelp.zendesk.com/agent/tickets/11216

TODO

  • [ ] what support does rrweb have already
    • [ ] are we init-ing incorrectly
  • [ ] test reverse proxy with JS to get around CORS issues
  • [ ] should the reverse proxy be caching assets as well

pauldambra avatar Nov 09 '23 16:11 pauldambra

Internal support request: https://posthoghelp.zendesk.com/agent/tickets/11285

Digging into what is going on here and making notes of everything I discover to aid future investigations...

Styles exist and are being applied on the customers DOM elements: Screenshot 2024-04-02 at 11 22 53

But the referenced stylesheet is empty: Screenshot 2024-04-02 at 11 21 49

How can that be? Well it can be. Looks like calling insertRule will have that effect.

And confirmed in the console: Screenshot 2024-04-02 at 11 24 45 Styles exist but not in the dom.

This means that during playback of recordings the necessary CSS has not been captured which is what causes the incorrect playback issues for CSS-in-JS.


Seems like there is some old RRWeb issues related to insertRule:

https://github.com/rrweb-io/rrweb/issues/563 https://github.com/rrweb-io/rrweb/issues/104

And an issue specifically related to emotion: https://github.com/emotion-js/emotion/issues/1248

There is an open issue with the exact same problem as reported by this customer but with no progress / resolution:

https://github.com/rrweb-io/rrweb/issues/1230


On the emotion side...

Some suggest clashing keys could be causing them issues: https://stackoverflow.com/questions/69980394/emotion-cache-on-ssr-with-material-ui-is-always-empty

daibhin avatar Apr 02 '24 10:04 daibhin

Ok, a ton more investigations into this...

I'm back to thinking this is a capture bug. I've primarily been following a missing class on the button. What was confusing is that the class seemed to exist in the snapshot data... but I've noticed that it is only first included in the next full snapshot.

This suggests that the CSS rule is inserted when the page navigates but RRWeb doesn't capture the insertion. Instead the class only becomes available once a follow up full snapshot is taken (at which point the user might have navigated away from the page in question making it look like the styles never existed)

daibhin avatar Apr 09 '24 13:04 daibhin

Coming back to this as we understand a lot more about how CSS is handled in RRWeb. CSS in JS is supported as the styles should be inlined and reapplied in a style tag.

Recent investigations found that the ast parser included in RRWeb is ported from https://github.com/reworkcss/css. On top of that regex expressions are used to mutate the CSS for two purposes:

  1. Convert pseudo hover selectors to classes (so hover styles can be shown during playback)
  2. To replace {min|max}-device-{width|height} with $1-$2 e.g. min-device-width to min-width

Both of these could be better achieved using AST mutations over regex expressions. The original implementation dates back to 2018

I am attempting to do this in https://github.com/rrweb-io/rrweb/pull/1458

PostCSS seems like the best choice when considering benchmarks and the fact that it is well maintained with commits in the last month versus Rework CSS (which is what is currently used, contains the selector bugs and hasn't had updates in 3+ years) or CSSOM which actively says it is no longer maintained.

(I originally tried CSSTree which seemed promising but adapting the AST was difficult and would probably still rely on the regex expressions)

Getting https://github.com/rrweb-io/rrweb/pull/1458 over the line might take a while so in the meantime we could:

  1. Add an RRWeb plugin that preprocesses the _cssText using postcss ourselves
  2. Patch RRWeb to set skip the mutation internally in the package

daibhin avatar Apr 25 '24 15:04 daibhin

https://github.com/rrweb-io/rrweb/pull/1458 is now ready to go and passing all tests (locally at least). Might take some time to get it over the line so I'm going to move ahead with adapting the changes and applying them directly in PostHog - hoping this will fix a lot of our broken CSS issues

daibhin avatar Apr 30 '24 11:04 daibhin

Update: We're made as much progress on CSS as possible. Our last remaining known issues are around shorthand variables not being correctly parsed during capture - https://github.com/rrweb-io/rrweb/pull/1322.

I think the final improvements for customers will come once https://github.com/rrweb-io/rrweb/pull/1475 lands. We should hold off working on issues likely to be fixed in favour of helping get that PR over the line

daibhin avatar Jun 17 '24 11:06 daibhin