Summary: | Undo fails in RemoveCSSPropertyCommand when the corresponding style attribute is removed | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | darin, eric, jparent, justin.garcia, ojan, tony | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 26871 | ||||||||||||
Attachments: |
|
Created attachment 63763 [details]
fixes the bug
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. Comment on attachment 63763 [details]
fixes the bug
Need to add one more test.
Created attachment 63769 [details]
Added more tests
> 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());
}
> 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.
(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). Created attachment 63942 [details]
modified RemoveCSSPropertyCommand to restore styled element instead of style
Comment on attachment 63942 [details]
modified RemoveCSSPropertyCommand to restore styled element instead of style
r=me
(In reply to comment #9) > (From update of attachment 63942 [details]) > r=me Thanks for the promptly review! Committed r65014: <http://trac.webkit.org/changeset/65014> |
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.