Bug 60988 - Wrap copied contents by one style span instead of two
Summary: Wrap copied contents by one style span instead of two
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 60996 60998
Blocks: 34564 60914
  Show dependency treegraph
 
Reported: 2011-05-17 14:27 PDT by Ryosuke Niwa
Modified: 2011-05-23 14:08 PDT (History)
13 users (show)

See Also:


Attachments
work in progress (13.55 KB, patch)
2011-05-17 14:38 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixes the bug (28.39 KB, patch)
2011-05-18 00:23 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
reverted xcodeproj change (23.54 KB, patch)
2011-05-18 00:24 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.24 MB, application/zip)
2011-05-18 00:48 PDT, WebKit Review Bot
no flags Details
fixes the bug (34.28 KB, patch)
2011-05-19 14:00 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed per comment (33.36 KB, patch)
2011-05-20 14:16 PDT, Ryosuke Niwa
enrica: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-05-17 14:27:40 PDT
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.
Comment 1 Ryosuke Niwa 2011-05-17 14:38:00 PDT
Created attachment 93817 [details]
work in progress
Comment 2 Ryosuke Niwa 2011-05-18 00:23:53 PDT
Created attachment 93877 [details]
fixes the bug
Comment 3 Ryosuke Niwa 2011-05-18 00:24:47 PDT
Created attachment 93878 [details]
reverted xcodeproj change
Comment 4 Ryosuke Niwa 2011-05-18 00:29:11 PDT
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.
Comment 5 WebKit Review Bot 2011-05-18 00:48:15 PDT
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
Comment 6 WebKit Review Bot 2011-05-18 00:48:21 PDT
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
Comment 7 Ryosuke Niwa 2011-05-18 01:10:34 PDT
(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.
Comment 8 Ryosuke Niwa 2011-05-19 10:09:46 PDT
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.
Comment 9 Ryosuke Niwa 2011-05-19 14:00:12 PDT
Created attachment 94109 [details]
fixes the bug
Comment 10 Ryosuke Niwa 2011-05-19 14:09:00 PDT
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.
Comment 11 Enrica Casucci 2011-05-20 11:37:31 PDT
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?
Comment 12 Ryosuke Niwa 2011-05-20 11:42:50 PDT
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.
Comment 13 Enrica Casucci 2011-05-20 11:48:07 PDT
> 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.
Comment 14 Ryosuke Niwa 2011-05-20 11:52:09 PDT
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;
> -    }
Comment 15 Ryosuke Niwa 2011-05-20 14:16:13 PDT
Created attachment 94269 [details]
fixed per comment
Comment 16 Enrica Casucci 2011-05-20 14:19:56 PDT
Comment on attachment 94269 [details]
fixed per comment

Looks good now. Thanks for addressing the comments.
Comment 17 Ryosuke Niwa 2011-05-20 14:23:30 PDT
Committed r86983: <http://trac.webkit.org/changeset/86983>
Comment 18 Justin Garcia 2011-05-21 18:21:00 PDT
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!
Comment 19 Ryosuke Niwa 2011-05-21 18:39:34 PDT
(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.
Comment 20 Ademar Reis 2011-05-23 14:08:45 PDT
Revision r86983 cherry-picked into qtwebkit-2.2 with commit cff1777 <http://gitorious.org/webkit/qtwebkit/commit/cff1777>