Bug 101682 – RemoveFormat command doesn't remove background color

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

[Ryosuke Niwa]

Steps to reproduce the problem:
1. create js node 'some <span style="color: black; background-color: #ff0000;">red text</span> here' element 
2. call execCommand with removeFormat on the created node

What is the expected behavior?
'some <span>red text</span> here'
ie: Background colour is removed

What went wrong?
'some <span style="background-color: #ff0000;">red text</span> here'
ie: Background colour is not removed

http://code.google.com/p/chromium/issues/detail?id=159407

[Radar WebKit Bug Importer]

<rdar://problem/12668350>

[Ryosuke Niwa]

Created attachment 173196 [details]
Fixes the bug

[Tony Chang]

Comment on attachment 173196 [details]
Fixes the bug

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

> LayoutTests/editing/execCommand/remove-format-background-color-expected.txt:3
> +This tests removing format on text that has background color. There should be no span or inline style below:
> +
> +hello world WebKit.

Should we use something other than a dumpAsText test?  I think a script test that uses getComputedStyle or a reftest would be better for checking the background color.

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 173196 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173196&action=review
> 
> > LayoutTests/editing/execCommand/remove-format-background-color-expected.txt:3
> > +This tests removing format on text that has background color. There should be no span or inline style below:
> > +
> > +hello world WebKit.
> 
> Should we use something other than a dumpAsText test?  I think a script test that uses getComputedStyle or a reftest would be better for checking the background color.

I’m dumping HTML here. We don’t want to leave a span. It’s important the markup we generate is as concise as possible.

[Tony Chang]

Comment on attachment 173196 [details]
Fixes the bug

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

> Source/WebCore/editing/RemoveFormatCommand.cpp:92
> +    defaultStyle->style()->setProperty(CSSPropertyBackgroundColor, "transparent");

Can we pass in CSSValueTransparent instead of "transparent"?  Looks like that would avoid having to run the CSS parser.

>>> LayoutTests/editing/execCommand/remove-format-background-color-expected.txt:3
>>> +hello world WebKit.
>> 
>> Should we use something other than a dumpAsText test?  I think a script test that uses getComputedStyle or a reftest would be better for checking the background color.
> 
> I’m dumping HTML here. We don’t want to leave a span. It’s important the markup we generate is as concise as possible.

Ah, I see.  Using dump-as-markup.js would be more clear.  Should there be a test case with background-color: transparent?

[Ryosuke Niwa]

Comment on attachment 173196 [details]
Fixes the bug

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

>> Source/WebCore/editing/RemoveFormatCommand.cpp:92
>> +    defaultStyle->style()->setProperty(CSSPropertyBackgroundColor, "transparent");
> 
> Can we pass in CSSValueTransparent instead of "transparent"?  Looks like that would avoid having to run the CSS parser.

Oh, you’re right. We’re talking to StylePropertySet here.

>>>> LayoutTests/editing/execCommand/remove-format-background-color-expected.txt:3
>>>> +hello world WebKit.
>>> 
>>> Should we use something other than a dumpAsText test?  I think a script test that uses getComputedStyle or a reftest would be better for checking the background color.
>> 
>> I’m dumping HTML here. We don’t want to leave a span. It’s important the markup we generate is as concise as possible.
> 
> Ah, I see.  Using dump-as-markup.js would be more clear.  Should there be a test case with background-color: transparent?

No… I think we should be removing background-color: transparent as well in the future. We just don’t have a way to tell ApplyStyleCommand that it should remove a property regardless of its value.

[Ryosuke Niwa]

Committed r134096: <http://trac.webkit.org/changeset/134096>

[Tony Chang]

(In reply to comment #6)
> > Should there be a test case with background-color: transparent?
> 
> No… I think we should be removing background-color: transparent as well in the future. We just don’t have a way to tell ApplyStyleCommand that it should remove a property regardless of its value.

That's fine, you could check in a failing test case for that.  It would make it clear what the desired result is.