fix(rrweb-snapshot): align _cssText split points with css rule boundaries - #1920
fix(rrweb-snapshot): align _cssText split points with css rule boundaries#1920creasty wants to merge 1 commit into
Conversation
`applyCssSplits` hands each `/* rr_split */` part to one of the `<style>`'s
text nodes, so that a later mutation can modify an individual node. But the
split points recorded by `markCssSplits` regularly land in the middle of a
rule: `splitCssText` locates them by searching for a text node's content
inside the browser-serialized stylesheet (normalising whitespace to compare,
then converting the offset back), which lands close to the right place but
not exactly on it.
That is harmless while the parts stay adjacent — concatenating the text
nodes gives back the original css. It stops being harmless when a mutation
inserts a sibling text node between two parts, which is exactly what the
splits exist for: a css-in-js library adding a rule at runtime inserts a
text node into the same `<style>`, positioned by `nextId`. The browser then
sees
... color: rgba(0, 0, 0, 0.84.dqzkfX{cursor:pointer; ...
the declaration never closes, and every rule until the parser recovers is
discarded. In a 43-second recording of a styled-components app (~200 text
nodes in one `<style>`, 45 split points) 57 of 311 rules were dropped this
way, leaving two navigation buttons with the UA button appearance while
their neighbours rendered correctly.
Move each split point forward to the end of the rule it lands in before
distributing the parts. Braces inside strings and comments are not counted,
and nested blocks close at their outermost brace. The position of a split
point is a guess to begin with, so nothing is lost by moving it: the
concatenation is unchanged, only which text node holds which rule.
The same alignment is applied to `endIndex` on the `hackCss` path, since
`adaptCssForReplay` changes the length of the css — the existing `searchBit`
scan runs as before and its result is then snapped to a rule boundary in the
rewritten string.
Doing this on the replay side rather than in `splitCssText` also fixes
recordings that already exist, which matters given how long recorder rollout
takes relative to a player deploy.
🦋 Changeset detectedLatest commit: 2ccb13e The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks for your contribution! This is a gnarly bit of rrweb and I originally authored it thinking I was covering some very rare edge cases for correctness.
So that's reproduces the fault, but I would like to see what the original text nodes looked like in a minimal example. I don't believe that I think maybe a new integration test showing browser behaviour might be more valuable to ensure we don't regress this longer term. I'll add a few more comments inline |
| const markedCssText = [ | ||
| '.a { color: red; }.b { col', | ||
| 'or: green; }.c { color: blue; }', | ||
| ].join('/* rr_split */'); |
There was a problem hiding this comment.
This is starting with a broken split if I understand it correctly — is this a simulation of the 'bad/inexact' splitting?
| @@ -248,6 +258,13 @@ export function applyCssSplits( | |||
| // something went wrong, put a similar sized chunk in the right place | |||
| endIndex += cssTextSplits[i].length; | |||
There was a problem hiding this comment.
I'm assuming all the bad cases are produced in here ("something went wrong") so possibly we could confine the nextCssRuleBoundary call to only in here.
I'd also be interested in the input data that caused the above search to go wrong.
There was a problem hiding this comment.
Above comment was written before I realized we are in rebuild here not snapshot ...
Bundle Size ChangesSize change: +28.08 kB (+0.27%) | Total size: 10453.89 kB
|
| File | Base | PR | Diff |
|---|---|---|---|
all.cjs |
567.78 kB | 569.40 kB | +1.62 kB (+0.29%) |
all.js |
567.45 kB | 569.07 kB | +1.62 kB (+0.29%) |
all.umd.cjs |
570.99 kB | 572.64 kB | +1.66 kB (+0.29%) |
all.umd.min.cjs |
269.09 kB | 269.71 kB | +636 B (+0.23%) |
replay - 1407.10 kB -> 1412.63 kB (+5.52 kB (+0.39%))
| File | Base | PR | Diff |
|---|---|---|---|
replay.cjs |
403.84 kB | 405.46 kB | +1.62 kB (+0.40%) |
replay.js |
403.75 kB | 405.37 kB | +1.62 kB (+0.40%) |
replay.umd.cjs |
406.82 kB | 408.47 kB | +1.66 kB (+0.41%) |
replay.umd.min.cjs |
192.70 kB | 193.32 kB | +636 B (+0.32%) |
rrweb - 1913.48 kB -> 1919.00 kB (+5.52 kB (+0.29%))
| File | Base | PR | Diff |
|---|---|---|---|
rrweb.cjs |
550.94 kB | 552.56 kB | +1.62 kB (+0.29%) |
rrweb.js |
550.64 kB | 552.26 kB | +1.62 kB (+0.29%) |
rrweb.umd.cjs |
552.16 kB | 553.81 kB | +1.66 kB (+0.30%) |
rrweb.umd.min.cjs |
259.74 kB | 260.36 kB | +636 B (+0.24%) |
rrweb-player - 1643.88 kB -> 1649.40 kB (+5.52 kB (+0.34%))
| File | Base | PR | Diff |
|---|---|---|---|
rrweb-player.cjs |
473.37 kB | 474.99 kB | +1.62 kB (+0.34%) |
rrweb-player.js |
473.24 kB | 474.87 kB | +1.62 kB (+0.34%) |
rrweb-player.umd.cjs |
476.16 kB | 477.82 kB | +1.66 kB (+0.35%) |
rrweb-player.umd.min.cjs |
221.11 kB | 221.73 kB | +636 B (+0.28%) |
rrweb-snapshot - 609.57 kB -> 615.55 kB (+5.99 kB (+0.98%))
| File | Base | PR | Diff |
|---|---|---|---|
rrweb-snapshot.cjs |
175.01 kB | 176.79 kB | +1.78 kB (+1.01%) |
rrweb-snapshot.js |
174.07 kB | 175.76 kB | +1.69 kB (+0.97%) |
rrweb-snapshot.umd.cjs |
177.48 kB | 179.29 kB | +1.80 kB (+1.02%) |
rrweb-snapshot.umd.min.cjs |
83.00 kB | 83.72 kB | +737 B (+0.87%) |
| // text node holds whole rules rather than half of one | ||
| expect((sn3.childNodes[0] as textNode).textContent).toEqual( | ||
| badStartThird.replace('.a:hover', '.a:hover,\n.a.\\:hover'), | ||
| '.a:hover,\n.a.\\:hover { background-color: red; }', |
There was a problem hiding this comment.
so IIRC the idea with this test was to test actual weird text nodes that don't land in logical css split points ... i.e. the original text nodes were injected arbitrarily ... it's a contrived example designed to show that we don't throw an error trying to parse any of the parts as valid CSS, so I'd reject this test modification, but rather ask you to find the root cause of what the styled-components was actually injecting (was there ever invalid css produced at record time ... if not we should not produce invalid css in our splitting ... that's the challenge)
I get that you want to fix at replay for your existing recordings, but this needs to be fixed at record time, so I'd love to see an example of the 'off by x chars' splits being produced, as that is not the intention for it to be inexact. |
eoghanmurray
left a comment
eoghanmurray
left a comment
There was a problem hiding this comment.
Request changes as I don't think moving boundaries at replay time is correct for all recorded material, and could introduce new errors.
I'd like to see a test page (even as simple as pointing me to a failing production site) so I can see where the algorithm is going wrong in the record time splitting. If that can be identified, along with the associated insertions which are causing the trouble, then we might be able to apply the replay time fix less invasively, e.g. by doing it the prior way first, catching an exception and only then trying to shift things around
2 participants
applyCssSplitshands each/* rr_split */part to one of the<style>'s text nodes. The whole reason those parts are kept separate is the one written inbuildStyleNode's docstring — "in case they are modified by subsequent mutations". But the split points recorded bymarkCssSplitsregularly land in the middle of a rule, and when a mutation then inserts a sibling text node between two parts, the result is invalid css.The problem
splitCssTextdoesn't know where the rule boundaries are. It locates each split point by searching for a text node's content inside the browser-serialized stylesheet, normalising whitespace to compare, then converting the offset back. That lands close to the right place but not exactly on it — routinely a few characters short, in the middle of a declaration:While the parts stay adjacent this is harmless: concatenating the text nodes gives back the original css, and that is what the browser parses. It stops being harmless as soon as something is inserted between them. A css-in-js library (styled-components here) adds rules at runtime by inserting a text node into the same
<style>, positioned bynextId— right between two of the parts. What the browser gets is:The declaration never closes, so the parser goes looking for the closing brace and discards every rule until it recovers.
In a 43-second recording of our app (~200 text nodes in one
<style>, 45 split points), 57 of 311 rules were dropped this way. On screen, two navigation buttons lost their styling entirely and fell back to the UA button appearance (border: outset 2px,background: buttonface) while their neighbours were fine — the difference being only whether their rule happened to sit after a splice point.Reproducing it needs nothing exotic:
The fix
Move each split point forward to the end of the rule it lands in, before distributing the parts (
snapCssSplitsToRuleBoundaries). Braces inside strings and comments aren't counted, and nested blocks (@media,@supports,@keyframes) close at their outermost brace.The exact position of a split point is a guess to begin with, so nothing is lost by moving it — the concatenation is unchanged, only which text node holds which rule. And once every part is a whole number of rules, a sibling inserted between any two of them is harmless.
The same alignment is applied to
endIndexon thehackCsspath:adaptCssForReplaychanges the length of the css, so the existingsearchBitscan is used as before and then snapped to a rule boundary in the rewritten string. This also covers the case where the next part is empty after snapping and the scan has nothing to search for.Why on the replay side
The split points are baked into
_cssTextat record time, so fixingsplitCssTextwould only help recordings made after the change ships. Doing it inapplyCssSplitsfixes recordings that already exist, which matters given how long recorder rollout takes relative to a player deploy.Behaviour change
One existing expectation changes:
applies css splits correctly when split parts are invalid by themselves x3asserted per-node contents that were half-rules. The parts are now whole rules; the test asserts both the new distribution and that the concatenation is unchanged. The other existing cases are unaffected (split points that already sit on a boundary don't move, and the remerge/too-few-nodes paths are untouched).Tests
In
applyCssSplits css rejoiner:And a
snapCssSplitsToRuleBoundariesdescribe covering: already-aligned splits are left alone, a split point moved forward, braces in strings and comments, nested at-rules kept together, trailing splits emptied when everything snapped into an earlier one, and the single-split no-op.All ten fail on
mainand pass with the change.packages/rrweb-snapshot:vitest run141 passed / 1 skipped,tsc --noEmitclean,eslint srcclean.packages/rrwebis unchanged againstmain(the same 11 puppeteer benchmark/integration tests time out on both, in a container).