[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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #168872|review?                     |review-
               Flag|                            |




--- Comment #5 from Darin Adler <darin at apple.com>  2012-10-18 20:52:28 PST ---
(From update of attachment 168872)
View in context: https://bugs.webkit.org/attachment.cgi?id=168872&action=review

> Source/WebCore/ChangeLog:10
> +        When Element::removeAttribute is invoked, check whether the given name
> +        is 'style' or not. If the element has no style attribute and style is
> +        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.

> Source/WebCore/ChangeLog:18
> +        If 'style' is given but the element has no style attribute, invoke
> +        removeBlockProperties to reset the style. After the reset, invoke
> +        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.

> Source/WebCore/dom/Element.cpp:1578
> +        if (localName == styleAttr && style() && style()->length() > 0) {
> +            style()->makeMutable()->removeBlockProperties();
> +            didRemoveAttribute(QualifiedName(nullAtom, localName, nullAtom));
> +        }

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.

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