Bug 71203 – REGRESSION(r96870): WebKit generates background: transparent on blogger.com

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

[Ryosuke Niwa]

It appears that one of my changesets to markup.cpp / ReplaceSelectionCommand.cpp introduced a regression.

[Ryosuke Niwa]

Created attachment 113011 [details]
demo

[Ryosuke Niwa]

Created attachment 113014 [details]
fixes the bug

[Ojan Vafai]

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?

[Ryosuke Niwa]

(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.

[Ojan Vafai]

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>

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 113089 [details]
Added ojan's test case also fixed a bug

[Ojan Vafai]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Any reviewers?

[Ryosuke Niwa]

Yay! Thanks for the review.

[Ryosuke Niwa]

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>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.