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 dynamic style serialization #2397

Merged
merged 6 commits into from Sep 4, 2023

Conversation

BenoitZugmeyer
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer commented Aug 29, 2023

Motivation

See #2380. When a website uses a non-empty <style> element, edits it with insertRule or deleteRule, and then starts recording with startSessionReplayRecording(), changes made to the style are lost. Example:

Let's assume the page contains this piece of HTML:

<style>
body { color: red }
</style>

If a script is modifying the <style> state:

document.querySelector('style').sheet.insertRule('body { font-size: 10px }')

And only then we are taking a full snapshot

DD_RUM.startSessionReplayRecording()

The inserted rule will be ignored during serialization. The <style> will be serialized like this:

{
  type: NodeType.Element,
  tagName: 'style',
  attributes: {},
  childNodes: [{
    type: NodeType.Text,
    textContent: 'body { color: red }',
    isStyle: true
  }]
}

This behavior is inherited from rrweb which still has it. There is no justification on why this condition is here in the commit history.

Changes

  • Always use the <style> element state (from its associated StyleSheet) when serializing it.
  • Do not serialize <style> element children anymore
  • Do not define the isStyle property on serialized text nodes

In the above example, the <style> will be serialized like this:

{
  type: NodeType.Element,
  tagName: 'style',
  attributes: { _cssText: 'body { font-size: 10px } body {color: red }' },
  childNodes: []
}

This change is compatible with the current implementation of the player.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

Some tests were incorrectly labeled and hard to grasp. This commit
improves a bit, and adds a test on how style element containing text are
currently serialized.
Previously, we serialized the <style> element state (as reflected by its
associated StyleSheet object) only if the element was empty / did not
have children.

This is an issue when the recording starts after a non-empty <style>
element is edited via `insertRule`/`deleteRule`, as doing so only
changes the <style> element state and not its children, so changes were
ignored.

This commit fixes this issue by always serializing the <style> element
state.
Because the <style> state is now always serialized in the _cssText
attribute, providing children is not useful and implies sending the CSS
text twice in some cases.
When serializing text nodes, there is no need to check whether the
parent element is a <style> since we don't serialize <style> element
children anymore.

The `isStyle` property on the `TextNode` interface is kept for backward
compatibility but won't be defined for newer versions of the SDK.
@BenoitZugmeyer BenoitZugmeyer requested review from a team as code owners August 29, 2023 14:31
@BenoitZugmeyer
Copy link
Member Author

/to-staging

@dd-devflow
Copy link

dd-devflow bot commented Aug 29, 2023

🚂 Branch Integration: starting soon, merge in < 0s

commit 86f65b3ee4 will soon be integrated into staging-35.

This build is going to start soon! (estimated merge in less than 0s)

you can cancel this operation by commenting your pull request with /to-staging -c!

@dd-devflow
Copy link

dd-devflow bot commented Aug 29, 2023

🚂 Branch Integration: this commit was successfully integrated

Commit 86f65b3ee4 has been merged into staging-35 in merge commit 4fd8de709b.

Check out the triggered pipeline on Gitlab 🦊

Browsers are serializing some CSS rules slightly differently. Let's use
CSS that is serialized the same every where.
@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2023

Codecov Report

Merging #2397 (703ba60) into main (ef8f6ca) will decrease coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2397      +/-   ##
==========================================
- Coverage   93.99%   93.99%   -0.01%     
==========================================
  Files         212      212              
  Lines        6298     6292       -6     
  Branches     1405     1399       -6     
==========================================
- Hits         5920     5914       -6     
  Misses        378      378              
Files Changed Coverage Δ
packages/rum/src/domain/record/privacy.ts 97.79% <100.00%> (-0.05%) ⬇️
...domain/record/serialization/serializeAttributes.ts 97.14% <100.00%> (-0.12%) ⬇️
...m/src/domain/record/serialization/serializeNode.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/replay--fix-dynamic-style-serialization branch from 83ae66a to 703ba60 Compare August 29, 2023 16:15
@BenoitZugmeyer BenoitZugmeyer merged commit 0ee210b into main Sep 4, 2023
18 checks passed
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/replay--fix-dynamic-style-serialization branch September 4, 2023 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants