Bug 70137 - [MutationObservers] Modifications to the style property don't dispatch "attributes" Mutation Records
Summary: [MutationObservers] Modifications to the style property don't dispatch "attri...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Weinstein
URL:
Keywords:
Depends on:
Blocks: 68729
  Show dependency treegraph
 
Reported: 2011-10-14 13:35 PDT by Rafael Weinstein
Modified: 2011-11-23 13:40 PST (History)
7 users (show)

See Also:


Attachments
Patch (18.86 KB, patch)
2011-11-23 11:07 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (22.05 KB, patch)
2011-11-23 12:08 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (22.04 KB, patch)
2011-11-23 12:29 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Weinstein 2011-10-14 13:35:56 PDT
e.g. div.style.background = "red"; // doesn't dispatch a mutation record of type "attributes"
Comment 1 Rafael Weinstein 2011-10-14 14:08:59 PDT
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?
Comment 2 Rafael Weinstein 2011-11-23 11:07:39 PST
Created attachment 116383 [details]
Patch
Comment 3 Rafael Weinstein 2011-11-23 11:10:00 PST
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.
Comment 4 Ryosuke Niwa 2011-11-23 11:35:34 PST
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?
Comment 5 Rafael Weinstein 2011-11-23 11:59:45 PST
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.
Comment 6 Adam Klein 2011-11-23 12:02:32 PST
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.
Comment 7 Rafael Weinstein 2011-11-23 12:08:22 PST
Created attachment 116393 [details]
Patch
Comment 8 Ryosuke Niwa 2011-11-23 12:16:16 PST
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!
Comment 9 Rafael Weinstein 2011-11-23 12:29:19 PST
Created attachment 116397 [details]
Patch
Comment 10 WebKit Review Bot 2011-11-23 13:40:21 PST
Comment on attachment 116397 [details]
Patch

Clearing flags on attachment: 116397

Committed r101101: <http://trac.webkit.org/changeset/101101>
Comment 11 WebKit Review Bot 2011-11-23 13:40:26 PST
All reviewed patches have been landed.  Closing bug.