1197705 - Setting inline style (Element.style) with an invalid value triggers a MutationObserver event

Status: RESOLVED DUPLICATE of bug 1428246

(Core :: DOM: CSS Object Model, defect, P3)

Description

[Duan Yao]

Attachment: MutationObserver-ff-style-invalid-value.html

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150812163655

Steps to reproduce:

Suppose div is an Element object, run the following JS code:
  div.style.width = '-100px';



Actual results:

Firefox fires a MutationObserver event on 'style' attribute with same old value and current value.


Expected results:

Firefox should not fire any MutationObserver event, because the invalid CSS value ('-100px') was ignored and the 'style' attribute was unchanged.

You can reproduce this issue with the attachment. In firefox, it show the following messages in console:

  old value of div/@style : width: 100px; font-size: 20px;
  new value of div/@style : width: 100px; font-size: 20px;

In Chrome and MS Edge, nothing is shown in console.

Comment 1

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

I can't find any spec defining whether mutation observer record should be or should not be created in this case.

Comment 2

[Duan Yao]

I think it is spec'ed in CSSOM:

https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setproperty

>The setProperty(property, value, priority) method must run these steps:
>6. Let component value list be the result of parsing value for property property. Note: value can not include "!important". 
>7. If component value list is null terminate these steps. 

Because the parsing of the CSS value ('-100px') failed, CSSStyleDeclaration.setProperty(property, value, priority) aborted, so 'style' attribute should not be modified.

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #1)
> I can't find any spec defining whether mutation observer record should be or
> should not be created in this case.

I think compatibility with other browsers is probably more valuable than the spec here unless you can see a good reason to do otherwise.  That said, comment 2 sounds good enough for me.

I'd imagine bz might have an opinion here, though.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Not firing a mutation observer notification in this case would make sense to me, yes.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

The spec change would be in w3c/csswg-drafts#2269, and the corresponding test would be added in w3c/web-platform-tests#9376.

I investigated a bit about this for bug 1435139, and found it is less trivial to fix this issue than bug 1435139. When I'm filing a new bug to record my thoughts, I found this bug.

So, currently, the mutation record is queued via nsMutationReceiver::AttributeWillChange invoked from nsDOMCSSAttributeDeclaration::GetCSSDeclaration. At that point, we still don't know whether we will change the value. Actually, it is non-trivial to know whether we are going to change something before we commit that change.

I think there are two possible approaches forward:

One is that, we don't queue mutation record in AttributeWillChange, but instead put it aside as a pending one, and we only queue it when we get it again in AttributeChanged. This way we wouldn't be queuing mutation record for no-op. But this would require us to change how nsMutationReceiver::CurrentRecord works, which needs some thought.

The other is that, we check whether there is any mutation observer wanting the old value of style attribute, and in case there is, we set the declaration block immutable in GetCSSDeclaration, and then move AttributeWillChange call to SetCSSDeclaration. To do this, we may need to iterate over all observers of a node, try to cast them to nsMutationReceiver, and check whether they are interested in old value of style.

Not sure which approach is better.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Xidorn, do you plan to work on this?

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

I don't have plan at this moment, since it is more involving than I thought.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

I'm adding a test for this in https://github.com/w3c/web-platform-tests/pull/10429.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Bug 1428246 fixed the test-cases.