Bug 70137 – [MutationObservers] Modifications to the style property don't dispatch "attributes" Mutation Records

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Rafael Weinstein]

e.g. div.style.background = "red"; // doesn't dispatch a mutation record of type "attributes"

[Rafael Weinstein]

It looks to me like adding a check in CSSMutableStyleDeclaration::setNeedsStyleRecalc which asks whether there are any observers interested in the style attribute is the way to do this.

If any observer cares about the value, then we synchronously update the style attribute, otherwise we can just dispatch a record inline and leave the style attribute invalid.

Does this seem like a reasonable approach?

[Rafael Weinstein]

Created attachment 116383 [details]
Patch

[Rafael Weinstein]

Patch ready for review.

Note that this patch takes a slightly different patch that the previous comment suggests. In particular, detecting that observers exist during setNeedsStyleRecalc will be too late if any observer requests oldValue, because at that point the change has already taken place and the last value may not have been serialized to the attribute.

[Ryosuke Niwa]

Comment on attachment 116383 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116383&action=review

It's great that this patch is small :) Just need some polishing.

> Source/WebCore/ChangeLog:9
> +        the RAIA pattern similar to the public ChildListMutationScope). This manages the (sometimes conditional)

s/RAIA/RAII/

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:47
> +namespace {

I really don't like these anonymous namespaces.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:49
> +class AttributesMutationScope {

This should be named StyleAttributeMutationScope unless you're planning to use it for other attribute changes as well.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:68
> +        AtomicString styleAtom("style");
> +        s_mutationRecipients = MutationObserverInterestGroup::createForAttributesMutation(s_currentDecl->node(), styleAtom);

Why doesn't createForAttributesMutation just take qualified name? If we can't make that change, we should still be able to pass styleAttr.localName().

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:74
> +        Element* element = static_cast<Element*>(s_currentDecl->node());

We don't have toElement? I swear I've used it before...

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:76
> +        s_mutation = MutationRecord::createAttributes(element, QualifiedName(nullAtom, styleAtom, nullAtom), oldValue);

Why are we creating a qualified name here? We should just use styleAttr. r- because of this.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:102
> +unsigned AttributesMutationScope::s_scopeCount = 0;

This is an integral type so we should be able to initialize it in the class declaration, no?

[Rafael Weinstein]

Comment on attachment 116383 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116383&action=review

>> Source/WebCore/ChangeLog:9
>> +        the RAIA pattern similar to the public ChildListMutationScope). This manages the (sometimes conditional)
> 
> s/RAIA/RAII/

done

>> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:49
>> +class AttributesMutationScope {
> 
> This should be named StyleAttributeMutationScope unless you're planning to use it for other attribute changes as well.

done

>> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:68
>> +        s_mutationRecipients = MutationObserverInterestGroup::createForAttributesMutation(s_currentDecl->node(), styleAtom);
> 
> Why doesn't createForAttributesMutation just take qualified name? If we can't make that change, we should still be able to pass styleAttr.localName().

good call. done.

>> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:74
>> +        Element* element = static_cast<Element*>(s_currentDecl->node());
> 
> We don't have toElement? I swear I've used it before...

done.

>> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:76
>> +        s_mutation = MutationRecord::createAttributes(element, QualifiedName(nullAtom, styleAtom, nullAtom), oldValue);
> 
> Why are we creating a qualified name here? We should just use styleAttr. r- because of this.

done.

>> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:102
>> +unsigned AttributesMutationScope::s_scopeCount = 0;
> 
> This is an integral type so we should be able to initialize it in the class declaration, no?

That's what I thought as well. As adam explained to me, static declarations don't actually reserve storage, so nothing can be initialized. It has to be done as below.

[Adam Klein]

Comment on attachment 116383 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116383&action=review

>>> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:102
>>> +unsigned AttributesMutationScope::s_scopeCount = 0;
>> 
>> This is an integral type so we should be able to initialize it in the class declaration, no?
> 
> That's what I thought as well. As adam explained to me, static declarations don't actually reserve storage, so nothing can be initialized. It has to be done as below.

To be precise, the issue here is the non-constness.  "static const" integral types are allowed to be initialized inline (and don't even necessarily need storage), but non-const integrals both need storage and can't be initialized inline.

[Rafael Weinstein]

Created attachment 116393 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 116393 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116393&action=review

> Source/WebCore/dom/WebKitMutationObserver.cpp:157
> -PassOwnPtr<MutationObserverInterestGroup> MutationObserverInterestGroup::createForAttributesMutation(Node* target, const AtomicString& attributeName)
> +PassOwnPtr<MutationObserverInterestGroup> MutationObserverInterestGroup::createForAttributesMutation(Node* target, const QualifiedName& attributeName)
>  {
> -    return adoptPtr(new MutationObserverInterestGroup(target, WebKitMutationObserver::Attributes, attributeName));
> +    return adoptPtr(new MutationObserverInterestGroup(target, WebKitMutationObserver::Attributes, attributeName.localName()));

Nice!

[Rafael Weinstein]

Created attachment 116397 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116397 [details]
Patch

Clearing flags on attachment: 116397

Committed r101101: <http://trac.webkit.org/changeset/101101>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.