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.
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.
Created attachment 201939 [details] test case
Created attachment 202049 [details] WIP patch to get Mac layout tests result
(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.
Created attachment 202264 [details] proposed patch
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.
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.
Created attachment 202383 [details] apply Ryosuke's suggestions
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.
Created attachment 202386 [details] fix coding style
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?
(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!
Created attachment 204705 [details] passing EditingPropertiesToInclude to removeAllButEditingProperties()
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.
(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.
Created attachment 204891 [details] apply Darin's comments
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.
Created attachment 204985 [details] apply Ryosuke's review
Comment on attachment 204985 [details] apply Ryosuke's review Clearing flags on attachment: 204985 Committed r151772: <http://trac.webkit.org/changeset/151772>
All reviewed patches have been landed. Closing bug.