Bug 116215

Summary: background-color and text-decoration are not preserved when merging paragraphs
Product: WebKit Reporter: Grzegorz Czajkowski <g.czajkowski>
Component: HTML EditingAssignee: Grzegorz Czajkowski <g.czajkowski>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
test case
none
WIP patch to get Mac layout tests result
none
proposed patch
rniwa: review-
apply Ryosuke's suggestions
none
fix coding style
none
passing EditingPropertiesToInclude to removeAllButEditingProperties()
darin: review-
apply Darin's comments
none
apply Ryosuke's review none

Grzegorz Czajkowski
Reported 2013-05-16 03:01:18 PDT
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.
Attachments
test case (356 bytes, text/html)
2013-05-16 03:34 PDT, Grzegorz Czajkowski
no flags
WIP patch to get Mac layout tests result (1.81 KB, patch)
2013-05-17 00:06 PDT, Grzegorz Czajkowski
no flags
proposed patch (6.31 KB, patch)
2013-05-20 02:15 PDT, Grzegorz Czajkowski
rniwa: review-
apply Ryosuke's suggestions (6.31 KB, patch)
2013-05-21 00:01 PDT, Grzegorz Czajkowski
no flags
fix coding style (6.31 KB, patch)
2013-05-21 00:15 PDT, Grzegorz Czajkowski
no flags
passing EditingPropertiesToInclude to removeAllButEditingProperties() (8.95 KB, patch)
2013-06-14 05:33 PDT, Grzegorz Czajkowski
darin: review-
apply Darin's comments (11.83 KB, patch)
2013-06-18 02:06 PDT, Grzegorz Czajkowski
no flags
apply Ryosuke's review (13.09 KB, patch)
2013-06-19 04:07 PDT, Grzegorz Czajkowski
no flags
Grzegorz Czajkowski
Comment 1 2013-05-16 03:09:42 PDT
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
Comment 2 2013-05-16 03:34:14 PDT
Created attachment 201939 [details] test case
Grzegorz Czajkowski
Comment 3 2013-05-17 00:06:29 PDT
Created attachment 202049 [details] WIP patch to get Mac layout tests result
Grzegorz Czajkowski
Comment 4 2013-05-17 08:10:12 PDT
(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
Comment 5 2013-05-20 02:15:40 PDT
Created attachment 202264 [details] proposed patch
Ryosuke Niwa
Comment 6 2013-05-20 12:23:39 PDT
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 7 2013-05-21 00:00:02 PDT
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
Comment 8 2013-05-21 00:01:21 PDT
Created attachment 202383 [details] apply Ryosuke's suggestions
WebKit Commit Bot
Comment 9 2013-05-21 00:04:52 PDT
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
Comment 10 2013-05-21 00:15:25 PDT
Created attachment 202386 [details] fix coding style
Ryosuke Niwa
Comment 11 2013-05-21 00:32:49 PDT
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
Comment 12 2013-05-21 05:24:52 PDT
(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
Comment 13 2013-06-14 05:33:45 PDT
Created attachment 204705 [details] passing EditingPropertiesToInclude to removeAllButEditingProperties()
Darin Adler
Comment 14 2013-06-14 09:29:32 PDT
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
Comment 15 2013-06-17 07:14:14 PDT
(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
Comment 16 2013-06-18 02:06:13 PDT
Created attachment 204891 [details] apply Darin's comments
Ryosuke Niwa
Comment 17 2013-06-18 11:38:16 PDT
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
Comment 18 2013-06-19 04:07:03 PDT
Created attachment 204985 [details] apply Ryosuke's review
WebKit Commit Bot
Comment 19 2013-06-20 00:40:04 PDT
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
Comment 20 2013-06-20 00:40:07 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.