Masking: Avoid the repeated calls to closest when recursing through the DOM
#1349
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 36618ab The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…correct as a newly added text node needs to check all ancestors as well as just the parentNode. This problem suggested a new approach which facilitated the removal of the ugly `needsMask: false` initialization: - needsMask===true means that an ancestor has tested positively for masking, and so this node and all descendends should be masked - needsMask===false means that no ancestors have tested positively for masking, we should check each node encountered - needsMask===undefined means that we don't know whether ancestors are masked or not (e.g. after a mutation) and should look up the tree
…hich is now self evident
… text mutation - modify the test to show where this would have failed
eoghanmurray
changed the title
closest when recursing through the DOMclosest when recursing through the DOM
1 task
Juice10
approved these changes
Nov 23, 2023
This was referenced Dec 4, 2023
billyvg
added a commit
to getsentry/rrweb
that referenced
this pull request
Apr 26, 2024
… through the DOM (rrweb-io#1349)" This reverts commit 07ac5c9.
eoghanmurray
added a commit
to eoghanmurray/rrweb
that referenced
this pull request
Jun 17, 2024
…ements' 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
eoghanmurray
added a commit
that referenced
this pull request
Jun 17, 2024
* Minor fixup for #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 #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.
jeffdnguyen
pushed a commit
to pendo-io/rrweb
that referenced
this pull request
Jul 29, 2024
* 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.
jeffdnguyen
pushed a commit
to pendo-io/rrweb
that referenced
this pull request
Jul 29, 2024
* 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.
jxiwang
pushed a commit
to amplitude/rrweb
that referenced
this pull request
Jul 31, 2024
… the DOM (rrweb-io#1349) * masking performance: avoid the repeated calls to `closest` when recursing through the DOM - needsMask===true means that an ancestor has tested positively for masking, and so this node and all descendents should be masked - needsMask===false means that no ancestors have tested positively for masking, we should check each node encountered - needsMask===undefined means that we don't know whether ancestors are masked or not (e.g. after a mutation) and should look up the tree * Add tests including an explicit characterData mutation tests * Further performance improvement: avoid calls to `el.matches` when on a leaf node, e.g. a `<br/>` --------- Authored-by: eoghanmurray <eoghan@getthere.ie> Based on initial PR rrweb-io#1338 by Alexey Babik <alexey.babik@noibu.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref #1337
This is an alternative take on #1338