Bug 116215 – background-color and text-decoration are not preserved when merging paragraphs

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

[Grzegorz Czajkowski]

CSS properties background-color and text-decoration defined in class are not copied when paragraph is being merged.
It works fine when they are defined inline.

We already had similar issue for determining typing style (bug 53196).
This is probably a regression from http://trac.webkit.org/changeset/130532.

[Grzegorz Czajkowski]

In my opinion, in EditingStyle::mergeInlineAndImplicitStyleOfElement we erroneously copy only CSS inheritable properties via removeNonEditingProperties(). 

It seems that passing AllEditingProperties (instead of implicit OnlyInheritableEditingProperties) solves this issue.

Ryosuke, I'd glad hear your opinion on that.

[Grzegorz Czajkowski]

Created attachment 201939 [details]
test case

[Grzegorz Czajkowski]

Created attachment 202049 [details]
WIP patch to get Mac layout tests result

[Grzegorz Czajkowski]

(In reply to comment #3)
> Created an attachment (id=202049) [details]
> WIP patch to get Mac layout tests result

This patch needs to distinguish propertyToInlcude in  mergeInlineAndImplicitStyleOfElement as we don't want to include non inheritable properties sometimes, for example propertyToInlcude == OnlyEditingInheritableProperties.

I'll fix it and add layout test for that issue in the next patch.

[Grzegorz Czajkowski]

Created attachment 202264 [details]
proposed patch

[Ryosuke Niwa]

Comment on attachment 202264 [details]
proposed patch

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

> Source/WebCore/ChangeLog:8
> +        New test - editing/deleting/merge-paragraph-from-span-with-style.html

Nit: wrong format. This should appear after the long description but before per-file description and should be in the form of:
Test: editing/deleting/merge-paragraph-from-span-with-style.html

See other change log entries that add tests.

> Source/WebCore/editing/EditingStyle.cpp:619
> -        m_mutableStyle = copyEditingProperties(m_mutableStyle.get());
> +        m_mutableStyle = copyEditingProperties(m_mutableStyle.get(), removeNonInheritableEditingProperties ? OnlyInheritableEditingProperties : AllEditingProperties);

Either this boolean means removeInheritableEditingProperties or that this code is wrong. r- because of this.

[Grzegorz Czajkowski]

Comment on attachment 202264 [details]
proposed patch

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

>> Source/WebCore/ChangeLog:8
>> +        New test - editing/deleting/merge-paragraph-from-span-with-style.html
> 
> Nit: wrong format. This should appear after the long description but before per-file description and should be in the form of:
> Test: editing/deleting/merge-paragraph-from-span-with-style.html
> 
> See other change log entries that add tests.

Right. I'll move it below the long description and change the format of this entry to the recommended one.

>> Source/WebCore/editing/EditingStyle.cpp:619
>> +        m_mutableStyle = copyEditingProperties(m_mutableStyle.get(), removeNonInheritableEditingProperties ? OnlyInheritableEditingProperties : AllEditingProperties);
> 
> Either this boolean means removeInheritableEditingProperties or that this code is wrong. r- because of this.

My intention was to add possibility of not removing non inheritable editing properties via 'removeNonEditingProperties' method. In context of 'copyEditingProperties' method, 'removeNonInheritableEditingProperties' boolean means copy only inheritable editing properties.
I believe, 'preserveNonInheritableEditingProperties' will have a better meaning.

[Grzegorz Czajkowski]

Created attachment 202383 [details]
apply Ryosuke's suggestions

[WebKit Commit Bot]

Attachment 202383 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/editing/deleting/merge-paragraph-from-span-with-style-expected.txt', u'LayoutTests/editing/deleting/merge-paragraph-from-span-with-style.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/editing/EditingStyle.cpp', u'Source/WebCore/editing/EditingStyle.h']" exit_code: 1
Source/WebCore/editing/EditingStyle.cpp:619:  Extra space before )  [whitespace/parens] [2]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Grzegorz Czajkowski]

Created attachment 202386 [details]
fix coding style

[Ryosuke Niwa]

Comment on attachment 202386 [details]
fix coding style

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

> LayoutTests/editing/deleting/merge-paragraph-from-span-with-style.html:1
> +<html>

Missing DOCTYPE. We also don't indent the markup like it's done here.

> LayoutTests/editing/deleting/merge-paragraph-from-span-with-style.html:7
> +          .NonInheritableProperties {
> +              background-color: yellow;
> +              text-decoration: underline;
> +          }

Why does this need to be in the CSS rule? Can't it be an inline style?

> Source/WebCore/editing/EditingStyle.cpp:616
> +void EditingStyle::removeNonEditingProperties(bool preserveNonInheritableEditingProperties)

I don't think we should be introducing terms like "preserve". It's confusing.
Why can't we call this removeInheritableEditingProperties and negate the condition?

[Grzegorz Czajkowski]

(In reply to comment #11)
> (From update of attachment 202386 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202386&action=review
> 
> > LayoutTests/editing/deleting/merge-paragraph-from-span-with-style.html:1
> > +<html>
> 
> Missing DOCTYPE. We also don't indent the markup like it's done here.

Right. Thanks.

> 
> > LayoutTests/editing/deleting/merge-paragraph-from-span-with-style.html:7
> > +          .NonInheritableProperties {
> > +              background-color: yellow;
> > +              text-decoration: underline;
> > +          }
> 
> Why does this need to be in the CSS rule? Can't it be an inline style?

This bug is not reproducible for an inline style. I mentioned about this in ChangeLog.

> 
> > Source/WebCore/editing/EditingStyle.cpp:616
> > +void EditingStyle::removeNonEditingProperties(bool preserveNonInheritableEditingProperties)
> 
> I don't think we should be introducing terms like "preserve". It's confusing.
> Why can't we call this removeInheritableEditingProperties and negate the condition?

Do we need to remove inheritable editing properties anywhere? This fix doesn't need it.
I am asking as I am little confused what '!removeInheritableEditingProperties' means in context of EditingStyle::copyEditingProperties. Assuming we have two options to copy the styles (OnlyInheritableEditingProperties, AllEditingProperties) the proposed negation should copy AllEditingProperties I guess.

That's why I proposed 'preserveNonInheritableEditingProperties' which can be used for both removeNonEditingProperties and copyEditingProperties.

Thanks for your review!

[Grzegorz Czajkowski]

Created attachment 204705 [details]
passing EditingPropertiesToInclude to removeAllButEditingProperties()

[Darin Adler]

Comment on attachment 204705 [details]
passing EditingPropertiesToInclude to removeAllButEditingProperties()

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

Generally looks OK. Would be nice to have more than just the single test.

> Source/WebCore/ChangeLog:22
> +        Reverse logic to be used after casting PropertiesToInlcude to EditingPropertiesToInclude.

Typo "Inlcude" occurs many times in this change log.

> Source/WebCore/ChangeLog:38
> +        Reorder PropertiesToInlcude values to match EditingPropertiesToInclude to easy cast it.

This is a bad design pattern. We don’t want subtle relationships between different enums.

> Source/WebCore/editing/EditingStyle.cpp:975
> +    styleFromRules->removeAllButEditingProperties(static_cast<EditingPropertiesToInclude>(propertiesToInclude));

How can this cast be correct? One enum has 3 values and the other has 2. Either this won’t do the right thing if propertiesToInclude is AllProperties, or it does the right thing for a strange reason.

[Grzegorz Czajkowski]

(In reply to comment #14)
> (From update of attachment 204705 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204705&action=review
> 
> Generally looks OK. Would be nice to have more than just the single test.
> 
> > Source/WebCore/ChangeLog:22
> > +        Reverse logic to be used after casting PropertiesToInlcude to EditingPropertiesToInclude.
> 
> Typo "Inlcude" occurs many times in this change log.
> 
> > Source/WebCore/ChangeLog:38
> > +        Reorder PropertiesToInlcude values to match EditingPropertiesToInclude to easy cast it.
> 
> This is a bad design pattern. We don’t want subtle relationships between different enums.
> 
> > Source/WebCore/editing/EditingStyle.cpp:975
> > +    styleFromRules->removeAllButEditingProperties(static_cast<EditingPropertiesToInclude>(propertiesToInclude));
> 
> How can this cast be correct? One enum has 3 values and the other has 2. Either this won’t do the right thing if propertiesToInclude is AllProperties, or it does the right thing for a strange reason.

Thanks Darin for review! I'll take it into consideration and submit next patch.

[Grzegorz Czajkowski]

Created attachment 204891 [details]
apply Darin's comments

[Ryosuke Niwa]

Comment on attachment 204891 [details]
apply Darin's comments 

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

> LayoutTests/editing/deleting/merge-div-from-span-with-style.html:19
> +Markup.description('The span style should be preserved when merging div element.\n' +
> +                   'The test passes if the bar contatins the yellow background and it\'s underlined.');

Wrong indentation. ' on the second line should appear exactly 4 spaces to the right of Markup.
Also + should be at the beginning of the second line, not at the end of the first line.

> LayoutTests/editing/deleting/merge-div-from-span-with-style.html:22
> +var spanToMerge = document.getElementById("spanToMerge");
> +spanToMerge.focus();

Why do we need a local variable here?
Why not just document.getElementById("spanToMerge").focus()?

> LayoutTests/editing/deleting/merge-div-from-span-with-style.html:24
> +var selection = window.getSelection();
> +selection.collapse(spanToMerge, 0);

It doesn't seem necessary to define selection here.
You can just do getSelection().collapse(spanToMerge, 0); instead.

> LayoutTests/editing/deleting/merge-paragraph-from-span-with-style.html:7
> +text-decoration: underline;

Please add a test case when we have multiple text-decorations; e.g. underline & strike-through.

> LayoutTests/editing/deleting/merge-paragraph-from-span-with-style.html:26
> +Markup.description('The span style should be preserved when merging a pragraph.\n' +
> +                   'The test passes if the bar contatins the yellow background and it\'s underlined.');
> +
> +var spanToMerge = document.getElementById("spanToMerge");
> +spanToMerge.focus();
> +var selection = window.getSelection();
> +selection.collapse(spanToMerge, 0);
> +document.execCommand("Delete");
> +Markup.dump(document.getElementById("test"));

Ditto.

> Source/WebCore/editing/EditingStyle.cpp:1308
> +EditingStyle::EditingPropertiesToInclude EditingStyle::toEditingProperties(PropertiesToInclude properties)

This function doesn't need to be a member function. We can just keep it as an inline static function.

[Grzegorz Czajkowski]

Created attachment 204985 [details]
apply Ryosuke's review

[WebKit Commit Bot]

Comment on attachment 204985 [details]
apply Ryosuke's review

Clearing flags on attachment: 204985

Committed r151772: <http://trac.webkit.org/changeset/151772>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.