Changeset 101101 in webkit


Ignore:
Timestamp:
Nov 23, 2011 1:40:06 PM (12 years ago)
Author:
commit-queue@webkit.org
Message:

[MutationObservers] Modifications to the style property don't dispatch "attributes" Mutation Records
​https://bugs.webkit.org/show_bug.cgi?id=70137

Patch by Rafael Weinstein <​rafaelw@chromium.org> on 2011-11-23
Reviewed by Ryosuke Niwa.

Source/WebCore:

This patch adds a private AttributesMutationScope mechanism to CSSMutableStyleDeclaration (which uses
the RAII pattern similar to the public ChildListMutationScope). This manages the (sometimes conditional)
pre-change serialization of the style attribute (if an observer has requested oldValue), creation of
the mutation record, and dispatch if the declaration was actual affected.

  • css/CSSMutableStyleDeclaration.cpp:

(WebCore::CSSMutableStyleDeclaration::removeProperty):
(WebCore::CSSMutableStyleDeclaration::setProperty):
(WebCore::CSSMutableStyleDeclaration::setPropertyInternal):
(WebCore::CSSMutableStyleDeclaration::parseDeclaration):
(WebCore::CSSMutableStyleDeclaration::addParsedProperties):
(WebCore::CSSMutableStyleDeclaration::addParsedProperty):
(WebCore::CSSMutableStyleDeclaration::setCssText):
(WebCore::CSSMutableStyleDeclaration::merge):
(WebCore::CSSMutableStyleDeclaration::removePropertiesInSet):

  • dom/Element.cpp:

(WebCore::Element::setAttribute):

LayoutTests:

Added tests asserting that changes to the style property are observable and work correctly if oldValue is requested.

  • fast/mutation/observe-attributes-expected.txt:
  • fast/mutation/observe-attributes.html:
Location:
trunk
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r101098 r101101  
     12011-11-23  Rafael Weinstein  <rafaelw@chromium.org>
     2
     3        [MutationObservers] Modifications to the style property don't dispatch "attributes" Mutation Records
     4        https://bugs.webkit.org/show_bug.cgi?id=70137
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        Added tests asserting that changes to the style property are observable and work correctly if oldValue is requested.
     9
     10        * fast/mutation/observe-attributes-expected.txt:
     11        * fast/mutation/observe-attributes.html:
     12
    1132011-11-23  Adam Klein  <adamk@chromium.org>
    214

  • trunk/LayoutTests/fast/mutation/observe-attributes-expected.txt

    r99992 r101101  
    153153PASS mutations[3].attributeNamespace is "http://www.w3.org/2000/svg"
    154154
     155Testing that modifying an elements style property dispatches Mutation Records.
     156PASS mutations.length is 3
     157PASS mutations[0].type is "attributes"
     158PASS mutations[0].attributeName is "style"
     159PASS mutations[0].oldValue is null
     160PASS mutations[1].type is "attributes"
     161PASS mutations[1].attributeName is "style"
     162PASS mutations[1].oldValue is null
     163PASS mutations[2].type is "attributes"
     164PASS mutations[2].attributeName is "style"
     165PASS mutations[2].oldValue is null
     166...mutation record created.
     167PASS mutations is null
     168
     169Testing that modifying an elements style property dispatches Mutation Records with correct oldValues.
     170PASS mutations.length is 3
     171PASS mutations[0].type is "attributes"
     172PASS mutations[0].attributeName is "style"
     173PASS mutations[0].oldValue is "color: yellow; width: 100px; "
     174PASS mutations[1].type is "attributes"
     175PASS mutations[1].attributeName is "style"
     176PASS mutations[1].oldValue is "width: 100px; color: red; "
     177PASS mutations[2].type is "attributes"
     178PASS mutations[2].attributeName is "style"
     179PASS mutations[2].oldValue is "color: red; width: 200px; "
     180...mutation record created.
     181PASS mutations is null
     182
    155183PASS successfullyParsed is true
    156184

  • trunk/LayoutTests/fast/mutation/observe-attributes.html

    r99992 r101101  
    630630        shouldBe('mutations[3].attributeName', '"pathLength"');
    631631        shouldBe('mutations[3].attributeNamespace', '"http://www.w3.org/2000/svg"');
     632
     633        observer.disconnect();
     634        debug('');
     635        runNextTest();
     636    }
     637
     638    start();
     639}
     640
     641function testStyleAttributePropertyAccess() {
     642    var svgDoc, div, path;
     643    var observer;
     644
     645    function start() {
     646        debug('Testing that modifying an elements style property dispatches Mutation Records.');
     647
     648        mutations = null;
     649        observer = new WebKitMutationObserver(function(m) {
     650            mutations = m;
     651        });
     652
     653        div = document.createElement('div');
     654        div.setAttribute('style', 'color: yellow; width: 100px; ');
     655        observer.observe(div, { attributes: true });
     656        div.style.color = 'red';
     657        div.style.width = '200px';
     658        div.style.color = 'blue';
     659
     660        setTimeout(checkAndContinue, 0);
     661    }
     662
     663    function checkAndContinue() {
     664        shouldBe('mutations.length', '3');
     665        shouldBe('mutations[0].type', '"attributes"');
     666        shouldBe('mutations[0].attributeName', '"style"');
     667        shouldBe('mutations[0].oldValue', 'null');
     668        shouldBe('mutations[1].type', '"attributes"');
     669        shouldBe('mutations[1].attributeName', '"style"');
     670        shouldBe('mutations[1].oldValue', 'null');
     671        shouldBe('mutations[2].type', '"attributes"');
     672        shouldBe('mutations[2].attributeName', '"style"');
     673        shouldBe('mutations[2].oldValue', 'null');
     674
     675        mutations = null;
     676        div.getAttribute('style');
     677        setTimeout(finish, 0);
     678    }
     679
     680    function finish() {
     681        debug('...mutation record created.');
     682
     683        shouldBe('mutations', 'null');
     684
     685        observer.disconnect();
     686        debug('');
     687        runNextTest();
     688    }
     689
     690    start();
     691}
     692
     693function testStyleAttributePropertyAccessOldValue() {
     694    var svgDoc, div, path;
     695    var observer;
     696
     697    function start() {
     698        debug('Testing that modifying an elements style property dispatches Mutation Records with correct oldValues.');
     699
     700        mutations = null;
     701        observer = new WebKitMutationObserver(function(m) {
     702            mutations = m;
     703        });
     704
     705        div = document.createElement('div');
     706        div.setAttribute('style', 'color: yellow; width: 100px; ');
     707        observer.observe(div, { attributes: true, attributeOldValue: true });
     708        div.style.color = 'red';
     709        div.style.width = '200px';
     710        div.style.color = 'blue';
     711
     712        setTimeout(checkAndContinue, 0);
     713    }
     714
     715    function checkAndContinue() {
     716        shouldBe('mutations.length', '3');
     717        shouldBe('mutations[0].type', '"attributes"');
     718        shouldBe('mutations[0].attributeName', '"style"');
     719        shouldBe('mutations[0].oldValue', '"color: yellow; width: 100px; "');
     720        shouldBe('mutations[1].type', '"attributes"');
     721        shouldBe('mutations[1].attributeName', '"style"');
     722        shouldBe('mutations[1].oldValue', '"width: 100px; color: red; "');
     723        shouldBe('mutations[2].type', '"attributes"');
     724        shouldBe('mutations[2].attributeName', '"style"');
     725        shouldBe('mutations[2].oldValue', '"color: red; width: 200px; "');
     726
     727        mutations = null;
     728        div.getAttribute('style');
     729        setTimeout(finish, 0);
     730    }
     731
     732    function finish() {
     733        debug('...mutation record created.');
     734
     735        shouldBe('mutations', 'null');
    632736
    633737        observer.disconnect();
    … …  
    654758    testAttributeFilterSubtree,
    655759    testAttributeFilterNonHTMLElement,
    656     testAttributeFilterNonHTMLDocument
     760    testAttributeFilterNonHTMLDocument,
     761    testStyleAttributePropertyAccess,
     762    testStyleAttributePropertyAccessOldValue
    657763];
    658764var testIndex = 0;

  • trunk/Source/WebCore/ChangeLog

    r101099 r101101  
     12011-11-23  Rafael Weinstein  <rafaelw@chromium.org>
     2
     3        [MutationObservers] Modifications to the style property don't dispatch "attributes" Mutation Records
     4        https://bugs.webkit.org/show_bug.cgi?id=70137
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        This patch adds a private AttributesMutationScope mechanism to CSSMutableStyleDeclaration (which uses
     9        the RAII pattern similar to the public ChildListMutationScope). This manages the (sometimes conditional)
     10        pre-change serialization of the style attribute (if an observer has requested oldValue), creation of
     11        the mutation record, and dispatch if the declaration was actual affected.
     12
     13        * css/CSSMutableStyleDeclaration.cpp:
     14        (WebCore::CSSMutableStyleDeclaration::removeProperty):
     15        (WebCore::CSSMutableStyleDeclaration::setProperty):
     16        (WebCore::CSSMutableStyleDeclaration::setPropertyInternal):
     17        (WebCore::CSSMutableStyleDeclaration::parseDeclaration):
     18        (WebCore::CSSMutableStyleDeclaration::addParsedProperties):
     19        (WebCore::CSSMutableStyleDeclaration::addParsedProperty):
     20        (WebCore::CSSMutableStyleDeclaration::setCssText):
     21        (WebCore::CSSMutableStyleDeclaration::merge):
     22        (WebCore::CSSMutableStyleDeclaration::removePropertiesInSet):
     23        * dom/Element.cpp:
     24        (WebCore::Element::setAttribute):
     25
    1262011-11-23  Dmitry Lomov  <dslomov@google.com>
    227

  • trunk/Source/WebCore/css/CSSMutableStyleDeclaration.cpp

    r101078 r101101  
    3333#include "Document.h"
    3434#include "ExceptionCode.h"
     35#include "HTMLNames.h"
    3536#include "InspectorInstrumentation.h"
     37#include "MutationRecord.h"
    3638#include "StyledElement.h"
     39#include "WebKitMutationObserver.h"
    3740#include <wtf/text/StringBuilder.h>
    3841#include <wtf/text/WTFString.h>
    … …  
    4144
    4245namespace WebCore {
     46
     47#if ENABLE(MUTATION_OBSERVERS)
     48namespace {
     49
     50class StyleAttributeMutationScope {
     51    WTF_MAKE_NONCOPYABLE(StyleAttributeMutationScope);
     52public:
     53    StyleAttributeMutationScope(CSSMutableStyleDeclaration* decl)
     54    {
     55        ++s_scopeCount;
     56
     57        if (s_scopeCount != 1) {
     58            ASSERT(s_currentDecl == decl);
     59            return;
     60        }
     61
     62        ASSERT(!s_currentDecl);
     63        s_currentDecl = decl;
     64
     65        if (!s_currentDecl->isInlineStyleDeclaration())
     66            return;
     67
     68        s_mutationRecipients = MutationObserverInterestGroup::createForAttributesMutation(s_currentDecl->node(), HTMLNames::styleAttr);
     69        if (s_mutationRecipients->isEmpty()) {
     70            s_mutationRecipients.clear();
     71            return;
     72        }
     73
     74        Element* element = toElement(s_currentDecl->node());
     75        AtomicString oldValue = s_mutationRecipients->isOldValueRequested() ? element->getAttribute(HTMLNames::styleAttr) : nullAtom;
     76        s_mutation = MutationRecord::createAttributes(element, HTMLNames::styleAttr, oldValue);
     77    }
     78
     79    ~StyleAttributeMutationScope()
     80    {
     81        --s_scopeCount;
     82        if (!s_scopeCount)
     83            s_currentDecl = 0;
     84    }
     85
     86    void enqueueMutationRecord()
     87    {
     88        if (!s_mutation)
     89            return;
     90        s_mutationRecipients->enqueueMutationRecord(s_mutation);
     91        s_mutation.clear();
     92        s_mutationRecipients.clear();
     93    }
     94
     95private:
     96    static unsigned s_scopeCount;
     97    static OwnPtr<MutationObserverInterestGroup> s_mutationRecipients;
     98    static RefPtr<MutationRecord> s_mutation;
     99    static CSSMutableStyleDeclaration* s_currentDecl;
     100};
     101
     102unsigned StyleAttributeMutationScope::s_scopeCount = 0;
     103OwnPtr<MutationObserverInterestGroup> StyleAttributeMutationScope::s_mutationRecipients;
     104RefPtr<MutationRecord> StyleAttributeMutationScope::s_mutation;
     105CSSMutableStyleDeclaration* StyleAttributeMutationScope::s_currentDecl = 0;
     106
     107} // namespace
     108#endif // ENABLE(MUTATION_OBSERVERS)
    43109
    44110CSSMutableStyleDeclaration::CSSMutableStyleDeclaration()
    … …  
    521587    ASSERT(!m_iteratorCount);
    522588
     589#if ENABLE(MUTATION_OBSERVERS)
     590    StyleAttributeMutationScope mutationScope(this);
     591#endif
     592
    523593    if (removeShorthandProperty(propertyID, notifyChanged)) {
    524594        // FIXME: Return an equivalent shorthand when possible.
    … …  
    535605    // and sweeping them when the vector grows too big.
    536606    m_properties.remove(foundProperty - m_properties.data());
     607
     608#if ENABLE(MUTATION_OBSERVERS)
     609    mutationScope.enqueueMutationRecord();
     610#endif
    537611
    538612    if (notifyChanged)
    … …  
    603677    ASSERT(!m_iteratorCount);
    604678
     679#if ENABLE(MUTATION_OBSERVERS)
     680    StyleAttributeMutationScope mutationScope(this);
     681#endif
     682
    605683    // Setting the value to an empty string just removes the property in both IE and Gecko.
    606684    // Setting it to null seems to produce less consistent results, but we treat it just the same.
    … …  
    616694        // CSS DOM requires raising SYNTAX_ERR here, but this is too dangerous for compatibility,
    617695        // see <http://bugs.webkit.org/show_bug.cgi?id=7296>.
    618     } else if (notifyChanged)
     696        return false;
     697    }
     698
     699#if ENABLE(MUTATION_OBSERVERS)
     700    mutationScope.enqueueMutationRecord();
     701#endif
     702
     703    if (notifyChanged)
    619704        setNeedsStyleRecalc();
    620705
    621     return success;
     706    return true;
    622707}
    623708
    … …  
    625710{
    626711    ASSERT(!m_iteratorCount);
     712
     713#if ENABLE(MUTATION_OBSERVERS)
     714    StyleAttributeMutationScope mutationScope(this);
     715#endif
    627716
    628717    if (!removeShorthandProperty(property.id(), false)) {
    … …  
    634723    }
    635724    m_properties.append(property);
     725
     726#if ENABLE(MUTATION_OBSERVERS)
     727    mutationScope.enqueueMutationRecord();
     728#endif
    636729}
    637730
    … …  
    674767    ASSERT(!m_iteratorCount);
    675768
     769#if ENABLE(MUTATION_OBSERVERS)
     770    StyleAttributeMutationScope mutationScope(this);
     771#endif
     772
    676773    m_properties.clear();
    677774    CSSParser parser(useStrictParsing());
    678775    parser.parseDeclaration(this, styleDeclaration);
     776
     777#if ENABLE(MUTATION_OBSERVERS)
     778    mutationScope.enqueueMutationRecord();
     779#endif
     780
    679781    setNeedsStyleRecalc();
    680782}
    … …  
    683785{
    684786    ASSERT(!m_iteratorCount);
     787
     788#if ENABLE(MUTATION_OBSERVERS)
     789    StyleAttributeMutationScope mutationScope(this);
     790#endif
    685791
    686792    m_properties.reserveCapacity(numProperties);
    … …  
    688794        addParsedProperty(*properties[i]);
    689795
     796#if ENABLE(MUTATION_OBSERVERS)
     797    mutationScope.enqueueMutationRecord();
     798#endif
     799
    690800    // FIXME: This probably should have a call to setNeedsStyleRecalc() if something changed. We may also wish to add
    691801    // a notifyChanged argument to this function to follow the model of other functions in this class.
    … …  
    695805{
    696806    ASSERT(!m_iteratorCount);
     807
     808#if ENABLE(MUTATION_OBSERVERS)
     809    StyleAttributeMutationScope mutationScope(this);
     810#endif
    697811
    698812    // Only add properties that have no !important counterpart present
    … …  
    701815        m_properties.append(property);
    702816    }
     817
     818#if ENABLE(MUTATION_OBSERVERS)
     819    mutationScope.enqueueMutationRecord();
     820#endif
    703821}
    704822
    … …  
    791909    ASSERT(!m_iteratorCount);
    792910
     911#if ENABLE(MUTATION_OBSERVERS)
     912    StyleAttributeMutationScope mutationScope(this);
     913#endif
     914
    793915    ec = 0;
    794916    m_properties.clear();
    795917    CSSParser parser(useStrictParsing());
    796918    parser.parseDeclaration(this, text);
     919
     920#if ENABLE(MUTATION_OBSERVERS)
     921    mutationScope.enqueueMutationRecord();
     922#endif
     923
    797924    // FIXME: Detect syntax errors and set ec.
    798925    setNeedsStyleRecalc();
    … …  
    802929{
    803930    ASSERT(!m_iteratorCount);
     931
     932#if ENABLE(MUTATION_OBSERVERS)
     933    StyleAttributeMutationScope mutationScope(this);
     934#endif
    804935
    805936    unsigned size = other->m_properties.size();
    … …  
    814945            m_properties.append(toMerge);
    815946    }
     947
     948#if ENABLE(MUTATION_OBSERVERS)
     949    mutationScope.enqueueMutationRecord();
     950#endif
     951
    816952    // FIXME: This probably should have a call to setNeedsStyleRecalc() if something changed. We may also wish to add
    817953    // a notifyChanged argument to this function to follow the model of other functions in this class.
    … …  
    8711007        return;
    8721008
     1009#if ENABLE(MUTATION_OBSERVERS)
     1010    StyleAttributeMutationScope mutationScope(this);
     1011#endif
     1012
    8731013    // FIXME: This is always used with static sets and in that case constructing the hash repeatedly is pretty pointless.
    8741014    HashSet<int> toRemove;
    … …  
    8931033    m_properties = newProperties;
    8941034
    895     if (changed && notifyChanged)
    896         setNeedsStyleRecalc();
     1035    if (!changed || !notifyChanged)
     1036        return;
     1037
     1038#if ENABLE(MUTATION_OBSERVERS)
     1039    mutationScope.enqueueMutationRecord();
     1040#endif
     1041
     1042    setNeedsStyleRecalc();
    8971043}
    8981044

  • trunk/Source/WebCore/dom/Element.cpp

    r101061 r101101  
    620620static void enqueueAttributesMutationRecord(Element* target, const QualifiedName& attributeName, const AtomicString& oldValue)
    621621{
    622     OwnPtr<MutationObserverInterestGroup> mutationRecipients = MutationObserverInterestGroup::createForAttributesMutation(target, attributeName.localName());
     622    OwnPtr<MutationObserverInterestGroup> mutationRecipients = MutationObserverInterestGroup::createForAttributesMutation(target, attributeName);
    623623    mutationRecipients->enqueueMutationRecord(MutationRecord::createAttributes(target, attributeName, oldValue));
    624624}
    … …  
    647647#if ENABLE(MUTATION_OBSERVERS)
    648648    // The call to attributeChanged below may dispatch DOMSubtreeModified, so it's important to enqueue a MutationRecord now.
    649     enqueueAttributesMutationRecord(this, attributeName, old ? old->value() : nullAtom);
     649    if (!isSynchronizingStyleAttribute())
     650        enqueueAttributesMutationRecord(this, attributeName, old ? old->value() : nullAtom);
    650651#endif
    651652
    … …  
    685686#if ENABLE(MUTATION_OBSERVERS)
    686687    // The call to attributeChanged below may dispatch DOMSubtreeModified, so it's important to enqueue a MutationRecord now.
    687     enqueueAttributesMutationRecord(this, name, old ? old->value() : nullAtom);
     688    if (!isSynchronizingStyleAttribute())
     689        enqueueAttributesMutationRecord(this, name, old ? old->value() : nullAtom);
    688690#endif
    689691

  • trunk/Source/WebCore/dom/WebKitMutationObserver.cpp

    r101061 r101101  
    4141#include "MutationRecord.h"
    4242#include "Node.h"
     43#include "QualifiedName.h"
    4344#include <wtf/ListHashSet.h>
    4445
    … …  
    152153}
    153154
    154 PassOwnPtr<MutationObserverInterestGroup> MutationObserverInterestGroup::createForAttributesMutation(Node* target, const AtomicString& attributeName)
    155 {
    156     return adoptPtr(new MutationObserverInterestGroup(target, WebKitMutationObserver::Attributes, attributeName));
     155PassOwnPtr<MutationObserverInterestGroup> MutationObserverInterestGroup::createForAttributesMutation(Node* target, const QualifiedName& attributeName)
     156{
     157    return adoptPtr(new MutationObserverInterestGroup(target, WebKitMutationObserver::Attributes, attributeName.localName()));
    157158}
    158159

  • trunk/Source/WebCore/dom/WebKitMutationObserver.h

    r101061 r101101  
    4848class MutationRecord;
    4949class Node;
     50class QualifiedName;
    5051
    5152typedef int ExceptionCode;
    … …  
    100101    static PassOwnPtr<MutationObserverInterestGroup> createForChildListMutation(Node* target);
    101102    static PassOwnPtr<MutationObserverInterestGroup> createForCharacterDataMutation(Node* target);
    102     static PassOwnPtr<MutationObserverInterestGroup> createForAttributesMutation(Node* target, const AtomicString& attributeName);
     103    static PassOwnPtr<MutationObserverInterestGroup> createForAttributesMutation(Node* target, const QualifiedName& attributeName);
    103104
    104105    bool isOldValueRequested();
Note: See TracChangeset for help on using the changeset viewer.