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
Ryosuke, this looks up your alley.
Created attachment 47104 [details] Patch
(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).
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?
Comment on attachment 47104 [details] Patch I see. I will investigate some more.
(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.
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 @@ > +2010-01-21 Tony Chang <tony@chromium.org> > + > + 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 <dbates@webkit.org> > > 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 @@ > +2010-01-21 Tony Chang <tony@chromium.org> > + > + 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 <ukai@chromium.org> > > 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.
Created attachment 47180 [details] work in progress
(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.
(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?
This bug has been fixed by Enrica's patch. Yay!