Skip to content

Add options to mask texts #540

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 6 commits into from
Apr 22, 2021
Merged

Add options to mask texts #540

merged 6 commits into from
Apr 22, 2021

Conversation

re-fort
Copy link
Contributor

@re-fort re-fort commented Apr 17, 2021

This draft PR, which resolves #502, adds options to enable to mask texts.
it depends on rrweb-io/rrweb-snapshot#73

added options:

key default description
maskTextClass 'rr-mask' Use a string or RegExp to configure which elements should be masked
maskTextSelector null Use a string to configure which selector should be masked
maskTextFn - customize mask text content recording logic(e.g. (t) => t.replace(/[a-z]/ig, '*'))

if the direction of implementation looks good, I'll add documentation and tests.

@Yuyz0112
Copy link
Member

Hi @re-fort, thanks for the great PRs!

The design and implementation look good to me, and there are two things I think need a discussion:

  1. From the original request(and what I believe is a good use case), users may need to configure "mask all texts". What should the configs look like in this scenario?
  2. What will be the default mask function? Since different string lengths may cause the DOM layout quite differently, maybe we can choose a default mask function like this: (t) => '*'.repeat(t.length)?

@re-fort
Copy link
Contributor Author

re-fort commented Apr 18, 2021

Thanks @Yuyz0112 !

From the original request(and what I believe is a good use case), users may need to configure "mask all texts". What should the configs look like in this scenario?

I think that can be achieved by passing * to the maskSelector.

What will be the default mask function?

It certainly would be nice to have a default mask function as well as maskInputFn.
textContent might be included special characters like a line feed(\n).
if replacing these characters to *, it may cause a broken layout.
(t) => t.replace(/[\S]/g, '*') might be better, I think.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions/Character_Classes#types

@ucjonathan
Copy link

@re-fort I like the approach you have taken for configuration and usage of this feature.

@Yuyz0112
Copy link
Member

Another suggestion is naming the options like maskClass, maskSelector to maskTextClass, maskTextSelector . This may help users identify the difference between mask text and mask input.

@re-fort
Copy link
Contributor Author

re-fort commented Apr 20, 2021

@Yuyz0112 I revised docs and add some tests.(also rrweb-io/rrweb-snapshot#73)
could you review it again?

@Yuyz0112
Copy link
Member

@re-fort All the things look great to me and I've merged the PR in snapshot repo and release a new version(1.1.2).

Could you help upgrade rrweb-snapshot in this PR and pass the CI?

@re-fort
Copy link
Contributor Author

re-fort commented Apr 22, 2021

@Yuyz0112 Thanks!
I've updated rrweb-snapshot version and passed CI!

@Yuyz0112 Yuyz0112 merged commit 18ad3da into rrweb-io:master Apr 22, 2021
@re-fort re-fort deleted the mask-text branch April 22, 2021 08:28
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.

Masking all text
3 participants