Bug 27522 – Creating list clears background color

• Component: HTML Editing
• Status: RESOLVED FIXED

[Annie Sullivan]

STEPS TO REPRODUCE:
1. Type a few lines of text
2. Select all the text
3. Use execCommand to set the background color of the text.
4. Use execCommand to create and unordered list

EXPECTED RESULT:
List is created with background color

ACTUAL RESULT:
List is created, background color removed.

Repro at
http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cdiv%20id%3Deditor%20contenteditable%3Dtrue%3Eone%3Cdiv%3Etwo%3C%2Fdiv%3E%3Cdiv%3Ethree%3C%2Fdiv%3E%3C%2Fdiv%3E&ohh=1&ohj=1&jt=var%20div%20%3D%20document.getElementById('editor')%3B%0Adiv.focus()%3B%0A%2F%2F%20After%20this%2C%20background%20is%20yellow%0Adocument.execCommand('backcolor'%2Cfalse%2C%20'yellow')%3B%0Adiv.focus()%3B%0A%2F%2F%20After%20this%2C%20background%20color%20is%20removed.%0Adocument.execCommand('insertUnorderedList'%2C%20false%2C%20null)%3B%0Adiv.blur()%3B&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1

[Julie Parent]

Ryosuke, this looks up your alley.

[Tony Chang]

Created attachment 47104 [details]
Patch

[Tony Chang]

(In reply to comment #2)
> Created an attachment (id=47104) [details]
> Patch

A few notes:
- TextEdit doesn't seem to have the ability to have multiple background colors, so it's hard to use that as a reference implementation.
- This seems to match the behavior on Firefox which remembers the background color.
- 2 tests had to be rebaselined.  There's no visible change in these tests, but they are cases where the background color of a form control (white) was copied.  This seems like a general bug with form controls where we inherit styles-- we were already inheriting the font face and the font size from the form control.  I think this should be fixed in a different patch.

background-color is not and inheritable property

See <http://trac.webkit.org/changeset/49985> (fix for bug 27522).

[Ryosuke Niwa]

As Mitz pointed out, copying background-color all the time caused some regression and cannot be re-introduced.  We need an alternative approach to this problem.  Could you post a reduction as a separate attachment?

[Tony Chang]

Comment on attachment 47104 [details]
Patch

I see.  I will investigate some more.

[Tony Chang]

(In reply to comment #6)
> As Mitz pointed out, copying background-color all the time caused some
> regression and cannot be re-introduced.  We need an alternative approach to
> this problem.  Could you post a reduction as a separate attachment?

The reduction is in the original bug report.

[Enrica Casucci]

Comment on attachment 47104 [details]
Patch

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index f230799..6286626 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,15 @@
> [phone]  Tony Chang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=27522
> +        Preserve background color when creating a list.
> +
> +        * editing/execCommand/insert-list-preserve-bgcolor-expected.txt: Added.
> +        * editing/execCommand/insert-list-preserve-bgcolor.html: Added.
> +        * editing/inserting/5994480-2-expected.txt:
> +        * platform/mac/editing/pasteboard/select-element-1-expected.txt:
> +
>  2010-01-20  Daniel Bates  <[e-mail]>
>  
>          Unreviewed. Add failing test fast/css/button-height.html
> diff --git a/LayoutTests/editing/execCommand/insert-list-preserve-bgcolor-expected.txt b/LayoutTests/editing/execCommand/insert-list-preserve-bgcolor-expected.txt
> new file mode 100644
> index 0000000..a7236fa
> --- /dev/null
> +++ b/LayoutTests/editing/execCommand/insert-list-preserve-bgcolor-expected.txt
> @@ -0,0 +1,3 @@
> +this should have a yellow background
> +
> +<ul><li><span class="Apple-style-span" style="background-color: rgb(255, 255, 0); ">this should have a yellow background</span></li></ul>
> diff --git a/LayoutTests/editing/execCommand/insert-list-preserve-bgcolor.html b/LayoutTests/editing/execCommand/insert-list-preserve-bgcolor.html
> new file mode 100644
> index 0000000..f64442c
> --- /dev/null
> +++ b/LayoutTests/editing/execCommand/insert-list-preserve-bgcolor.html
> @@ -0,0 +1,14 @@
> +<body contentEditable='true'>
> +<p id="paragraph"><span style='background-color: yellow;'>this should have a yellow background</span></p>
> +<p id="results"></p>
> +</body>
> +<script src="../editing.js"></script>
> +<script>
> +  if (window.layoutTestController)
> +      layoutTestController.dumpAsText();
> +  var p = document.getElementById("paragraph");
> +  setSelectionCommand(p, 0, p, 0);
> +  execExtendSelectionForwardBySentenceCommand();
> +  document.execCommand("insertunorderedlist");
> +  document.getElementById("results").innerText = p.innerHTML;
> +</script>
> diff --git a/LayoutTests/editing/inserting/5994480-2-expected.txt b/LayoutTests/editing/inserting/5994480-2-expected.txt
> index 224592b..5a360f2 100644
> --- a/LayoutTests/editing/inserting/5994480-2-expected.txt
> +++ b/LayoutTests/editing/inserting/5994480-2-expected.txt
> @@ -1 +1 @@
> -<font class="Apple-style-span" face="'Lucida Grande'" size="3"><span class="Apple-style-span" style="font-size: 11px;"><br></span></font>
> +<font class="Apple-style-span" face="'Lucida Grande'" size="3"><span class="Apple-style-span" style="background-color: rgb(255, 255, 255); font-size: 11px;"><br></span></font>
> diff --git a/LayoutTests/platform/mac/editing/pasteboard/select-element-1-expected.txt b/LayoutTests/platform/mac/editing/pasteboard/select-element-1-expected.txt
> index 30d1d13..510acfb 100644
> --- a/LayoutTests/platform/mac/editing/pasteboard/select-element-1-expected.txt
> +++ b/LayoutTests/platform/mac/editing/pasteboard/select-element-1-expected.txt
> @@ -30,7 +30,7 @@ layer at (0,0) size 800x600
>            text run at (253,0) width 220: "All the options should be included."
>        RenderBlock {DIV} at (0,34) size 784x13
>          RenderInline {FONT} at (0,0) size 0x18
> -          RenderInline {SPAN} at (0,0) size 0x13
> +          RenderInline {SPAN} at (0,0) size 0x13 [bgcolor=#FFFFFF]
>              RenderBR {BR} at (0,0) size 0x13
>        RenderBlock {DIV} at (0,47) size 784x22
>          RenderMenuList {SELECT} at (2,2) size 62x18 [bgcolor=#FFFFFF]
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 47f3eb2..873b8b0 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,16 @@
> [phone]  Tony Chang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=27522
> +        Preserve background color when creating a list.
> +
> +        Test: editing/execCommand/insert-list-preserve-bgcolor.html
> +
> +        * editing/ApplyStyleCommand.cpp:
> +        (WebCore::):
> +        (WebCore::editingStyleAtPosition):
> +
>  2010-01-20  Fumitoshi Ukai  <[e-mail]>
>  
>          Reviewed by Alexey Proskuryakov.
> diff --git a/WebCore/editing/ApplyStyleCommand.cpp b/WebCore/editing/ApplyStyleCommand.cpp
> index 7a8f025..93d9310 100644
> --- a/WebCore/editing/ApplyStyleCommand.cpp
> +++ b/WebCore/editing/ApplyStyleCommand.cpp
> @@ -388,6 +388,7 @@ RefPtr<CSSMutableStyleDeclaration> getPropertiesNotInComputedStyle(CSSStyleDecla
>  // FIXME: The current editingStyleProperties contains all inheritableProperties but we may not need to preserve all inheritable properties
>  static const int editingStyleProperties[] = {
>      // CSS inheritable properties
> +    CSSPropertyBackgroundColor,
>      CSSPropertyBorderCollapse,
>      CSSPropertyColor,
>      CSSPropertyFontFamily,
This is not the correct fix and will have unwanted side effects. We had the equivalent bug for Indent command.Take a look at https://bugs.webkit.org/show_bug.cgi?id=29697 and also to https://bugs.webkit.org/show_bug.cgi?id=29740 to see how the style gets preserved.I did a lot of work in this area and I'll be happy to provide more context if you want.

[Tony Chang]

Created attachment 47180 [details]
work in progress

[Tony Chang]

(In reply to comment #10)
> Created an attachment (id=47180) [details]
> work in progress

I'm not done with this, but I wanted to post it to see if it's moving in the right direction.  There are some issues with an extra <br> getting inserted by moveParagraphWithClones and I need to make a thorough layout test for this.

[Ryosuke Niwa]

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=47180) [details] [details]
> > work in progress
> 
> I'm not done with this, but I wanted to post it to see if it's moving in the right direction.  There are some issues with an extra <br> getting inserted by moveParagraphWithClones and I need to make a thorough layout test for this.

Will your extra BRs go away if you used
    if (beforeParagraph.isNotNull() && !isTableElement(beforeParagraph.deepEquivalent().node()) && ((!isEndOfParagraph(beforeParagraph) && !isStartOfParagraph(beforeParagraph)) || beforeParagraph == afterParagraph)) {

instead of
    if (beforeParagraph.isNotNull() && !isTableElement(beforeParagraph.deepEquivalent().node()) && (!isEndOfParagraph(beforeParagraph) || beforeParagraph == afterParagraph)) {

for the last if statement in moveParagraphWithClones?

[Ryosuke Niwa]

This bug has been fixed by Enrica's patch.  Yay!