[Webkit-unassigned] [Bug 99295] removeAttribute('style') not working in certain circumstances

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=99295





--- Comment #9 from Takashi Sakamoto <tasak at google.com>  2012-10-22 00:38:01 PST ---
(From update of attachment 168872)
View in context: https://bugs.webkit.org/attachment.cgi?id=168872&action=review

Thank you for reviewing.

>> Source/WebCore/ChangeLog:10
>> +        given, reset the element's style by using removeBlockProperties.
> 
> This is not a good comment. It repeats what the code does in English, but doesn’t answer the question why this is the right thing to do.

I updated the comment.

>> Source/WebCore/ChangeLog:18
>> +        didRemoveAttribute to notify that the element's style is updated.
> 
> And this is just that same text again. The change log needs to say why this is being done, not just restate what’s in the code.

I updated the comment too.

>>> Source/WebCore/dom/Element.cpp:1578
>>> +        }
>> 
>> All the other code to special case the style attribute is in the StyledElement class. It’s not appropriate to put this mysterious code here in the Element class; the code to handle removal belongs in the StyledElement class with the rest of the code.
>> 
>> I suggest investigating whether this can be fixed in the StyledElement::attributeChanged function instead of here. I’m pretty sure it can be.
>> 
>> It’s incorrect for performance reasons to call style(), which will force a CSSStyleDeclaration to be created. The work should be done directly on the StylePropertySet. The function inlineStyle() should be called, and checked for null. Then if the style is non-empty, we should remove all the properties and call inlineStyleChanged.
>> 
>> It’s incorrect to call removeBlockProperties. We need to remove all the properties, not just the block properties. We may need to add a function to StylePropertySet that removes all properties.
> 
> I do think we will need to add some code to Element::removeAttribute, but it will be code something like this:
> 
>     if (UNLIKELY(name == styleAttr) && !isStyleAttributeValid())
>         updateStyleAttribute();

I agree that my first patch is very hacky. So I added clear() method to StylePropertySet and StyledElement to remove all inline style properties.
Talking about StyledElement::attributeChanged(), the method will be invoked from Element::didRemoveAttribute. The original code will quickly exit the method if index == notFound. didRemoveAttribute has been never invoked.
We need to modify Element::removeAttribute.

Talking about updateStyleAttribute(), it doesn't work well in this case. Because, 
(1) StyledElement::updateAttributeStyle() invokes Element::setSynchronizedLazyAttribute,
(2) Element::setSynchronizedLazyAttribute invokes Element::setAttributeInternal,
(3) Element::setAttributeInternal does:

    Attribute* existingAttribute = index != notFound ? attributeData->attributeItem(index) : 0;
    if (newValue.isNull()) {
        if (existingAttribute)
            removeAttributeInternal(index, inSynchronizationOfLazyAttribute);
        return;
    }

Now index == notFound and newValue is null, so just return. No inline styles will be removed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.