This is an archived snapshot of W3C's public bugzilla bug tracker, decommissioned in April 2019. Please see the home page for more details.

Bug 19402 - MutationObservers: When appending a record to the queue the last item needs to be replaced if it represents the same mutation and the new record has an oldValue
Summary: MutationObservers: When appending a record to the queue the last item needs t...
Status: RESOLVED FIXED
Alias: None
Product: WebAppsWG
Classification: Unclassified
Component: DOM (show other bugs)
Version: unspecified
Hardware: All All
: P2 normal
Target Milestone: ---
Assignee: Anne
QA Contact: public-webapps-bugzilla
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-09 17:46 UTC by Erik Arvidsson
Modified: 2012-11-17 19:22 UTC (History)
5 users (show)

See Also:

Attachments

Description Erik Arvidsson 2012-10-09 17:46:46 UTC 
When appending a record to the queue the last item needs to be replaced if it represents the same mutation and the new record has an oldValue.

This can happen if there are more than one registered observer.

var div = document.createElement('div');
var child = div.appendChild(document.createElement('div'));
child.setAttribute('a', 'A');
var observer = new MutationObserver(function() {});
observer.observe(child, {
  attributes: true
});
observer.observe(div, {
  attributes: true,
  subtree: true,
  attributeOldValue: true
});
child.setAttribute('a', 'B');
var records = observer.takeRecords();
assert(records.length === 1);
assert(records[0].type === 'attributes');
assert(records[0].target === child);
assert(records[0].attributeName === 'a');
assert(records[0].oldValue === 'A');

Both Gecko and WebKit pass this test but the spec algorithm creates 2 records. If the tree is deeper and there are more registered observers this can get larger.

The problem this causes is that there can be multiple records delivered to the same observer representing the same mutation.

The proposed solution is to add a an algorithm called append record which takes a list of records and the record to append.

1. If the list is non-empty and the last item in the list represents the same mutation and the current record represents a record with oldValue replace the last item in the list.
2. Otherwise, append the record.

This is a bit vague because we haven't defined what it means that two records represent the same mutation. Same thing goes for defining whether the record has an oldValue or not.
Comment 1 Anne 2012-10-10 09:58:30 UTC 
My understanding thus far is that this affects 'queue an "attributes" record' and 'queue a "characterData" record'. 'queue a "childList" record' is fine as it does not use the oldValue member and therefore does not require this.

To determine equality I suggest we simply check type/target and when type is "attributes" we also check attributeName and attributeNamespace, for the last record in the record queue.

Am I missing something? Mutation observer algorithms still manage to confuse me :/
Comment 2 Rafael Weinstein 2012-10-15 17:52:31 UTC 
Anne,

You are correct that this only affects characterData and attributes (not childList).

I don't think checking the last record in the queue is the right approach. The spec is pretty clear that no-op changes still generate mutation records. So with this approach,

setAttribute('foo', 'bar');
setAttribute('foo', 'bar');

will only generate one record, and I've already seen code in the wild depending on setAttribute *always* creating a mutation record.

I think the "right" way to spec this is unfortunately more complicated. Perhaps Adam can weigh in here. I'd be tempted to implement it the way that webkit (and I'm guessing gecko) does:

1. Define an "Interest Group" which is a mapping of Observer -> Delivery Options, which is initially empty.
2. Start at |target| and walk in the ancestor chain. For each node, visit the registered and transient observers and add them to the map if they care about the given mutation type (and have subtree set if the current node isn't |target|. If a given observer is already present in the map, the Delivery Options are OR'd into the existing options.
3. Now enqueue either the record or recordWithOldValue for each observer, depending on whether the delivery options specify old value.
Comment 3 Olli Pettay 2012-10-15 18:07:44 UTC 
(In reply to comment #2)
> 1. Define an "Interest Group" which is a mapping of Observer -> Delivery
> Options, which is initially empty.
> 2. Start at |target| and walk in the ancestor chain. For each node, visit
> the registered and transient observers and add them to the map if they care
> about the given mutation type (and have subtree set if the current node
> isn't |target|. If a given observer is already present in the map, the
> Delivery Options are OR'd into the existing options.
> 3. Now enqueue either the record or recordWithOldValue for each observer,
> depending on whether the delivery options specify old value.

This sounds pretty much what Gecko does.
Comment 4 Anne 2012-11-08 15:04:10 UTC 
I would like some review first before committing this. I merged the various queue algorithms into one and I believe I have addressed the problem, but I'm not confident: http://html5.org/temp/mo-queue.html
Comment 5 Adam Klein 2012-11-09 12:12:15 UTC 
(In reply to comment #4)
> I would like some review first before committing this. I merged the various
> queue algorithms into one and I believe I have addressed the problem, but
> I'm not confident: http://html5.org/temp/mo-queue.html

Looks good to me, assuming that this doesn't regress the recent change to make removedNodes and addedNodes non-nullable on MutationRecord by only setting them if passed in (I'm not clear on how WebIDL works there).
Comment 6 Anne 2012-11-09 12:53:31 UTC 
When MutationRecord is initialized all its members are set to default values already so that should be fine. This just overrides particular values.

https://github.com/whatwg/dom/tree/142658892ada92c6b857b0142ef5c1cc3270831f

Incidentally this made the specification a little shorter, yay!
Comment 7 Erik Arvidsson 2012-11-09 15:01:32 UTC 
Yeah, this looks(In reply to comment #4)
> I would like some review first before committing this. I merged the various
> queue algorithms into one and I believe I have addressed the problem, but
> I'm not confident: http://html5.org/temp/mo-queue.html

LGTM too
Comment 8 Rafael Weinstein 2012-11-17 18:09:17 UTC 
lgtm.

Typo: "If type is "attributes", options's attributeFilter is non-empty, and either options's attributeFilter not contain name or namespace is non-null, continue."

==> attributeFilter *does* not contain name