Skip to content

feat: With maskAllText, mask the attributes: placeholder, title, aria-label #40

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

Merged
merged 5 commits into from
Feb 8, 2023

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Feb 2, 2023

If maskAllText is enabled, let's mask some specific attributes: placeholder, title, and aria-label. I've opted to just hardcode these instead of adding them as options for now since they are only tied to maskAllText.

Depends on #35

@billyvg billyvg changed the base branch from sentry-v1 to feat-native-mask-all-text-option February 2, 2023 22:24
@billyvg billyvg force-pushed the feat-native-mask-all-text-option branch 2 times, most recently from 60d7d40 to 326ab31 Compare February 3, 2023 19:12
@billyvg billyvg force-pushed the feat-mask-placeholder-title-attributes branch from e69e2aa to eec6a2f Compare February 3, 2023 19:36
@billyvg billyvg changed the title feat mask placeholder title attributes feat: With maskAllText, mask the attributes: placeholder, title, aria-label Feb 3, 2023
@billyvg billyvg requested review from mydea and Lms24 February 3, 2023 19:40
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -6,50 +6,50 @@ export declare enum NodeType {
CDATA = 4,
Comment = 5
}
export declare type documentNode = {
export type documentNode = {
Copy link
Member

Choose a reason for hiding this comment

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

No action required, just curious: Why remove the declare here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this was auto generated? I'll undo and see what happens

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah not sure what happened before, but export type is the correct statement (it also watches others in the file for type).

Base automatically changed from feat-native-mask-all-text-option to sentry-v1 February 8, 2023 18:32
Similar to `maskAllInputs`, this option will mask all text nodes (except those defined by `unmaskSelector`)
Mask the title and placeholder attributes when `maskAllText` is enabled.
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 force-pushed the feat-mask-placeholder-title-attributes branch from f7c1e66 to 736070a Compare February 8, 2023 18:37
@billyvg billyvg merged commit 9f70e0e into sentry-v1 Feb 8, 2023
@billyvg billyvg deleted the feat-mask-placeholder-title-attributes branch February 8, 2023 20:18
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.

2 participants