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

Extended text masking function to include relevant HTMLElement #1310

Merged
merged 13 commits into from
Oct 13, 2023

Conversation

benjackwhite
Copy link
Contributor

@benjackwhite benjackwhite commented Sep 26, 2023

A while back I added the element-to-be-masked to the masking function for inputs. This offers ultimate flexibility as the implementor can re-implement any of the other masking checks or any other thing they can come up with.

What was missing was the corresponding thing for text. This PR adds it.

For example it can solve the request in this issue without needing to add more, conflicting options.

{
    maskAllInputs: true,
    maskInputFn: (text, element) => {
        if (element?.dataset['rr-unmask'] === 'true') {
            return text
        }
        return '*'.repeat(text.length)
    },
    maskTextSelector: "*",
    maskTextFn: (text, element) => {
        if (element?.dataset['rr-unmask'] === 'true') {
            return text
        }
        return '*'.repeat(text.length)
    },
}

With this example, everything will be masked except elements with the data attribute rr-unmask.

@changeset-bot
Copy link

changeset-bot bot commented Sep 26, 2023

🦋 Changeset detected

Latest commit: 0c26420

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

@ArkadyFukzon
Copy link

+1 for this, really useful change

@benjackwhite
Copy link
Contributor Author

@Juice10 made the changes 👍

Copy link
Contributor

@Juice10 Juice10 left a comment

Choose a reason for hiding this comment

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

Great addition @benjackwhite thanks!

Copy link
Contributor

@eoghanmurray eoghanmurray left a comment

Choose a reason for hiding this comment

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

naming suggestion

getElementFromNode sounded like a built in getElementById and hid the 'find parent' aspect.

the "closest" idea has precedence in https://developer.mozilla.org/en-US/docs/Web/API/Element/closest but maybe I'm overthinking it?

packages/rrweb/src/record/mutation.ts Outdated Show resolved Hide resolved
packages/rrweb/src/record/mutation.ts Outdated Show resolved Hide resolved
packages/rrweb/src/utils.ts Outdated Show resolved Hide resolved
packages/rrweb/src/utils.ts Outdated Show resolved Hide resolved
@benjackwhite
Copy link
Contributor Author

@eoghanmurray committed your changes (an then corrected the typos that I didn't initially notice 😅 )
Naming definitely makes sense

@eoghanmurray eoghanmurray merged commit 7c0dc9d into rrweb-io:master Oct 13, 2023
6 checks passed
billyvg pushed a commit to getsentry/rrweb that referenced this pull request Oct 20, 2023
…-io#1310)

* Extends maskTextFn to pass the HTMLElement to the deciding function

---------

Authored-by: benjackwhite <[email protected]>
Co-authored-by: Justin Halsall <[email protected]>
Co-authored-by: Eoghan Murray <[email protected]>
@daibhin daibhin deleted the fix/text-masking-fn branch November 1, 2023 11:01
billyvg pushed a commit to getsentry/rrweb that referenced this pull request Jan 31, 2024
…-io#1310)

* Extends maskTextFn to pass the HTMLElement to the deciding function

---------

Authored-by: benjackwhite <[email protected]>
Co-authored-by: Justin Halsall <[email protected]>
Co-authored-by: Eoghan Murray <[email protected]>
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.

4 participants