Closed
Bug 1192270
Opened 9 years ago
Closed 9 years ago
Attribute mutation is fired after adding and removing the same attribute with mergeAttributeRecords: true set in Mutation Observer
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: simon.lindholm10, Assigned: smaug)
References
Details
When executing: > document.body.setAttribute("a", "b"); > document.body.removeAttribute("a"); the markup view throws an exception at markup-view.js:2741 about calling a function on a null value. This is apparently because with |mergeAttributeRecords: true| the inspector actor now receives a record {attributeName: "a", newValue: null} about a non-existent attribute, which it does not expect. The fix for this in devtools is trivial: > [at inspector.js:816, updateMutation] > - if (!found) { > + if (!found && change.newValue !== null) { but it's not clear to me whether this should be fixed there or in Gecko. Thoughts?
Assignee | ||
Comment 1•9 years ago
|
||
well, MutationObserver doesn't play with new values, so the result is expected. I don't really see how else MO could do merging without loosing information. Well, hmm, if old value is null and the new value is also new, that would mean an attribute is added and then removed. But we don't store the old value unless attributeOldValue is used for observing .
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bugs
Assignee | ||
Comment 2•9 years ago
|
||
Where should I execute document.body.setAttribute("a", "b"); document.body.removeAttribute("a"); to see the exception?
Reporter | ||
Comment 3•9 years ago
|
||
Open the Inspector, then press escape to open the split console. Running it from there should work (and log an exception to the browser console).
Reporter | ||
Comment 4•9 years ago
|
||
The bug seems to have morphed into showing `a="null"` in the Inspector view, rather than failing outright (same behavior in Nightly and Beta). Can we land the one liner mentioned in comment 0?
Comment 5•9 years ago
|
||
(In reply to Simon Lindholm from comment #4) > The bug seems to have morphed into showing `a="null"` in the Inspector view, > rather than failing outright (same behavior in Nightly and Beta). Can we > land the one liner mentioned in comment 0? Yeah, I can go ahead and fix this in the devtools server, at least as a work around. I can file a new bug for that so we can refer back to this one to track any platform fix. Olli, this is a bug in the MutationObserver with mergeAttributeRecords=true, right?
Flags: needinfo?(bugs)
Assignee | ||
Comment 6•9 years ago
|
||
I'd say no. As I said MutationObserver doesn't play with new values and merging just means we tell what the old value was (if old value is observed), and in case of adding attribute it is null. But then merging with attribute removal just keeps that information. But having the record for attribute change doesn't say anything about the existence of the attribute. But I don't understand why we actually get "null" anywhere. I see 2821 mutation.newValue = targetNode.hasAttribute(mutation.attributeName) ? 2822 targetNode.getAttribute(mutation.attributeName) 2823 : null; So that should deal with attribute removals, including the case when there was a merge.
Comment 7•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #6) > I'd say no. > As I said MutationObserver doesn't play with new values and merging just > means we tell what the old value was (if old value is observed), and in case > of adding attribute it is null. But then merging with attribute removal just > keeps that information. But having the record for attribute change doesn't > say anything about the existence of the attribute. > > But I don't understand why we actually get "null" anywhere. > I see > 2821 mutation.newValue = > targetNode.hasAttribute(mutation.attributeName) ? > 2822 > targetNode.getAttribute(mutation.attributeName) > 2823 : null; > So that should deal with attribute removals, including the case when there > was a merge. Yeah, I think the machinery handles this case in general, but this particular sequence of events didn't used to happen before merging started. The fix in Bug 1209342 is handling the case where there is a 'new' attribute in the mutation that has a null value (the condition never happened before since we would get two records in this case). I'll close this bug and finish up the work in Bug 1209342.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(bugs)
Resolution: --- → WONTFIX
Summary: [markup-view] Exception when adding and immediately removing attribute → Attribute mutation is fired after adding and removing the same attribute with mergeAttributeRecords: true set in Mutation Observer
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•