Skip to content

Conversation

@mydea
Copy link
Contributor

@mydea mydea commented Mar 2, 2023

This PR improves masking via maskAllInputs for:

  • <option> elements as part of <select>
  • <input type="checkbox">
  • <input type="radio">

What is masked

For radio/checkbox, we only mask the value="xxx" attribute. So for example:

<input type="radio" value="my-val">

becomes

<input type="radio" value="******">

If no value is specified, nothing is added.

For option tags, we mask both the value attribute on them (so they also match the value of the select), as well as the content of the option - this is IMHO more in line with text input masking than to leave the displayed text.

<select value="val1">
  <option value="val1">Value 1</option>
  <option value="val2">Value 2</option>
</select>

becomes

<select value="****">
  <option value="****">*******</option>
  <option value="****">*******</option>
</select>

@changeset-bot
Copy link

changeset-bot bot commented Mar 2, 2023

🦋 Changeset detected

Latest commit: b310e6f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
rrweb-snapshot Minor
rrweb Minor
rrdom Minor
rrdom-nodejs Minor
rrweb-player Minor
@rrweb/types Minor
@rrweb/web-extension Minor
rrvideo Minor

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

Comment on lines 171 to 188
// Handle `option` like `select
if (tagName.toLowerCase() === 'option') {
tagName = 'select';
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method always gets an upper case tagName, keeping it upper case would save us one toLowerCase call, slightly improving performance

Suggested change
// Handle `option` like `select
if (tagName.toLowerCase() === 'option') {
tagName = 'select';
}
// Handle `option` like `select
if (tagName === 'OPTION') {
tagName = 'SELECT';
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't quite sure here myself, as sometimes tagNames are passed as lowercase, sometimes as uppercase strings 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, actually it is not always lowercase (which nicely the tests caught!).
I've actually changed the type to use Uppercase to ensure that the correct case is passed in. This is supported since TS 4.1, is this OK?

YunFeng0817

This comment was marked as outdated.

Copy link
Member

@YunFeng0817 YunFeng0817 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review. There are so many changes to the Jest snapshot files so I took some time to check them.
I may find an issue with masking radio/checkbox elements. It would be nice to check them. I'm willing to merge this PR after then.

\\"data\\": {
\\"source\\": 5,
\\"text\\": \\"off\\",
\\"text\\": \\"radio-on\\",
Copy link
Member

@YunFeng0817 YunFeng0817 Mar 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may find a bug for masking radio input. In this test, the value "radio-on" is masked in the full snapshot however its actual value is exposed in the input mutation event here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found the culprit and fixed it! 🎉

\\"type\\": 3,
\\"data\\": {
\\"source\\": 5,
\\"text\\": \\"radio-off\\",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@mydea
Copy link
Contributor Author

mydea commented Jul 7, 2023

I rebased & updated this PR on latest master!

@billyvg billyvg force-pushed the fn/mask-select-option branch from 9aa9fbf to b310e6f Compare July 17, 2023 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants