Bug 60988

Created attachment 93817 [details] work in progress

Created attachment 93877 [details] fixes the bug

Created attachment 93878 [details] reverted xcodeproj change

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

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

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

Created attachment 94109 [details] fixes the bug

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

> 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 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; > - }

Created attachment 94269 [details] fixed per comment

Comment on attachment 94269 [details] fixed per comment Looks good now. Thanks for addressing the comments.

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

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!

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

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