Background color applied on a selection of text is lost when the selection is deleted

Background color applied on a selection of text is lost when the selection is deleted

While deleting a selection, only inheritable style properties were
saved. Since background color is considered as noninheritable style
property, it was not being saved and hence was lost. Because of this
background color set on a selection of text was lost when the selection
was deleted and if new text is inserted after this it would not have
the background color set.

Before deleting a selection we should save all style properties applied
on that particular selection. Modified accordingly.

NOTE: This patch makes chrome behavior compatible with that of IE and FF.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158906

[yosin_UTC9, 2013-09-09 04:57:48]

It seems IE and FireFox keep background-color, please mention about this in
description. Since, this patch improves compatibility. 

Note: Editing API specification doesn't mention about keeping style after
deleting.

https://chromiumcodereview.appspot.com/19477010/diff/1/LayoutTests/editing/st...
File LayoutTests/editing/style/background-color-retained.html (right):

https://chromiumcodereview.appspot.com/19477010/diff/1/LayoutTests/editing/st...
LayoutTests/editing/style/background-color-retained.html:11:
document.execCommand("BackColor", false, "green");
Can we put <span style="background-color: green">..</span> in testDiv instead of
using "BackColor" command for making this test case independent from "BackColor"
command result?

https://chromiumcodereview.appspot.com/19477010/diff/1/LayoutTests/editing/st...
LayoutTests/editing/style/background-color-retained.html:12: execCutCommand();
"Cut" command works only in DRT. So, this test case should describe it.

https://chromiumcodereview.appspot.com/19477010/diff/1/LayoutTests/editing/st...
LayoutTests/editing/style/background-color-retained.html:12: execCutCommand();
Can we use "Delete" command instead of "Cut" command? I think it should be
"yes".

https://chromiumcodereview.appspot.com/19477010/diff/1/LayoutTests/editing/st...
LayoutTests/editing/style/background-color-retained.html:14:
execTypeCharacterCommand('N');
We don't need to use functions in editor.js.

https://chromiumcodereview.appspot.com/19477010/diff/1/LayoutTests/editing/st...
LayoutTests/editing/style/background-color-retained.html:42:
runDumpAsTextEditingTest(true);
This test has shouldBe(), so we don't need to dump text. Just call
js-test-post.js is fine.

https://chromiumcodereview.appspot.com/19477010/diff/1/Source/core/editing/De...
File Source/core/editing/DeleteSelectionCommand.cpp (right):

https://chromiumcodereview.appspot.com/19477010/diff/1/Source/core/editing/De...
Source/core/editing/DeleteSelectionCommand.cpp:295: m_typingStyle =
EditingStyle::create(m_selectionToDelete.start(),
EditingStyle::EditingPropertiesInEffect);
How about changing default value of EditingStyle::create() to
EditingPropertiesInEffect? 

It seems following functions also need to pass EditingPropertiesInEffect:
- CompositeEditCommand::moveParagraphs(),
- ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline
- ReplaceSelectionCommand::handleStyleSpansBeforeInsertion
- ReplaceSelectionCommand::handleStyleSpans
- InsertParagraphSeparatorCommand::calculateStyleBeforeInsertion

If all of them using EditingPropertiesInEffect, 
do we need to have OnlyEditingInheritableProperties?

https://chromiumcodereview.appspot.com/19477010/diff/1/Source/core/editing/De...
Source/core/editing/DeleteSelectionCommand.cpp:301: m_deleteIntoBlockquoteStyle
= EditingStyle::create(m_selectionToDelete.end());
Should we pass EditingStyle::EditingPropertiesInEffect?

[yosin_UTC9, 2013-09-09 05:24:01]

How about adding background-color into inheritableEditingProperties()? It seems
we remove background-color: transparent in
EditingStyle::removeStyleFromRulesAndContext().

[vanihegde, 2013-09-30 11:32:48]

On 2013/09/09 04:57:48, Yoshifumi Inoue wrote:
> It seems IE and FireFox keep background-color, please mention about this in
> description. Since, this patch improves compatibility. 
DONE
 
> Note: Editing API specification doesn't mention about keeping style after
> deleting.
> 
>
https://chromiumcodereview.appspot.com/19477010/diff/1/LayoutTests/editing/st...
> File LayoutTests/editing/style/background-color-retained.html (right):
> 
>
https://chromiumcodereview.appspot.com/19477010/diff/1/LayoutTests/editing/st...
> LayoutTests/editing/style/background-color-retained.html:11:
> document.execCommand("BackColor", false, "green");
> Can we put <span style="background-color: green">..</span> in testDiv instead
of
> using "BackColor" command for making this test case independent from
"BackColor"
> command result?
> 
>
https://chromiumcodereview.appspot.com/19477010/diff/1/LayoutTests/editing/st...
> LayoutTests/editing/style/background-color-retained.html:12: execCutCommand();
> "Cut" command works only in DRT. So, this test case should describe it.
> 
>
https://chromiumcodereview.appspot.com/19477010/diff/1/LayoutTests/editing/st...
> LayoutTests/editing/style/background-color-retained.html:12: execCutCommand();
> Can we use "Delete" command instead of "Cut" command? I think it should be
> "yes".
> 
>
https://chromiumcodereview.appspot.com/19477010/diff/1/LayoutTests/editing/st...
> LayoutTests/editing/style/background-color-retained.html:14:
> execTypeCharacterCommand('N');
> We don't need to use functions in editor.js.
> 
>
https://chromiumcodereview.appspot.com/19477010/diff/1/LayoutTests/editing/st...
> LayoutTests/editing/style/background-color-retained.html:42:
> runDumpAsTextEditingTest(true);
> This test has shouldBe(), so we don't need to dump text. Just call
> js-test-post.js is fine.

All the changes suggested for layout test can be done.

>
https://chromiumcodereview.appspot.com/19477010/diff/1/Source/core/editing/De...
> File Source/core/editing/DeleteSelectionCommand.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/19477010/diff/1/Source/core/editing/De...
> Source/core/editing/DeleteSelectionCommand.cpp:295: m_typingStyle =
> EditingStyle::create(m_selectionToDelete.start(),
> EditingStyle::EditingPropertiesInEffect);
> How about changing default value of EditingStyle::create() to
> EditingPropertiesInEffect? 

Works without regressing the existing layout tests. But I am not sure if
changing this would be right.

> It seems following functions also need to pass EditingPropertiesInEffect:
> - CompositeEditCommand::moveParagraphs(),
> - ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline
> - ReplaceSelectionCommand::handleStyleSpansBeforeInsertion
> - ReplaceSelectionCommand::handleStyleSpans
> - InsertParagraphSeparatorCommand::calculateStyleBeforeInsertion

Passing EditingPropertiesInEffect to
InsertParagraphSeparatorCommand::calculateStyleBeforeInsertion fixes
https://bug-117138-attachments.webkit.org/attachment.cgi?id=203572

For other cases, I could not figure out the need. But it does not cause any
regression either.

Also it does not apply for
- ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline
- ReplaceSelectionCommand::handleStyleSpans
as they the overloaded version
   static PassRefPtr<EditingStyle> create(const StylePropertySet* style)


> If all of them using EditingPropertiesInEffect, 
> do we need to have OnlyEditingInheritableProperties?
> 
>
https://chromiumcodereview.appspot.com/19477010/diff/1/Source/core/editing/De...
> Source/core/editing/DeleteSelectionCommand.cpp:301:
m_deleteIntoBlockquoteStyle
> = EditingStyle::create(m_selectionToDelete.end());
> Should we pass EditingStyle::EditingPropertiesInEffect?

Again, I could not figure out the need. But doing so does not cause regression
either.

[vanihegde, 2013-09-30 11:33:35]

On 2013/09/09 05:24:01, Yoshifumi Inoue wrote:
> How about adding background-color into inheritableEditingProperties()? It
seems
> we remove background-color: transparent in
> EditingStyle::removeStyleFromRulesAndContext().

The spec http://www.w3.org/TR/CSS2/propidx.html says background-color is not
inherited.
If we have to follow this, then it cannot be added to
inheritableEditingProperties.
Also, this introduces spans with background-color:transparent and hence causes
few layout tests to fail.

[yosin_UTC9, 2013-10-01 01:30:22]

On 2013/09/30 11:33:35, vanihegde wrote:
> On 2013/09/09 05:24:01, Yoshifumi Inoue wrote:
> > How about adding background-color into inheritableEditingProperties()? It
> seems
> > we remove background-color: transparent in
> > EditingStyle::removeStyleFromRulesAndContext().
> 
> The spec http://www.w3.org/TR/CSS2/propidx.html says background-color is not
> inherited.
> If we have to follow this, then it cannot be added to
> inheritableEditingProperties.
> Also, this introduces spans with background-color:transparent and hence causes
> few layout tests to fail.

I mean inheritable-editing-properties != CSS inherit and handle
background-color:transparent as other properties which have CSS value "inherit",
then removing "background-color:transparent" just before applying.

[ojan, 2013-10-02 01:04:13]

The C++ side of this patch LGTM. Please address Yoshifumi's comments on the
test.

I agree with Yoshifumi that we should look into whether we should get rid of
OnlyEditingInheritableProperties, but I'm also OK saying that it should be a
separate patch from this bug fix.

[vanihegde, 2013-10-03 05:55:35]

On 2013/10/02 01:04:13, ojan wrote:
> The C++ side of this patch LGTM. Please address Yoshifumi's comments on the
> test.
> 
> I agree with Yoshifumi that we should look into whether we should get rid of
> OnlyEditingInheritableProperties, but I'm also OK saying that it should be a
> separate patch from this bug fix.

I will look into it.

[vanihegde, 2013-10-03 05:58:49]

Using span.style.backgroundColor was wrong, as it was always returning
backgroundColor to be green even if the color was not actually retained.
So I have changed the test to use dump-as-markup.

[ojan, 2013-10-03 23:30:20]

lgtm

[commit-bot: I haz the power, 2013-10-03 23:31:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vani.hegde@samsung.com/19477010/11001

[commit-bot: I haz the power, 2013-10-04 07:52:03]

Change committed as 158906