Fix serialization and mutation of <textarea> elements #1351
Conversation
🦋 Changeset detectedLatest commit: 9df40e4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
eoghanmurray
force-pushed
the
textarea-doubles
branch
from
512f48b
to
b5a4594
Compare
eoghanmurray
force-pushed
the
textarea-doubles
branch
3 times, most recently
from
607f794
to
b961dcd
Compare
|
I've added a new aspect to rrweb/test/integration.test.ts in that it now tests the replayer as well as checking the snapshot; the replayer is needed to read out what the actual values of the textarea are after specific mutations are applied. |
|
@eoghanmurray Do we have any blockers to getting this PR merged? After merging this PR, in which version will the fix be available? |
…the duality that the value can be set in either the child node, or in the value _parameter_ (not attribute)
- this is to fix up 'historical' recordings, as duplicate textarea content should no longer be being created at record time (see previous commit in this PR) - new test shows what the snapshot generated by previous versions of rrweb used to look like, hence 'bad' - original 0efe23f fix either didn't work or no longer works due to childNodes being appended subsequent to this part of the code
…he value was recorded as a child node - I didn't notice that form.html was used in other tests, so lucky that I noticed that those tests also should have the 'pre value' masked out
…te (from it's DOM property) and not as a childNode. It should still be rebuilt as a childNode rather than a property
…e was extremely painful to get right from a typings point of view)
eoghanmurray
force-pushed
the
textarea-doubles
branch
from
897fa18
to
71203c7
Compare
|
I've rebased based on upstream; we need a review from another core team member. |
| return document.querySelector('textarea')!; | ||
| }; | ||
|
|
||
| it('should record textarea values once', () => { |
There was a problem hiding this comment.
Maybe we can add some comments here to document that we are doing the inverse of browser behaviour for our serialising (we are using the textarea's value attribute, and ignoring child nodes during serialization, where the browser ignores the value attribute but uses the child text nodes)
…prehension of why we are doing things this way
| id: 6, | ||
| type: 2, | ||
| tagName: 'textarea', | ||
| attributes: { | ||
| value: 'dupe', | ||
| }, | ||
| childNodes: [ | ||
| { | ||
| type: 3, | ||
| textContent: 'dupe should ignore', | ||
| id: 7, | ||
| }, |
There was a problem hiding this comment.
Maybe we can add some comments here to document that we are doing the inverse of browser behaviour for our serialising (we are using the textarea's value attribute, and ignoring child nodes during serialization, where the browser ignores the value attribute but uses the child text nodes)
| type: 2, | ||
| tagName: 'textarea', | ||
| attributes: { | ||
| value: 'dupe', |
There was a problem hiding this comment.
| value: 'dupe', | |
| value: "text that is used in rrweb's replay", |
| childNodes: [ | ||
| { | ||
| type: 3, | ||
| textContent: 'dupe should ignore', |
There was a problem hiding this comment.
| textContent: 'dupe should ignore', | |
| textContent: 'ignored when rrweb's replay reads this serialized child node', |
Bug report: #1301
Fix serialization and mutation of <textarea> elements, taking account the duality that the value can be set in either the child node, or in the value parameter (not attribute)
This was prompted by the #1326 PR but goes much further by adding tests and clarifying that textarea values should always be serialized in the 'value' attribute in the snapshot, and rebuilt as a childNode (textareas are unusual compared with where the value is always in the value attribute/property)
So
In snapshots, I've chosen to always serialize the childNodes into a 'value' attribute, so that masking can be done consistently with other elements (I actually first went with the alternative of masking childNodes; the extra complexity can be seen in the addition of a new
maskInputValuecall in d5b93ac)When text is typed into the textarea by the user, the textarea value is changed but the childnodes stay the same;
if a childNode mutation happens it may or may not affect the value property depending on whether text has already been typed by user. To satisfy this, value mutations are rebuilt into childNodes, and don't modify the
.valueproperty directly; that property should only be modified by a textIncrementalSource.Inputevent.All of this should be covered by examples in the tests.