fix(masking): Propagate masking and blocking from a shadow host into its shadow root - #319
fix(masking): Propagate masking and blocking from a shadow host into its shadow root#319logaretm wants to merge 2 commits into
Conversation
…its shadow root `distanceToMatch` stopped its ancestor walk at the shadow boundary, since a ShadowRoot is a DOCUMENT_FRAGMENT_NODE and got caught by the non-element check. A mask or block match on an open shadow host was therefore invisible to every node inside that host's shadow tree. Step from a shadow root onto its host instead. `needMaskingText` and `isBlocked` share this walker, so one change covers both. The hop increments the distance, which also makes unmask and unblock selectors inside a shadow root resolve against a matched host, something that previously could not happen at all.
|
Keeping this in draft, I wanted to ask for initial vibe review or any opposition to this. |
|
This change seems reasonable, though the perf explosion would give me pause. [do we add yet another option?] |
|
Do we know the perf explosion is bad/impactful? Realistically this should only impact apps using web components, so not sure if an option makes sense here? Maybe an opt-out option if this really becomes a problem, but I think I'd tend to ship it as-is and add an option if necessary later. Or, we add an option to rrweb but do not expose/use it in replay yet, which would make it easier to adjust this in a follow up if needed? |
|
I think it is a bit unknown as it would cause the walker to go through nodes that it didn't go through before. We could always ship and see if people come in with their use case if affected. |
4 participants
Makes
data-sentry-mask/data-sentry-blockon an open shadow host actually apply to the nodes inside that host's shadow root.distanceToMatchwalked up viaparentNodeand bailed on the first non-element. AShadowRootis aDOCUMENT_FRAGMENT_NODE, so the walk died at the shadow boundary and never reachedshadowRoot.host.The upstream rrweb has the same bug via
el.closest(), which doesn't cross shadow boundaries either, and there's no upstream issue or fix for it 🤔The downside for this is I see the performance exploding for apps that make heavy use of web components, especially ones that don't mask anything in there.
Also this introduces a large behavior change, and maybe two trade offs:
A: mask and unmask both cross into the shadow root. Consistent, but it can silently un-mask content that's masked today. Technically we fixed a bug tho.
B: only mask and block cross. Unmask stays outside. Changes nothing for the default config, and fixes the original report. The cost is that
data-sentry-unmaskon a host silently does nothing, so you can't unmask a third-party component from outside it.The PR currently implements A which I think while more aggressive, is the more consistent and has less "but"s.