1149042 - MutationObserver: MutationRecord.oldValue can be empty string instead of null for previously non-existing style attribute

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Duan Yao]

Attachment: MutationObserver-ff-style.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0
Build ID: 20150306140254

Steps to reproduce:

1. Watch an element with MutationObserver. The element has no 'style' attribute initially.
2. Change the element's style via its "style" IDL property, e.g. "element.style.xxx = 'yyy';".

You can open the attachment to reproduce this issue.


Actual results:

MutationObserver reports that the oldValue of "style" attribute is an empty string.



Expected results:

MutationObserver should report that the oldValue of "style" attribute is null.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

heycam, could you take a look. .style handling is rather odd.

(http://mxr.mozilla.org/mozilla-central/source/layout/style/nsDOMCSSAttrDeclaration.cpp?rev=98a11afc6cb5#94 looks worrisome, for example)

Comment 2

[Boris Zbarsky [:bzbarsky]]

The basic problem here is that the AttributeWillChange is fired from DocToUpdate, which in nsDOMCSSDeclaration::ParsePropertyValue is called _after_ GetCSSDeclaration(true).

If you look at nsDOMCSSAttributeDeclaration::GetCSSDeclaration in the case when it allocates it calls SetInlineStyleRule on the element which lands in nsStyledElementNotElementCSSInlineStyle::SetInlineStyleRule which does SetAttrAndNotify (with aNotify == false).

This was fine for mutation events, where nonexistent attr old values were in fact supposed to be "", afaict.  But if MutationObserver needs to differentiate, then we need to do the AttributeWillChange earlier....

Maybe we could push the AttributeWillChange() down into GetCSSDeclaration; we'd need to indicate to it whether this was a call for an edit or not.  E.g. by changing the boolean arg into a tristate "read, modify-add, modify-remove" or something.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Not doing reviews right now from comment #2)
> This was fine for mutation events, where nonexistent attr old values were in
> fact supposed to be "", afaict.  But if MutationObserver needs to
> differentiate, then we need to do the AttributeWillChange earlier....
> 
Well, MutationEvents don't rely on AttributeWillChange.
And DOM MutationObserver relies on consistent AttributeWillChange behavior.

> Maybe we could push the AttributeWillChange() down into GetCSSDeclaration;
> we'd need to indicate to it whether this was a call for an edit or not. 
> E.g. by changing the boolean arg into a tristate "read, modify-add,
> modify-remove" or something.
That looks doable, thanks.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Well, MutationEvents don't rely on AttributeWillChange.

Ah, ok.  I think the other thing that relied on AttributeWillChange was CSS selector stuff, and this did not care about the exact value for the "style" attr, so worked ok either way.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Heycam, would you have time for this? If not, I'll take it.

Comment 6

[Cameron McCormack (:heycam)]

Yeah I can take a look this week.

Comment 7

[Cameron McCormack (:heycam)]

Will have to be next week.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Not doing reviews right now from comment #2)
> Maybe we could push the AttributeWillChange() down into GetCSSDeclaration;
> we'd need to indicate to it whether this was a call for an edit or not. 
> E.g. by changing the boolean arg into a tristate "read, modify-add,
> modify-remove" or something.

Should it be "read, add, modify", where "add" means call AttributeWillChange with nsIDOMMutationEvent::ADDITION and "modify" means call it with nsIDOMMutationEvent::MODIFICATION?  I'm not sure when we can end up removing the attribute in response to poking at elt.style.

Comment 9

[Boris Zbarsky [:bzbarsky]]

It's not about removing the attribute, or adding it, or modifying it.  It's about removing _properties_ from the declaration.

Specifically, when doing GetCSSDeclaration because we want to do a removeProperty, we do not want to force creation of the CSSDeclaration if it doesn't already exist.

If you look at the current behavior of GetCSSDeclaration, the caller passes a boolean for whether to force creation or not.  It passes false for reads and removeProperty.  In our new world we'd need to differentiate those two, since removeProperty should probably call AttributeWillChange while reads clearly should not.

I'm not sure distinguishing ADDITION vs MODIFICATION in the AttributeWillChange is necessarily worth it; we don't do that now for style attributes.  Do mutation observers care about the distinction between the two, or just the state at the point when AttributeWillChange happens?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

AttributeWillChange needs to be called before the new value has been set so that MutationObserver implementation can get the old value, if needed.
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMMutationObserver.cpp?rev=fc3461fbe72a&mark=135-137#105

Comment 11

[Boris Zbarsky [:bzbarsky]]

Right.  My question is whether that observer cares about ADDITION vs MODIFICATION.  Your link says it does not (aModType is not used in the function).

Comment 12

[Cameron McCormack (:heycam)]

(In reply to Not doing reviews right now from comment #9)
> If you look at the current behavior of GetCSSDeclaration, the caller passes
> a boolean for whether to force creation or not.  It passes false for reads
> and removeProperty.  In our new world we'd need to differentiate those two,
> since removeProperty should probably call AttributeWillChange while reads
> clearly should not.

I understand now. :-)

Comment 13

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f0520277779

Comment 14

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8590049 [details] [diff] [review]
patch

>+function testStyle() {
>+  m = new M(function(records, observer) {
>+    is(records.length, 1, "number of records");
>+    is(records[0].type, "attributes", "record.type");
>+    is(records[0].attributeName, "style", "record.attributeName");
>+    is(records[0].oldValue, null, "record.oldValue");
>+    isnot(div.getAttribute("style"), null, "style attribute after mutation");
>+    observer.disconnect();
>+    m = null;
>+    then();
>+  });
>+  m.observe(div, { attributes: true, attributeOldValue: true });
>+  is(div.getAttribute("style"), null, "style attribute before mutation");
>+  div.style.color = "blue";

Add tests also to check what happens when style attribute is changed from existing value to something else.
And also test that just accessing div.style.color or some such doesn't create any mutation records.

Comment 16

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16d00fd5504a

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/16d00fd5504a