Bug 60914 - REGRESSION(r84311): WebKit copies too much styles when copying
Summary: REGRESSION(r84311): WebKit copies too much styles when copying
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: NeedsRadar
Depends on: 60920 60923 60988
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-16 12:41 PDT by Ryosuke Niwa
Modified: 2011-05-23 14:09 PDT (History)
12 users (show)

See Also:


Attachments
work in progress (8.16 KB, patch)
2011-05-16 14:10 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixes the bug (23.44 KB, patch)
2011-05-20 18:19 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
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+
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-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.
Comment 1 Ryosuke Niwa 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.
Comment 2 Ryosuke Niwa 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.
Comment 3 Ryosuke Niwa 2011-05-20 18:19:12 PDT
Created attachment 94303 [details]
fixes the bug
Comment 4 WebKit Review Bot 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
Comment 5 WebKit Review Bot 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
Comment 6 Ryosuke Niwa 2011-05-20 20:48:52 PDT
Ugh... git is screwing me up.  I'll upload new patch using svn.
Comment 7 Ryosuke Niwa 2011-05-20 21:14:47 PDT
Created attachment 94315 [details]
patch made by svn
Comment 8 Enrica Casucci 2011-05-23 10:50:35 PDT
Comment on attachment 94315 [details]
patch made by svn

Looks good to me.
Comment 9 Ryosuke Niwa 2011-05-23 11:07:48 PDT
Committed r87082: <http://trac.webkit.org/changeset/87082>
Comment 10 Ademar Reis 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>