Bug 99295 - removeAttribute('style') not working in certain circumstances
Summary: removeAttribute('style') not working in certain circumstances
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Takashi Sakamoto
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-14 23:30 PDT by GU Yiling
Modified: 2012-11-06 03:33 PST (History)
12 users (show)

See Also:


Attachments
Test cases for removeAttribute('style') (1.73 KB, text/html)
2012-10-14 23:30 PDT, GU Yiling
no flags Details
WIP (6.78 KB, patch)
2012-10-15 22:06 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2012-10-15 23:49 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (10.33 KB, patch)
2012-10-22 00:08 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (10.37 KB, patch)
2012-10-22 00:20 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (11.99 KB, patch)
2012-11-04 23:04 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch for landing (11.79 KB, patch)
2012-11-06 01:22 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GU Yiling 2012-10-14 23:30:25 PDT
Created attachment 168630 [details]
Test cases for removeAttribute('style')

Hi all.

It seems that the creation of 'style' content attribute for an HTML element is delayed until getAttribute('style') or setAttribute('style', '...') is called for the first time *and* HTML style attribute exists. If style is modified through IDL attributes before that, the style cannot be removed successfully by calling removeAttribute('style').
Comment 1 GU Yiling 2012-10-14 23:39:23 PDT
Bug 28176 (https://bugs.webkit.org/show_bug.cgi?id=28176), it seems, is caused by the same problem.
Comment 2 Kang-Hao (Kenny) Lu 2012-10-15 00:04:07 PDT
(In reply to comment #0)
> Created an attachment (id=168630) [details]
> Test cases for removeAttribute('style')

Just FYI, the spec says you should not see any red in any cell. See also this relevant jQuery ticket: http://bugs.jquery.com/ticket/9699 .
Comment 3 Takashi Sakamoto 2012-10-15 22:06:32 PDT
Created attachment 168852 [details]
WIP
Comment 4 Takashi Sakamoto 2012-10-15 23:49:21 PDT
Created attachment 168872 [details]
Patch
Comment 5 Darin Adler 2012-10-18 20:52:28 PDT
Comment on attachment 168872 [details]
Patch

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.
Comment 6 Darin Adler 2012-10-18 20:54:23 PDT
Comment on attachment 168872 [details]
Patch

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

>> 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();
Comment 7 Takashi Sakamoto 2012-10-22 00:08:43 PDT
Created attachment 169835 [details]
Patch
Comment 8 Takashi Sakamoto 2012-10-22 00:20:22 PDT
Created attachment 169836 [details]
Patch
Comment 9 Takashi Sakamoto 2012-10-22 00:38:01 PDT
Comment on attachment 168872 [details]
Patch

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.
Comment 10 Ryosuke Niwa 2012-11-03 18:24:22 PDT
Comment on attachment 169836 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        style should be always removed by using "removeAttribute('style')".

Nit: s/removed by using/removable by/

> Source/WebCore/dom/StyledElement.cpp:224
> +void StyledElement::clearInlineStyleProperty()

Should be clearInlineStyleProperties or removeAllInlineStyleProperties.

> Source/WebCore/dom/StyledElement.cpp:228
> +    StylePropertySet* inlineStylePropertySet = ensureInlineStyle();

Why do you want to "ensure" it? We would have exited early if inline style was null.

> LayoutTests/fast/css/remove-attribute-style-expected.txt:6
> +PASS window.getComputedStyle(c11).backgroundColor is "rgba(0, 0, 0, 0)"

This output isn't great. I can't tell which line is testing what with cryptic names like c11.

> LayoutTests/fast/css/remove-attribute-style.html:19
> +      <td id="c11">no HTML style attribute, no get/setAttribute</td>

It would have been better if the id was elementWithoutStyleAttribute.

> LayoutTests/fast/css/remove-attribute-style.html:41
> +    document.body.offsetLeft;

Do we really need to trigger layout?

> LayoutTests/fast/css/remove-attribute-style.html:43
> +    shouldBe("window.getComputedStyle(c11).backgroundColor", '"rgba(0, 0, 0, 0)"');

You can omit "window."

> LayoutTests/fast/css/remove-attribute-style.html:62
> +    c22.getAttribute('style');

It would have been better if this line was included in shouldBe so that we can see what you're doing in the expected result.
Comment 11 Takashi Sakamoto 2012-11-04 23:04:58 PST
Created attachment 172273 [details]
Patch
Comment 12 Takashi Sakamoto 2012-11-04 23:09:48 PST
Comment on attachment 169836 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/ChangeLog:9
>> +        style should be always removed by using "removeAttribute('style')".
> 
> Nit: s/removed by using/removable by/

Thanks. Done.

>> Source/WebCore/dom/StyledElement.cpp:224
>> +void StyledElement::clearInlineStyleProperty()
> 
> Should be clearInlineStyleProperties or removeAllInlineStyleProperties.

Done.

>> Source/WebCore/dom/StyledElement.cpp:228
>> +    StylePropertySet* inlineStylePropertySet = ensureInlineStyle();
> 
> Why do you want to "ensure" it? We would have exited early if inline style was null.

I think, the case is covered by the above "!attributeData()->inlineStyle()".
Looking at the other related codes, i.e. removeInlineStyleProperty and so on, ensureInlineStyle() is used to obtain mutable StylePropertySet from mutableAttributeData.

>> LayoutTests/fast/css/remove-attribute-style-expected.txt:6
>> +PASS window.getComputedStyle(c11).backgroundColor is "rgba(0, 0, 0, 0)"
> 
> This output isn't great. I can't tell which line is testing what with cryptic names like c11.

I see. Done.

>> LayoutTests/fast/css/remove-attribute-style.html:19
>> +      <td id="c11">no HTML style attribute, no get/setAttribute</td>
> 
> It would have been better if the id was elementWithoutStyleAttribute.

Done.

>> LayoutTests/fast/css/remove-attribute-style.html:41
>> +    document.body.offsetLeft;
> 
> Do we really need to trigger layout?

I'm afraid that reattach() in Element::recalcStyle updates these styles. However, to reproduce this issue, we don't need this trigger. I removed all "document.body.offsetLeft;".

>> LayoutTests/fast/css/remove-attribute-style.html:43
>> +    shouldBe("window.getComputedStyle(c11).backgroundColor", '"rgba(0, 0, 0, 0)"');
> 
> You can omit "window."

Done.

>> LayoutTests/fast/css/remove-attribute-style.html:62
>> +    c22.getAttribute('style');
> 
> It would have been better if this line was included in shouldBe so that we can see what you're doing in the expected result.

Done.
Comment 13 Ryosuke Niwa 2012-11-04 23:46:55 PST
Comment on attachment 172273 [details]
Patch

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

> Source/WebCore/dom/StyledElement.cpp:226
> +void StyledElement::clearInlineStyleProperties()

On my second thought, clearInlineStyleProperties sounds as if resetting values of properties. So maybe we should call it removeAllInlineStyleProperties.

> LayoutTests/fast/css/remove-attribute-style.html:55
> +    shouldBeNull("elementWithoutStyleAttributeWithGetAttrBeforeStyleUpdate.getAttribute('style')");
> +    elementWithoutStyleAttributeWithGetAttrBeforeStyleUpdate.style.backgroundColor = 'red';
> +    elementWithoutStyleAttributeWithGetAttrBeforeStyleUpdate.removeAttribute('style');
> +    shouldBe("getComputedStyle(elementWithoutStyleAttributeWithGetAttrBeforeStyleUpdate).backgroundColor", '"rgba(0, 0, 0, 0)"');

You can do something like:
shouldBeNull("elementWithoutStyleAttribute2.getAttribute('style')");
shouldBe("elementWithoutStyleAttribute2.style.backgroundColor = 'red'; elementWithoutStyleAttribute2.removeAttribute('style'); elementWithoutStyleAttribute2.backgroundColor", "rgba(0, 0, 0, 0)");
so that the actual operations done before accessing backgroundColor is visible in the results instead of giving a really long variable name.

> LayoutTests/fast/css/remove-attribute-style.html:65
> +    elementWithoutStyleAttributeWithSetAttr.style.backgroundColor = 'red';
> +    elementWithoutStyleAttributeWithSetAttr.setAttribute('style', '');
> +    elementWithoutStyleAttributeWithSetAttr.removeAttribute('style');
> +    shouldBe("getComputedStyle(elementWithoutStyleAttributeWithSetAttr).backgroundColor", '"rgba(0, 0, 0, 0)"');

Similarly here, you can do:
shouldBe("elementWithoutStyleAttribute3.style.backgroundColor = 'red'; elementWithoutStyleAttribute3.setAttribute('style', ''); elementWithoutStyleAttribute3.removeAttribute('style); getComputedStyle(elementWithoutStyleAttribute3).backgroundColor", '"rgba(0, 0, 0, 0)"');
Comment 14 Takashi Sakamoto 2012-11-06 01:20:33 PST
Comment on attachment 172273 [details]
Patch

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

>> Source/WebCore/dom/StyledElement.cpp:226
>> +void StyledElement::clearInlineStyleProperties()
> 
> On my second thought, clearInlineStyleProperties sounds as if resetting values of properties. So maybe we should call it removeAllInlineStyleProperties.

I see. Done.

>> LayoutTests/fast/css/remove-attribute-style.html:55
>> +    shouldBe("getComputedStyle(elementWithoutStyleAttributeWithGetAttrBeforeStyleUpdate).backgroundColor", '"rgba(0, 0, 0, 0)"');
> 
> You can do something like:
> shouldBeNull("elementWithoutStyleAttribute2.getAttribute('style')");
> shouldBe("elementWithoutStyleAttribute2.style.backgroundColor = 'red'; elementWithoutStyleAttribute2.removeAttribute('style'); elementWithoutStyleAttribute2.backgroundColor", "rgba(0, 0, 0, 0)");
> so that the actual operations done before accessing backgroundColor is visible in the results instead of giving a really long variable name.

I see. Done.

>> LayoutTests/fast/css/remove-attribute-style.html:65
>> +    shouldBe("getComputedStyle(elementWithoutStyleAttributeWithSetAttr).backgroundColor", '"rgba(0, 0, 0, 0)"');
> 
> Similarly here, you can do:
> shouldBe("elementWithoutStyleAttribute3.style.backgroundColor = 'red'; elementWithoutStyleAttribute3.setAttribute('style', ''); elementWithoutStyleAttribute3.removeAttribute('style); getComputedStyle(elementWithoutStyleAttribute3).backgroundColor", '"rgba(0, 0, 0, 0)"');

Done.
Comment 15 Takashi Sakamoto 2012-11-06 01:22:38 PST
Created attachment 172513 [details]
Patch for landing
Comment 16 WebKit Review Bot 2012-11-06 03:33:34 PST
Comment on attachment 172513 [details]
Patch for landing

Clearing flags on attachment: 172513

Committed r133581: <http://trac.webkit.org/changeset/133581>
Comment 17 WebKit Review Bot 2012-11-06 03:33:38 PST
All reviewed patches have been landed.  Closing bug.