Bug 58925 - REGRESSION(r55762): Highlight color can't be copied in gmail
Summary: REGRESSION(r55762): Highlight color can't be copied in gmail
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-04-19 13:55 PDT by Enrica Casucci
Modified: 2011-04-19 16:38 PDT (History)
4 users (show)

See Also:


Attachments
Patch (11.15 KB, patch)
2011-04-19 14:58 PDT, Enrica Casucci
rniwa: review-
Details | Formatted Diff | Diff
Patch2 (11.50 KB, patch)
2011-04-19 15:45 PDT, Enrica Casucci
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2011-04-19 13:55:30 PDT
* 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>
Comment 1 Enrica Casucci 2011-04-19 13:57:53 PDT
The change in r55762 uncovered the real issue. The markup placed in the pasteboard doesn't contain the background color.
Comment 2 Enrica Casucci 2011-04-19 14:58:15 PDT
Created attachment 90260 [details]
Patch
Comment 3 Ryosuke Niwa 2011-04-19 15:01:06 PDT
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.
Comment 4 Ryosuke Niwa 2011-04-19 15:06:42 PDT
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?
Comment 5 Enrica Casucci 2011-04-19 15:15:26 PDT
I will definitely do that, thanks!
Comment 6 Enrica Casucci 2011-04-19 15:15:58 PDT
Will do.
Comment 7 Enrica Casucci 2011-04-19 15:45:49 PDT
Created attachment 90268 [details]
Patch2

Uses exiting hasTransparentBackgroundColor from the Editor class.
Added instructions to run test manually.
Comment 8 Ryosuke Niwa 2011-04-19 15:49:47 PDT
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.
Comment 9 Enrica Casucci 2011-04-19 15:55:56 PDT
> 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.
Comment 10 Ryosuke Niwa 2011-04-19 15:56:26 PDT
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%.
Comment 11 Ryosuke Niwa 2011-04-19 15:58:15 PDT
(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.
Comment 12 Enrica Casucci 2011-04-19 16:05:59 PDT
> 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.
Comment 13 Ryosuke Niwa 2011-04-19 16:07:08 PDT
(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?
Comment 14 Enrica Casucci 2011-04-19 16:10:09 PDT
> How about when we're in design mode?
I did not add highestEnclosingNodeOfType, therefore it behaves as it always had.
Comment 15 Ryosuke Niwa 2011-04-19 16:22:53 PDT
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.
Comment 16 Enrica Casucci 2011-04-19 16:31:25 PDT
(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!
Comment 17 Enrica Casucci 2011-04-19 16:38:07 PDT
http://trac.webkit.org/changeset/84311