Bug 99295

Bug 28176 (https://bugs.webkit.org/show_bug.cgi?id=28176), it seems, is caused by the same problem.

(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 .

Created attachment 168852 [details] WIP

Created attachment 168872 [details] Patch

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 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();

Created attachment 169835 [details] Patch

Created attachment 169836 [details] Patch

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 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.

Created attachment 172273 [details] Patch

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 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 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.

Created attachment 172513 [details] Patch for landing

Comment on attachment 172513 [details] Patch for landing Clearing flags on attachment: 172513 Committed r133581: <http://trac.webkit.org/changeset/133581>

All reviewed patches have been landed. Closing bug.