Skip to content

Commit

Permalink
Skip mask check on leaf elements (rrweb-io#1512)
Browse files Browse the repository at this point in the history
* Minor fixup for rrweb-io#1349; the 'we can avoid the check on leaf elements' optimisation wasn't being applied as `n.childNodes` was always truthy even when there were no childNodes.

Changing it to `n.childNodes.length` directly there (see rrweb-io#1402) actually caused a bug as during a mutation, we serialize the text node directly, and need to jump to the parentElement to do the check.
This is why I've reimplemented this optimisation inside `needMaskingText` where we are already had an `isElement` test

Thanks to @Paulhejia (https://github.com/Paulhejia/rrweb/) for spotting that `Boolean(n.childNodes)` is aways true.
  • Loading branch information
eoghanmurray authored and jeffdnguyen committed Jul 29, 2024
1 parent 33392b8 commit 9b6b1fd
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 12 deletions.
6 changes: 6 additions & 0 deletions .changeset/skip-mask-check-on-leaf-elements.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"rrweb-snapshot": patch
"rrweb": patch
---

optimisation: skip mask check on leaf elements
29 changes: 17 additions & 12 deletions packages/rrweb-snapshot/src/snapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -328,13 +328,21 @@ export function needMaskingText(
maskTextSelector: string | null,
checkAncestors: boolean,
): boolean {
let el: Element;
if (isElement(node)) {
el = node;
if (!el.childNodes.length) {
// optimisation: we can avoid any of the below checks on leaf elements
// as masking is applied to child text nodes only
return false;
}
} else if (node.parentElement === null) {
// should warn? maybe a text node isn't attached to a parent node yet?
return false;
} else {
el = node.parentElement;
}
try {
const el: HTMLElement | null =
node.nodeType === node.ELEMENT_NODE
? (node as HTMLElement)
: node.parentElement;
if (el === null) return false;
if (maskTextSelector === '*') return true;
if (typeof maskTextClass === 'string') {
if (checkAncestors) {
if (el.closest(`.${maskTextClass}`)) return true;
Expand Down Expand Up @@ -443,7 +451,7 @@ function serializeNode(
mirror: Mirror;
blockClass: string | RegExp;
blockSelector: string | null;
needsMask: boolean | undefined;
needsMask: boolean;
inlineStylesheet: boolean;
maskInputOptions: MaskInputOptions;
maskTextFn: MaskTextFn | undefined;
Expand Down Expand Up @@ -550,7 +558,7 @@ function serializeTextNode(
n: Text,
options: {
doc: Document;
needsMask: boolean | undefined;
needsMask: boolean;
maskTextFn: MaskTextFn | undefined;
maskInputOptions: MaskInputOptions;
maskInputFn: MaskInputFn | undefined;
Expand Down Expand Up @@ -1026,10 +1034,7 @@ export function serializeNodeWithId(
let { needsMask } = options;
let { preserveWhiteSpace = true } = options;

if (
!needsMask &&
n.childNodes // we can avoid the check on leaf elements, as masking is applied to child text nodes only
) {
if (!needsMask) {
// perf: if needsMask = true, children won't also need to check
const checkAncestors = needsMask === undefined; // if false, we've already checked ancestors
needsMask = needMaskingText(
Expand Down

0 comments on commit 9b6b1fd

Please sign in to comment.