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

rrweb element masking with :not selectors #20100

Closed
pauldambra opened this issue Feb 1, 2024 · 16 comments
Closed

rrweb element masking with :not selectors #20100

pauldambra opened this issue Feb 1, 2024 · 16 comments
Labels
bug Something isn't working right

Comments

@pauldambra
Copy link
Member

see

we've had two reports that masking elements using a :not selector didn't work for the user
a quick check with jsbin showed it working for a trivial example, but two reports is suspicious and we should dig in more thoroughly

e.g.

The maskTextSelector is set to ':not([data-record="true"])'
However, as you can see in the image, the span element has data-record attribute correctly set, but it's still masked in the video.

@FreddieHoy
Copy link

FreddieHoy commented Apr 4, 2024

@pauldambra Hey I've run in to the same issue!

I tried using the example in the docs

session_recording: {
    maskAllInputs: false,
    maskTextSelector: ":not([data-record='true'])",
},
  
<div data-record="true">
  Some copy
</div>

But that didn't work.

Looks like the use of

:not()

Stops it from working. I've tried:

*:not(...)
:not(.ph-show)
:not(#phShow)

But if I am hiding just selective items (instead of the inverse with :not) then it works. IE using:

.hide
#hide
etc..

I'm using tailwind and next.js I wonder if that is an issue?

What other information can I provide?

Do you know when this might be worked on?

Thanks

@pauldambra
Copy link
Member Author

hey @FreddieHoy thanks for letting us know... we're going to focus on "correctness" for a few sprints so might be able to pull this in...

When I made a little reproduction using basic html in htmlfiddle the :not selector worked fine. If you could make a reproducible example that would be amazing

(cc @daibhin)

@FreddieHoy
Copy link

Hey @pauldambra!
Thanks for getting back to me so quickly. Sure I will try creating one and will link it here. Thanks

@FreddieHoy
Copy link

@pauldambra - I've thrown together a repo 😂 thought this was the simplest way to do it while being close to the issue I have. I've left STR in the Readme. Let me know if there are any issues and your able to replicate. Thanks
https://github.com/FreddieHoy/posthog-selector-issue

@pauldambra
Copy link
Member Author

^^^ @daibhin (in case you can look at this today :))

@daibhin
Copy link
Contributor

daibhin commented Apr 5, 2024

Great, thanks @FreddieHoy! Let me reproduce this today and find out where the selector isn't matching

@daibhin
Copy link
Contributor

daibhin commented Apr 5, 2024

This is caused by a bug in our underlying recording tool (rrweb)

The elements are correctly evaluated (the "visible" text returns false)
But its ancestors are also considered... And because of the :not the wrapping div does not pass, hence causing the "visible" text to be masked

Screenshot 2024-04-05 at 15 52 49

I think this bug might have been inadvertently resolved by rrweb-io/rrweb#1349. We're in the process of upgrading to the latest version which includes that change. I will prioritise that upgrade and test this again once shipped

@FreddieHoy
Copy link

@daibhin @pauldambra Fantastic news, thanks for being so quick on this! Legends 🙌

@daibhin
Copy link
Contributor

daibhin commented Apr 8, 2024

It looks like the latest version has changed things again 🙃

https://github.com/rrweb-io/rrweb/blob/3d1877cff83d9a018630674fb6e730050ceef812/packages/rrweb-snapshot/src/snapshot.ts#L984-L996

A new performance improvement does not check child nodes if the parent node is marked as needing to be masked. Here you can see that the html element will need to be masked because it does not match the selector which in turn means that no child will be considered e.g. they will all default to masking
Screenshot 2024-04-08 at 14 06 02

While this feels like a good approach to avoid confusion around nested masking it is technically a breaking change from rrweb (and by proxy us) in cases where customers are trying to mask with selected exclusions (rather than selectively masking).

You can get around this and "mask by default" as follows:

// In your PostHog config options

session_recording: {
    maskTextSelector: "*",
    maskTextFn: (text: string, element: HTMLElement): string => {
        return element?.dataset['record'] === 'true' ? text : '*'.repeat(text.length)
    },
},
  1. Setting maskTextSelector to * will default to masking everything on your page.
  2. For specific elements that you want to show there's the maskTextFn which allows you to override the "masking" of the text to unmask it.

@daibhin
Copy link
Contributor

daibhin commented Apr 8, 2024

Have an update to the docs ready to go so closing this one out

@mwalcher
Copy link

mwalcher commented Apr 9, 2024

@daibhin This still isn't working and I think needs to be reopened. Your proposed solution with maskTextFn doesn't work because that function doesn't have access to the element parameter like the maskInputFn, so we can't check the data attribute exists for text elements, only inputs.

I'm on the latest version of the posthog package and I have a screenshot of the session recording option types:

Screenshot 2024-04-09 101935

@daibhin
Copy link
Contributor

daibhin commented Apr 9, 2024

@mwalcher ah, I think that might just be a typing issue on our end! You can see that the rrweb method does pass out the parent element: https://github.com/rrweb-io/rrweb/blob/master/packages/rrweb-snapshot/src/types.ts#L158

Will reopen until we get PostHog/posthog-js#1122 over the line

@daibhin daibhin reopened this Apr 9, 2024
@daibhin
Copy link
Contributor

daibhin commented Apr 9, 2024

Just checked on the latest version of posthog-js (1.120.3) and the correct types now exist. Thanks again for pointing it out @mwalcher

@daibhin daibhin closed this as completed Apr 9, 2024
@mwalcher
Copy link

mwalcher commented Apr 9, 2024

Thank you for resolving that so quickly @daibhin! I've updated to the latest version and everything is working as expected now.

I just had a suggestion for the docs because I think it will be handy for others as well. I want to use the data attribute to unmask sections of the page, so I'll add the data attribute to a parent element and everything nested in that element will be unmasked.

This is the code I'm using for the maskTextFn:

session_recording: {
    maskTextSelector: "*",
    maskTextFn: (text, element) => {
      const maskedText = '*'.repeat(text.length);
      if (!element) return maskedText;
      const isElementOptedIn = element.closest("[data-record='true']");
      return isElementOptedIn ? text : maskedText;
    },
},

The key here is using element.closest("[data-record='true']") to check for the data attribute instead of element.dataset['record'] === 'true' because it will find the element itself if it has the data attribute or any parent element that has the data attribute. This same function also works for maskInputFn as well.

@lobaorn
Copy link

lobaorn commented Apr 24, 2024

Hi there, one question:

I am using the hobby deployment, and I had this exact problem. The changelog has no mention of this fix, so I am not sure if it applies for hobby deployment or not. If I use the upgrade-hobby script will it use the latest assets versions (for rrweb and posthog-js) or only if it has a new version tag? @daibhin

@daibhin
Copy link
Contributor

daibhin commented Apr 30, 2024

@lobaorn it will depend on the version of the posthog-js you're running and not the Docker version. I suggest making sure that you're running the latest version of the PostHog Web SDK by using the snippet or upgrading the NPM package to v1.120.3 or higher

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working right
Development

No branches or pull requests

5 participants