Bug 60988 – Wrap copied contents by one style span instead of two

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

[Ryosuke Niwa]

createMarkup currently generates two wrapping spans around the serialized contents.  We should merge these two spans into one.

http://trac.webkit.org/changeset/30649 introduced the notion of document and user style spans.

[Ryosuke Niwa]

Created attachment 93817 [details]
work in progress

[Ryosuke Niwa]

Created attachment 93877 [details]
fixes the bug

[Ryosuke Niwa]

Created attachment 93878 [details]
reverted xcodeproj change

[Ryosuke Niwa]

This patch basically reverts http://trac.webkit.org/changeset/30649 and solves rdar://problem/4930986 by diff'ing style against document's default style.  This change paves a way to resolve the bugs 34564 and 60914.

[WebKit Review Bot]

Comment on attachment 93878 [details]
reverted xcodeproj change

Attachment 93878 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8708452

New failing tests:
editing/pasteboard/paste-text-011.html
editing/pasteboard/paste-text-003.html

[WebKit Review Bot]

Created attachment 93881 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Ryosuke Niwa]

(In reply to comment #5)
> (From update of attachment 93878 [details])
> Attachment 93878 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8708452
> 
> New failing tests:
> editing/pasteboard/paste-text-011.html
> editing/pasteboard/paste-text-003.html

Ugh... these tests have Chromium Windows specific results.

[Ryosuke Niwa]

Comment on attachment 93878 [details]
reverted xcodeproj change

Removing r? flag for now since this patch will very likely collide with my patch for the bug 61012.

[Ryosuke Niwa]

Created attachment 94109 [details]
fixes the bug

[Ryosuke Niwa]

This refactoring is required to fix the bug 60914, which is a Chrome release blocker for m-13 (See crbug.com/80498).  If we cannot fix this bug on time, we'd have to revert http://trac.webkit.org/changeset/84311 and http://trac.webkit.org/changeset/85090.

[Enrica Casucci]

Comment on attachment 94109 [details]
fixes the bug

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

> LayoutTests/editing/pasteboard/5245519-expected.txt:2
> +Hello <span style="display: none; " class="Apple-style-span"></span>World!

This is ugly. We should not have an invisible empty span.

> Source/WebCore/editing/EditingStyle.cpp:711
>  

Why are you doing this for font color? I thought the problem was the background color. What am I missing here?

[Ryosuke Niwa]

Comment on attachment 94109 [details]
fixes the bug

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

>> LayoutTests/editing/pasteboard/5245519-expected.txt:2
>> +Hello <span style="display: none; " class="Apple-style-span"></span>World!
> 
> This is ugly. We should not have an invisible empty span.

However, we're explicitly inserting an invisible span.

>> Source/WebCore/editing/EditingStyle.cpp:711
>>  
> 
> Why are you doing this for font color? I thought the problem was the background color. What am I missing here?

No, this patch itself is nothing to do with background color.  The reason I do this here is so that color: black == color: rgb(0, 0, 0) and color: black will be removed in the tests where we explicitly specify style span with color: black.

[Enrica Casucci]

> However, we're explicitly inserting an invisible span.
>
We did not before. Why is this happening now and why is it ok?
 
> No, this patch itself is nothing to do with background color.  The reason I do this here is so that color: black == color: rgb(0, 0, 0) and color: black will be removed in the tests where we explicitly specify style span with color: black.

Ok, I understand.

[Ryosuke Niwa]

Comment on attachment 94109 [details]
fixes the bug

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

Because we use to execute the following line for the said style span.  However, as far as I know, we don't serialize invisible contents in createMarkup whenever annotation is on so this shouldn't be an issue in practice.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:-675
> -    if (!sourceDocumentStyle->isEmpty() && !copiedRangeStyleSpan) {
> -        copyStyleToChildren(sourceDocumentStyleSpan, sourceDocumentStyle->style()); 
> -        removeNodePreservingChildren(sourceDocumentStyleSpan);
> -        return;
> -    }

[Ryosuke Niwa]

Created attachment 94269 [details]
fixed per comment

[Enrica Casucci]

Comment on attachment 94269 [details]
fixed per comment

Looks good now. Thanks for addressing the comments.

[Ryosuke Niwa]

Committed r86983: <http://trac.webkit.org/changeset/86983>

[Justin Garcia]

When I made this change, I wrote:

"we'll want to let Mail's Paste As Quotation blockquote override document default styles, but not others."

I think that I was concerned with keeping text that was explicitly set to black, copied, and pasted with Paste as Quotation from becoming the quotation color. I see now that this strange case was not worth complicating the code for. Thanks for fixing this!

[Ryosuke Niwa]

(In reply to comment #18)
> I think that I was concerned with keeping text that was explicitly set to black, copied, and pasted with Paste as Quotation from becoming the quotation color. I see now that this strange case was not worth complicating the code for. Thanks for fixing this!

Yeah, that's the only difference in new behavior.  At the end of the day, a user can still re-set the color back if they really want to keep them color black, etc... after paste.  And I think the user probably appreciates being able to preserve background color more.

[Ademar Reis]

Revision r86983 cherry-picked into qtwebkit-2.2 with commit cff1777 <http://gitorious.org/webkit/qtwebkit/commit/cff1777>