fix(matcher): coalesce mutation observer snapshots per callback batch - #992
Open
mdesrosiers wants to merge 1 commit into
Open
mdesrosiers wants to merge 1 commit into
mdesrosiers wants to merge 1 commit into
Conversation
mutationObserverCallback previously pushed one document snapshot per addedNode across all MutationRecords in a callback invocation. Since a MutationObserver already batches every synchronous DOM change into a single callback call, document.body reflects the same settled state for every record in that batch. Looping over addedNodes therefore queued many duplicate or ancestor-redundant snapshots for what was really one DOM settle event. Each queued snapshot is replayed through axe.run() in runAutomaticCheck, so the duplication multiplies accessibility check cost for no additional coverage: the same final/settled DOM states still get checked, just once per batch instead of once per added node. Capture document.body.innerHTML once per callback invocation instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #992 +/- ##
==========================================
+ Coverage 94.88% 95.21% +0.32%
==========================================
Files 27 32 +5
Lines 626 690 +64
Branches 137 136 -1
==========================================
+ Hits 594 657 +63
- Misses 32 33 +1
🚀 New features to boost your workflow:
|
This file contains hidden or 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
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.
Summary
mutationObserverCallbackin@sa11y/matcher'sautomatic.tspushes one DOM snapshot ontomutatedNodesfor everyaddedNodesentry across allMutationRecords in a single callback invocation:A
MutationObserveralready batches every synchronous DOM change into a single callback call —document.bodyreflects the exact same settled state for every record in that batch. Looping overaddedNodestherefore queues many duplicate/ancestor-redundant snapshots for what is really one DOM settle event (e.g. a single framework re-render that inserts several nested nodes).Each queued snapshot is later replayed through
axe.run()inrunAutomaticCheck'sfor await (const mutated of mutatedNodes)loop, so this duplication multiplies accessibility-check cost. In one of our own test suites (runDOMMutationObserver: trueenabled), the slowest test file went from 75axe.run()calls down to 4 after this fix, for the same set of childList-addition-driven re-renders.Fix
Capture
document.body.innerHTMLonce per callback invocation instead of once per added node:Behavior change beyond deduping — please review carefully
This fix does more than dedupe. Two side effects are worth calling out explicitly, since they affect what gets checked, not just how many times:
observerOptions(subtree,childList,attributes,characterData) has always configured the observer to watch attribute and text mutations, but the old callback only ever queued a snapshot when a record'saddedNodeswas non-empty — so attribute-only changes (e.g.aria-hidden,disabled, class toggles), characterData edits, and pure node removals never triggered a replay check at all. I confirmed this with realMutationRecords captured from an actual observer for each of those three cases:addedNodes.length === 0in all of them, and I verified the old callback made 0 pushes for such records. The new callback pushes once per callback invocation regardless of mutation content, so these previously-unchecked mutation types are now checked. This is arguably fixing a second, latent bug (the observer was configured to watch things the callback then ignored), but it does mean consumers relying onrunDOMMutationObserver: truemay see newaxe.run()invocations — and potentially newly-surfaced a11y violations — for DOM changes that were silently skipped before. It is not simply "fewer duplicate checks of the same coverage."node.parentElement.innerHTML(ornode.outerHTML) — a snippet scoped to the mutated node's ancestor chain — and replayed it by assigning it wholesale todocument.body.innerHTML. That means the old replay was already checking a partial/lossy view of the DOM (discarding content outside that subtree). The new code capturesdocument.body.innerHTMLin full, so each replay now checks the entire settled body. This is likely more correct, but it's a second behavior change bundled with the dedup fix.I don't believe either of these invalidates the fix — if anything, both make the check more correct and more consistent with
observerOptions's stated intent — but I want to flag them explicitly rather than characterize this as a pure performance/dedup change with "no change in coverage," since a consuming test suite could see new (valid) failures after upgrading.Test plan
mutationObserverCallback:MutationRecords (all reporting the same added node, mimicking one childList-based re-render) now queues exactly 1 snapshot per callback call instead of 5.axe.run()calls observed vs. the expected 3), confirming it actually pins the childList-duplication bug rather than trivially passing.MutationRecords produce 0 replay pushes under the old callback and 1 push per batch under the new callback, to characterize the coverage-expansion side effect described above.yarn lint— clean (pre-existing warnings only, unrelated to this change).yarn test— all 16 suites / 217 tests pass (2 pre-existing skips), no regressions.