Bug 58925 – REGRESSION(r55762): Highlight color can't be copied in gmail

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

[Enrica Casucci]

* STEPS TO REPRODUCE
1. Create new Gmail message
2. type "test"
3. select the word
4. select a background color from the toolbar
5. cmd-c to copy
6. hit the right arrow to move selection to the end of the word
7. cmd-v to paste

Expected:
testtest and the entire text has the selected background color

Actual:
the original word ("test") has the selected background whereas the pasted content has no background.

<rdar://problem/9253057>

[Enrica Casucci]

The change in r55762 uncovered the real issue. The markup placed in the pasteboard doesn't contain the background color.

[Enrica Casucci]

Created attachment 90260 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 90260 [details]
Patch

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

> Source/WebCore/editing/markup.cpp:455
> +static bool hasVisibleBackgroundColor(CSSStyleDeclaration* style)
> +{
> +    if (!style)
> +        return false;
> +    RefPtr<CSSValue> colorValue = style->getPropertyCSSValue(CSSPropertyBackgroundColor);
> +    if (!colorValue || !colorValue->isPrimitiveValue())
> +        return false;
> +    
> +    CSSPrimitiveValue* primitiveColor = static_cast<CSSPrimitiveValue*>(colorValue.get());
> +    RGBA32 rgba = 0;
> +
> +    if (primitiveColor->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR)
> +        rgba = primitiveColor->getRGBA32Value();
> +    else
> +        CSSParser::parseColor(rgba, colorValue->cssText());
> +    return alphaChannel(rgba);
> +}

There's hasTransparentBackgroundColor in Editor.cpp.  Please share code with that.

[Ryosuke Niwa]

Comment on attachment 90260 [details]
Patch

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

> LayoutTests/editing/pasteboard/copy-text-with-backgroundcolor.html:62
> +<div class="explanation">
> +<div class="scenario">
> +Tests: 
> +<br>
> +Fix for this bug: 
> +<a href="https://bugs.webkit.org/show_bug.cgi?id=58925">&lt;https://bugs.webkit.org/show_bug.cgi?id=58925&gt;</a> REGRESSION(r55762): Highlight color can't be copied in gmail.
> +</div>
> +<div class="expected-results">
> +Expected Results:
> +<br>
> +The pasted text should have the same background color as the copied text.
> +</div>
> +</div>

I'm not sure why you'd need so much markup here.  Also, could you explain the steps to manually (i.e. interactively) run this test?

[Enrica Casucci]

I will definitely do that, thanks!

[Enrica Casucci]

Will do.

[Enrica Casucci]

Created attachment 90268 [details]
Patch2

Uses exiting hasTransparentBackgroundColor from the Editor class.
Added instructions to run test manually.

[Ryosuke Niwa]

Comment on attachment 90268 [details]
Patch2

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

> Source/WebCore/editing/Editor.cpp:919
> -static bool hasTransparentBackgroundColor(CSSStyleDeclaration* style)
> +bool Editor::hasTransparentBackgroundColor(CSSStyleDeclaration* style)

Should it make more sense to move this function over to htmlediting.h/cpp?  I don't think we want to bloat the Editor class.

> LayoutTests/ChangeLog:9
> +        New tests.

Please explain what kind of a test you're adding.

> LayoutTests/editing/pasteboard/copy-text-with-backgroundcolor.html:60
> +The pasted text should have the same background color as the copied text. To run the test manually, select each of the two words with the same background color, copy, move the selection at the end of the second word and paste.

Please put some of this description in Markup.description so that it's printed out with the test results.

[Enrica Casucci]

> Should it make more sense to move this function over to htmlediting.h/cpp?  I don't think we want to bloat the Editor class.

I thought about that and decided against it, since all the functions in htmlediting.cpp deal with Position, VisiblePosition. Document and Node, but nothing that has to do with style.

> > LayoutTests/ChangeLog:9
> > +        New tests.
> 
> Please explain what kind of a test you're adding.
I thought it was fairly clear from the title of the bug and the file name of the test.

> 
> Please put some of this description in Markup.description so that it's printed out with the test results.
I believe there is enough description for such a simple test.

[Ryosuke Niwa]

Comment on attachment 90268 [details]
Patch2

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

> Source/WebCore/editing/markup.cpp:468
> -    return !propertyMissingOrEqualToNone(style.get(), CSSPropertyTextDecoration);
> +    return !propertyMissingOrEqualToNone(style.get(), CSSPropertyTextDecoration) || !Editor::hasTransparentBackgroundColor(style.get());

I'm not sure if this is the correct place to add this check.  This will force us to clone the highest ancestor with the background color.  So if html element had an opaque background color, we'd copy that as well.   But I don't think that's what we want.  We'd like to find the lowest common ancestor that has background color although this will not preserve the background color when the alpha value is less than 100%.

[Ryosuke Niwa]

(In reply to comment #9)
> > Please put some of this description in Markup.description so that it's printed out with the test results.
> I believe there is enough description for such a simple test.

Oh, I'm not saying that you need to explain anything.  I'm just pointing out that your nice description isn't included in the expected result because you didn't put them in Markup.description.

[Enrica Casucci]

> I'm not sure if this is the correct place to add this check.  This will force us to clone the highest ancestor with the background color.  So if html element had an opaque background color, we'd copy that as well.   But I don't think that's what we want.  We'd like to find the lowest common ancestor that has background color although this will not preserve the background color when the alpha value is less than 100%.

I don't believe this is true. The function I changed is used to decide which ancestors to consider and it is used, as I explained in the ChangeLog, by highestAncestorToWrapMarkup when calling highestEnclosingNodeOfType that will stop at root that is the highestEditableRoot.
I have not changed at all the logic to walk up the ancestors chain.

[Ryosuke Niwa]

(In reply to comment #9)
> > Should it make more sense to move this function over to htmlediting.h/cpp?  I don't think we want to bloat the Editor class.
> 
> I thought about that and decided against it, since all the functions in htmlediting.cpp deal with Position, VisiblePosition. Document and Node, but nothing that has to do with style.

Okay.  I was going to suggest EditingStyle.h/cpp but then this particular function doesn't fit well there either.  I guess keeping it as a method of the Editor class is fine as is.

(In reply to comment #12)
> I don't believe this is true. The function I changed is used to decide which ancestors to consider and it is used, as I explained in the ChangeLog, by highestAncestorToWrapMarkup when calling 
highestEnclosingNodeOfType that will stop at root that is the highestEditableRoot.

How about when we're in design mode?

[Enrica Casucci]

> How about when we're in design mode?
I did not add highestEnclosingNodeOfType, therefore it behaves as it always had.

[Ryosuke Niwa]

Comment on attachment 90268 [details]
Patch2

(In reply to comment #14)
> > How about when we're in design mode?
> I did not add highestEnclosingNodeOfType, therefore it behaves as it always had.

Well, I meant that in design mode, root editable element is body so it'll clone all nodes up to body element if body has an opaque background but it seems like we have a way of handling that in paste side so I guess we're fine.

[Enrica Casucci]

(In reply to comment #15)
> (From update of attachment 90268 [details])
> (In reply to comment #14)
> > > How about when we're in design mode?
> > I did not add highestEnclosingNodeOfType, therefore it behaves as it always had.
> 
> Well, I meant that in design mode, root editable element is body so it'll clone all nodes up to body element if body has an opaque background but it seems like we have a way of handling that in paste side so I guess we're fine.

The case of the of the body with opaque background is accounted for in this code and we have a test that verifies that we preserve that correctly (editing/pasteboard/19644-2.html). Keep in mind also that block elements in the markup generation code are handled in a different way.

Thanks for the review!

[Enrica Casucci]

http://trac.webkit.org/changeset/84311