It appears that one of my changesets to markup.cpp / ReplaceSelectionCommand.cpp introduced a regression.
Created attachment 113011 [details] demo
Created attachment 113014 [details] fixes the bug
Comment on attachment 113014 [details] fixes the bug Does this do the right thing for the following case? <div contentEditable> foo <div style="background: transparent">bar</div> baz </div> If you select bar and copy-paste it within the contentEditable element, should we maintain the transparent background?
(In reply to comment #3) > (From update of attachment 113014 [details]) > Does this do the right thing for the following case? > > <div contentEditable> > foo > <div style="background: transparent">bar</div> > baz > </div> > > If you select bar and copy-paste it within the contentEditable element, should we maintain the transparent background? We should not.
What about copy-pasting "foo" in the following case? Both the original and the copy-pasted foo should have a transparent background, right? Does that work with the following code? If so, can you add a test for this case as well? <style> .foo { background: blue; } </style> <div contentEditable> a<span class=foo style="background:transparent">foo</span>b </div>
(In reply to comment #5) > What about copy-pasting "foo" in the following case? Both the original and the copy-pasted foo should have a transparent background, right? Does that work with the following code? If so, can you add a test for this case as well? > > <style> > .foo { > background: blue; > } > </style> > <div contentEditable> > a<span class=foo style="background:transparent">foo</span>b > </div> Sure. Let me add that as a test.
Created attachment 113089 [details] Added ojan's test case also fixed a bug
Comment on attachment 113089 [details] Added ojan's test case also fixed a bug View in context: https://bugs.webkit.org/attachment.cgi?id=113089&action=review > Source/WebCore/editing/EditingStyle.cpp:1090 > - computedStyle->removePropertiesInElementDefaultStyle(element); > + if (!computedStyle->m_mutableStyle->getPropertyCSSValue(CSSPropertyBackgroundColor)) > + computedStyle->m_mutableStyle->setProperty(CSSPropertyBackgroundColor, CSSValueTransparent); > + > + removePropertiesInStyle(computedStyle->m_mutableStyle.get(), styleFromMatchedRules.get()); This changes from removing styleFromMatchedRules using AllButEmptyCSSRules to styleFromMatchedRules using UAAndUserCSSRules. Is that correct? Should 1087-1088 just be in removePropertiesInElementDefaultStyle? Sorry, I don't know this code well enough to know if this is the right change.
(In reply to comment #8) > (From update of attachment 113089 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113089&action=review > > > Source/WebCore/editing/EditingStyle.cpp:1090 > > - computedStyle->removePropertiesInElementDefaultStyle(element); > > + if (!computedStyle->m_mutableStyle->getPropertyCSSValue(CSSPropertyBackgroundColor)) > > + computedStyle->m_mutableStyle->setProperty(CSSPropertyBackgroundColor, CSSValueTransparent); > > + > > + removePropertiesInStyle(computedStyle->m_mutableStyle.get(), styleFromMatchedRules.get()); > > This changes from removing styleFromMatchedRules using AllButEmptyCSSRules to styleFromMatchedRules using UAAndUserCSSRules. Is that correct? Yes. That was a bug. Without this change, we'll end up removing background: transparent in your test case.
Any reviewers?
Yay! Thanks for the review.
Comment on attachment 113089 [details] Added ojan's test case also fixed a bug Clearing flags on attachment: 113089 Committed r99067: <http://trac.webkit.org/changeset/99067>
All reviewed patches have been landed. Closing bug.