-
Notifications
You must be signed in to change notification settings - Fork 166
♻️ [PANA-5105] Serialize all DOM attribute values as strings #3999
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
♻️ [PANA-5105] Serialize all DOM attribute values as strings #3999
Conversation
BeltranBulbarellaDD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and it works as expected!
Tizi42
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
…ibute-values-as-strings * main: (25 commits) 🎨 [PANA-5053] Separate DOM and virtual attribute serialization (#3998) 🐛 clear chain after finalize (#4027) 🎨 [PANA-5222] Make CODEOWNERS more accurate for recording code (#4034) ♻️ Replace longTaskRegistry by longTaskContexts (#4013) 👷 Bump staging to staging-51 Add flagEvaluationEndpointBuilder to TransportConfiguration interface. (#4025) 👷 Enable more renovate flags... (#4023) 🐛 fix developer extension packaging (#4024) 👷 add a script to easily create an access token (#4020) 👷 Ensure that renovate do a full install (#4021) [RUM Browser Profiler] stop profiler when session expires (#4011) 👷 Ensure to have tarballs built at install (#4019) 👷: migrate config renovate.json (#4016) 👷 Update dependency vite to v5.4.21 [SECURITY] (#4015) 👷 Configure test apps dependencies (#4014) v6.25.0 (#4012) 👷 Update dependency vite to v5.4.21 [SECURITY] (#4010) 👷 Include test apps in renovate scan (#4009) 👷 restore canary deployment (#4008) 👷 Update all non-major dependencies (#3980) ...
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 6dade4e | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
Motivation
The current DOM serialization algorithm encodes two specific DOM attributes as boolean values:
checkedattribute found in<input type="checkbox">and<input type="radio">.selectedattribute found in<option selected>.I'd like to avoid this special case handling in the new DOM serialization algorithm. Instead, I'd like to encode these attributes as strings, just like all other attributes. Since these are boolean attributes, the standard string encoding for the value is:
{ selected: '' }(the empty string) for true. (e.g.<option selected>){}(no value recorded) for false. (e.g.<option>)This is exactly the transformation that the session replay player already applies to boolean-valued attributes, so the result will be the same at playback time. Indeed, handling things this way is less work on both ends! The existing player will just work with this approach.
I'd like to take this opportunity to very slightly change the behavior of
<option selected>, resolving an issue that was discovered when building the test suite added in #3994. Currently, there's a very particular circumstance where we do the wrong thing. The problematic situation is when an<option>element is masked, and:selectedDOM attribute.selectedproperty is false. (Because the user has selected another<option>since page load, perhaps.)In this situation, we will add the
selectedDOM attribute to the element's list of attributes. Then, we will see that theselectedproperty is false. We won't update the element's list of attributes, intending to omit theselectedproperty, but because theselectedproperty from the DOM is already there, we'll end up serializing it and including it in the element's attribute list in the recording. This is wrong both from a privacy perspective (we shouldn't capture this attribute on masked elements) and because it could cause us to show the wrong<option>as selected at replay time.The fix is always delete
selectedfrom the element's list of attributes if theselectedproperty is false. This aligns with what we do forchecked, so in addition to fixing this bug, it ends up simplifying the tests and reducing the number of different attribute serialization behaviors we need to reason about.Changes
Following the plan above, this PR:
checkedandselected. Now, the attribute value is the empty string when these attributes are logically true, and the attributes are not recorded when they are logically false.selectedbug described above.serializeDOMAttributes()and related functions, since they can no longer produce boolean values.Test instructions
It's enough to visit a page with an HTML
<select>dropdown, a set of checkbox inputs, or a set of radio button inputs, and look at the generated session replay using the browser SDK extension. We should continue to show the correct checked or selected item after the change.Checklist