Bug 60914

Summary: REGRESSION(r84311): WebKit copies too much styles when copying
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, ademar, darin, dglazkov, enrica, komoroske, morrita, ojan, rniwa, tkent, tony, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 60920, 60923, 60988    
Bug Blocks:    
Attachments:
Description Flags
work in progress
none
fixes the bug
none
Archive of layout-test-results from ec2-cr-linux-02
none
patch made by svn enrica: review+

Ryosuke Niwa
Reported 2011-05-16 12:41:29 PDT
Apparently, http://trac.webkit.org/changeset/85090 wasn't enough. We still get reports on http://crbug.com/80498. Reproduction steps: 1. Go to gmail 2. Copy any mail content (without reply) 3. Pate into contentedtiable area Expected result: Only copied content is pasted Actual result: Too much style is copied with the content.
Attachments
work in progress (8.16 KB, patch)
2011-05-16 14:10 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
fixes the bug (23.44 KB, patch)
2011-05-20 18:19 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ec2-cr-linux-02 (1.34 MB, application/zip)
2011-05-20 18:59 PDT, WebKit Review Bot 		no flags
Details
patch made by svn (27.89 KB, patch)
2011-05-20 21:14 PDT, Ryosuke Niwa 		enrica: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2011-05-16 14:10:02 PDT
Created attachment 93693 [details] work in progress New approach is that we'll add text decoration property in the wrapping span instead of cloning the node that has this property as a special common ancestor.
Ryosuke Niwa
Comment 2 2011-05-16 20:02:51 PDT
I spent whole day debugging this bug but haven't found a solution yet. The problem is that if I add background color property like I'm doing in the work-in-progress patch, then string comparison in handleStyleSpansBeforeInsertion fail and we end up leaving style spans in paste. 592 // FIXME: This string comparison is a naive way of comparing two styles. 593 // We should be taking the diff and check that the diff is empty. 594 if (styleText == static_cast<Element*>(sourceDocumentStyleSpan)->getAttribute(styleAttr)) { 595 fragment.removeNodePreservingChildren(sourceDocumentStyleSpan); 596 if (!isStyleSpan(copiedRangeStyleSpan.get())) 597 return true; 598 } 599 600 if (isStyleSpan(copiedRangeStyleSpan.get()) && styleText == static_cast<Element*>(copiedRangeStyleSpan.get())->getAttribute(styleAttr)) { 601 fragment.removeNodePreservingChildren(copiedRangeStyleSpan.get()); 602 return true; 603 } We can't change these two style diffs either because the second diff must be done against the insertion position after sourceDocumentStyleSpan has been inserted. Resolving this bug properly may require fixing the bug 34564 first.
Ryosuke Niwa
Comment 3 2011-05-20 18:19:12 PDT
Created attachment 94303 [details] fixes the bug
WebKit Review Bot
Comment 4 2011-05-20 18:59:25 PDT
Comment on attachment 94303 [details] fixes the bug Attachment 94303 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8722324
WebKit Review Bot
Comment 5 2011-05-20 18:59:30 PDT
Created attachment 94305 [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
Comment 6 2011-05-20 20:48:52 PDT
Ugh... git is screwing me up. I'll upload new patch using svn.
Ryosuke Niwa
Comment 7 2011-05-20 21:14:47 PDT
Created attachment 94315 [details] patch made by svn
Enrica Casucci
Comment 8 2011-05-23 10:50:35 PDT
Comment on attachment 94315 [details] patch made by svn Looks good to me.
Ryosuke Niwa
Comment 9 2011-05-23 11:07:48 PDT
Committed r87082: <http://trac.webkit.org/changeset/87082>
Ademar Reis
Comment 10 2011-05-23 14:09:03 PDT
Revision r87082 cherry-picked into qtwebkit-2.2 with commit ca2dab5 <http://gitorious.org/webkit/qtwebkit/commit/ca2dab5>
Note You need to log in before you can comment on or make changes to this bug.