Bug 27522

Summary:

Creating list clears background color

Product:

WebKit

Reporter:

Annie Sullivan <sullivan>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

enrica, eric, jparent, michaelthomas, mitz, rniwa, tony

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

URL:

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

Bug Depends on:

42436

Bug Blocks:

Attachments:

Description	Flags
Patch	none
work in progress	none

Annie Sullivan

Reported 2009-07-21 15:23:14 PDT

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

Attachments
Patch (4.71 KB, patch) 2010-01-21 01:17 PST, Tony Chang	no flags	Details Formatted Diff Diff
work in progress (5.01 KB, patch) 2010-01-22 01:12 PST, Tony Chang	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Julie Parent

Comment 1 2009-07-21 17:22:26 PDT

Ryosuke, this looks up your alley.

Tony Chang

Comment 2 2010-01-21 01:17:38 PST

Created attachment 47104 [details] Patch

Tony Chang

Comment 3 2010-01-21 01:20:30 PST

(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.

mitz

Comment 4 2010-01-21 01:27:11 PST

background-color is not and inheritable property

mitz

Comment 5 2010-01-21 01:30:28 PST

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

Ryosuke Niwa

Comment 6 2010-01-21 01:33:02 PST

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 7 2010-01-21 01:35:36 PST

Comment on attachment 47104 [details] Patch I see. I will investigate some more.

Tony Chang

Comment 8 2010-01-21 01:36:34 PST

(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 9 2010-01-21 08:49:41 PST

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>this should have a yellow background</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'> > +this should have a yellow background > + > +</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 @@ > - > + > 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.

Tony Chang

Comment 10 2010-01-22 01:12:48 PST

Created attachment 47180 [details] work in progress

Tony Chang

Comment 11 2010-01-22 01:14:13 PST

(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 getting inserted by moveParagraphWithClones and I need to make a thorough layout test for this.

Ryosuke Niwa

Comment 12 2010-07-13 23:40:59 PDT

(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 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

Comment 13 2011-08-04 13:56:52 PDT

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

Note You need to log in before you can comment on or make changes to this bug.