Skip to content

Commit 5fbce3a

Browse files
eoghanmurrayjeffdnguyen
authored andcommitted
Skip mask check on leaf elements (rrweb-io#1512)
* 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.
1 parent f3c5f9f commit 5fbce3a

File tree

2 files changed

+23
-12
lines changed

2 files changed

+23
-12
lines changed
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"rrweb-snapshot": patch
3+
"rrweb": patch
4+
---
5+
6+
optimisation: skip mask check on leaf elements

packages/rrweb-snapshot/src/snapshot.ts

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -328,13 +328,21 @@ export function needMaskingText(
328328
maskTextSelector: string | null,
329329
checkAncestors: boolean,
330330
): boolean {
331+
let el: Element;
332+
if (isElement(node)) {
333+
el = node;
334+
if (!el.childNodes.length) {
335+
// optimisation: we can avoid any of the below checks on leaf elements
336+
// as masking is applied to child text nodes only
337+
return false;
338+
}
339+
} else if (node.parentElement === null) {
340+
// should warn? maybe a text node isn't attached to a parent node yet?
341+
return false;
342+
} else {
343+
el = node.parentElement;
344+
}
331345
try {
332-
const el: HTMLElement | null =
333-
node.nodeType === node.ELEMENT_NODE
334-
? (node as HTMLElement)
335-
: node.parentElement;
336-
if (el === null) return false;
337-
if (maskTextSelector === '*') return true;
338346
if (typeof maskTextClass === 'string') {
339347
if (checkAncestors) {
340348
if (el.closest(`.${maskTextClass}`)) return true;
@@ -443,7 +451,7 @@ function serializeNode(
443451
mirror: Mirror;
444452
blockClass: string | RegExp;
445453
blockSelector: string | null;
446-
needsMask: boolean | undefined;
454+
needsMask: boolean;
447455
inlineStylesheet: boolean;
448456
maskInputOptions: MaskInputOptions;
449457
maskTextFn: MaskTextFn | undefined;
@@ -550,7 +558,7 @@ function serializeTextNode(
550558
n: Text,
551559
options: {
552560
doc: Document;
553-
needsMask: boolean | undefined;
561+
needsMask: boolean;
554562
maskTextFn: MaskTextFn | undefined;
555563
maskInputOptions: MaskInputOptions;
556564
maskInputFn: MaskInputFn | undefined;
@@ -1026,10 +1034,7 @@ export function serializeNodeWithId(
10261034
let { needsMask } = options;
10271035
let { preserveWhiteSpace = true } = options;
10281036

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

0 commit comments

Comments
 (0)