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: masking on textarea #43

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Feb 7, 2023

If textarea has a child string content, it will get duplicated as a value attribute. Masking by text vs input can cause these two values to diverge (i.e. only one is masked). On playback, we remove duplicate textContent for textareas, which means we could show double textarea values: one masked, one unmasked.

If textarea has a child string content, it will get duplicated as a `value` attribute. Masking by text vs input can cause these two values to diverge (i.e. only one is masked). On playback, we remove duplicate textContent for textareas, which means we could show double textarea values: one masked, one unmasked.
@billyvg billyvg changed the base branch from sentry-v1 to feat-mask-placeholder-title-attributes February 7, 2023 03:07
@billyvg billyvg changed the title feat fix mask all text false and mask all inputs textarea fix: masking on textarea Feb 7, 2023
@billyvg billyvg marked this pull request as ready for review February 7, 2023 03:09
@billyvg billyvg requested review from mydea and Lms24 February 7, 2023 03:36
@@ -28,6 +28,7 @@ export function maskInputValue({
value: string | null;
maskInputFn?: MaskInputFn;
}): string {
console.log('maskInputValue', tagName)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this log or is it a debugging leftover?

@@ -590,6 +590,20 @@ describe('record integration tests', function (this: ISuite) {
assertSnapshot(snapshots);
});

it.only('should mask only inputs', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

is the only intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, what happens when I don't have precommit linters

@mydea
Copy link
Member

mydea commented Feb 8, 2023

Merging this fix so it can go out with todays release!

@mydea mydea merged commit f7c1e66 into feat-mask-placeholder-title-attributes Feb 8, 2023
@mydea mydea deleted the feat-fix-mask-all-text-false-and-mask-all-inputs-textarea branch February 8, 2023 09:31
@mydea
Copy link
Member

mydea commented Feb 8, 2023

damn, just noticed this doesn't go into develop, but into another feature branch 😅 hope I didn't mess anything up, sorry. I guess we can just fix this in the next release then 👍

@billyvg billyvg restored the feat-fix-mask-all-text-false-and-mask-all-inputs-textarea branch February 8, 2023 18:34
@billyvg billyvg deleted the feat-fix-mask-all-text-false-and-mask-all-inputs-textarea branch February 8, 2023 18:35
billyvg added a commit that referenced this pull request Feb 8, 2023
If textarea has a child string content, it will get duplicated as a
`value` attribute. Masking by text vs input can cause these two values
to diverge (i.e. only one is masked). On playback, we remove duplicate
textContent for textareas, which means we could show double textarea
values: one masked, one unmasked.
billyvg added a commit that referenced this pull request Feb 24, 2023
Fix bug introduced in #43 where
we masked both textarea's `value` and `textContent`, which meant the
text inside of the textarea would get duplicated. We can ignore
`textContent` in this case and set it to an empty string.
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