Bug 43639 – Undo fails in RemoveCSSPropertyCommand when the corresponding style attribute is removed

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Created attachment 63755 [details]
demonstrates the bug

Currently, the only usage of RemoveCSSPropertyCommand to trim some CSS property from a given HTML element.  However, because it only modifies CSSMutableStyleDeclaration and not the attribute of the styled element, RemoveCSSPropertyCommand cannot undo the removal if the style attribute of the corresponding element gets removed afterwards.  This happens when a CSS style is removed and style attribute becomes empty.

To fix this problem, we should re-implement removeCSSProperty using setNodeAttribute.

[Ryosuke Niwa]

Created attachment 63763 [details]
fixes the bug

[Ryosuke Niwa]

The patch is large because I had to delete RemoveCSSPropertyCommand.h/.cpp from all build files but the code change itself is trivial and must be easy to review.

[Ryosuke Niwa]

Comment on attachment 63763 [details]
fixes the bug

Need to add one more test.

[Ryosuke Niwa]

Created attachment 63769 [details]
Added more tests

[Justin Garcia]

> Currently, the only usage of RemoveCSSPropertyCommand to trim some CSS property from a given HTML element.  However, because it only modifies CSSMutableStyleDeclaration and not the attribute of the styled element, RemoveCSSPropertyCommand cannot undo the removal if the style attribute of the corresponding element gets removed afterwards.  This happens when a CSS style is removed and style attribute becomes empty.

The new approach has a higher memory overhead, since we hold on to the entire style string just to remove one property.  Can we just give RemoveCSSPropertyCommand the ability to remove the style attribute if removing the property empties the inline style declaration?

And I can't see where the new implementation removes the style attribute when necessary like you say:

-void CompositeEditCommand::removeCSSProperty(PassRefPtr<CSSMutableStyleDeclaration> style, CSSPropertyID property)
+void CompositeEditCommand::removeCSSProperty(PassRefPtr<StyledElement> element, CSSPropertyID property)
 {
-    applyCommandToComposite(RemoveCSSPropertyCommand::create(document(), style, property));
+    RefPtr<CSSMutableStyleDeclaration> style = element->inlineStyleDecl();
+    if (!style->getPropertyCSSValue(property))
+        return;
+    style = style->copy();
+    style->removeProperty(property);
+    setNodeAttribute(element, styleAttr, style->cssText());
 }

[Justin Garcia]

> And I can't see where the new implementation removes the style attribute when necessary like you say:

Oops, looks like Element::setAttribute does it.

[Ryosuke Niwa]

(In reply to comment #5)
> The new approach has a higher memory overhead, since we hold on to the entire style string just to remove one property.  Can we just give RemoveCSSPropertyCommand the ability to remove the style attribute if removing the property empties the inline style declaration?

Right, it's a higher overhead but I don't think removing the style attribute within RemoveCSSPropertyCommand solves the problem because there are other places (applyRelativeFontStyleChange, applyInlineStyleToRange, & applyTextDecorationStyle in ApplyStyleCommand for example) where we generate style attribute as a string and use setNodeAttribute to update, resulting in the loss of CSSMutableStyleDeclaration.

We could remember the property we removed though.

> And I can't see where the new implementation removes the style attribute when necessary like you say:

My point isn't really to remove the attribute inside of removeCSSProperty.  removeCSSProperty is used in ApplyStyleCommand to remove text decoration and such and we manually remove the attribute whenever style attribute is empty (see applyRelativeFontStyleChange, removeEmbeddingUpToEnclosingBlock, removeHTMLBidiEmbeddingStyle, removeCSSStyle for instance).

[Ryosuke Niwa]

Created attachment 63942 [details]
modified RemoveCSSPropertyCommand to restore styled element instead of style

[Justin Garcia]

Comment on attachment 63942 [details]
modified RemoveCSSPropertyCommand to restore styled element instead of style

r=me

[Ryosuke Niwa]

(In reply to comment #9)
> (From update of attachment 63942 [details])
> r=me

Thanks for the promptly review!

[Ryosuke Niwa]

Committed r65014: <http://trac.webkit.org/changeset/65014>