Bug 70799 - The copy and paste result in nested scrollbars on http://dojotoolkit.org/widgets
Summary: The copy and paste result in nested scrollbars on http://dojotoolkit.org/widgets
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Major
Assignee: Ryosuke Niwa
URL: http://dojotoolkit.org/widgets
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-24 23:11 PDT by Ryosuke Niwa
Modified: 2011-10-28 22:27 PDT (History)
11 users (show)

See Also:


Attachments
fixes the bug (18.62 KB, patch)
2011-10-25 14:04 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added change log entry (19.55 KB, patch)
2011-10-25 14:54 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated per comment (20.01 KB, patch)
2011-10-28 14:23 PDT, Ryosuke Niwa
no flags 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-10-24 23:11:41 PDT
Reproductions steps (copied from http://code.google.com/p/chromium/issues/detail?id=97104):
1.Go to http://dojotoolkit.org/widgets
2.Click the Editor tab.
3.In the Enabled editor, add multiple lines to the end of the content so the scrollbar shows.
4.Press Ctrl+A to select all contents and then Ctrl+C and copy them into the clipboard.
5.Press Ctrl+V.

http://crbug.com/97104
Comment 1 Ryosuke Niwa 2011-10-25 14:04:35 PDT
Created attachment 112398 [details]
fixes the bug
Comment 2 Enrica Casucci 2011-10-25 14:13:54 PDT
Comment on attachment 112398 [details]
fixes the bug

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

> Source/WebCore/ChangeLog:5
> +

It would be nice to have a description of the issue and the relevant fix.
It is not clear at all from the bug what is happening. Could you please provide more details? The patch seems to be adding handling of the CSSPropertyTextDecoration property, but I don't understand at all how this is related to the bug.
Comment 3 Ryosuke Niwa 2011-10-25 14:35:13 PDT
(In reply to comment #2)
> (From update of attachment 112398 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112398&action=review
> 
> > Source/WebCore/ChangeLog:5
> > +
> 
> It would be nice to have a description of the issue and the relevant fix.
> It is not clear at all from the bug what is happening. Could you please provide more details? The patch seems to be adding handling of the CSSPropertyTextDecoration property, but I don't understand at all how this is related to the bug.

Oops, apparently I forgot to add that. Will do in a minute.
Comment 4 WebKit Review Bot 2011-10-25 14:51:28 PDT
Comment on attachment 112398 [details]
fixes the bug

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

New failing tests:
editing/pasteboard/5134759.html
Comment 5 Ryosuke Niwa 2011-10-25 14:54:36 PDT
Created attachment 112410 [details]
Added change log entry
Comment 6 WebKit Review Bot 2011-10-25 15:33:43 PDT
Comment on attachment 112410 [details]
Added change log entry

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

New failing tests:
editing/pasteboard/5134759.html
Comment 7 Ryosuke Niwa 2011-10-26 11:40:49 PDT
Ping reviewers.
Comment 8 Enrica Casucci 2011-10-26 11:48:42 PDT
Comment on attachment 112410 [details]
Added change log entry

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

> Source/WebCore/editing/markup.cpp:188
> +    ASSERT(propertyMissingOrEqualToNone(style, CSSPropertyWebkitTextDecorationsInEffect));

It would be nice to have a comment about the new assert.

> Source/WebCore/editing/markup.cpp:-526
> -    return style && style->style() && !propertyMissingOrEqualToNone(style->style(), CSSPropertyTextDecoration);

This has been removed because we don't expect to have this property anymore, correct?

> LayoutTests/editing/pasteboard/19644-2-expected.txt:1
> +<span style="background-color: rgb(187, 187, 187); ">This tests to make sure that we wrap copied markup in a div to hold a fully selected body's attributes and style when the that body has a background-color. If you copy and paste this text into Mail, it should have a grey background.</span>

Why is having a span now equivalent to the div before?

> LayoutTests/platform/mac/editing/pasteboard/5134759-expected.txt:21
> +              text run at (39,0) width 45: "World!"

similar question: why is it ok not to have a div anymore?
Comment 9 Ryosuke Niwa 2011-10-26 12:11:47 PDT
Comment on attachment 112410 [details]
Added change log entry

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

>> Source/WebCore/editing/markup.cpp:188
>> +    ASSERT(propertyMissingOrEqualToNone(style, CSSPropertyWebkitTextDecorationsInEffect));
> 
> It would be nice to have a comment about the new assert.

How about "wrappingStyleForSerialization should have removed -webkit-text-decorations-in-effect"?

>> Source/WebCore/editing/markup.cpp:-526
>> -    return style && style->style() && !propertyMissingOrEqualToNone(style->style(), CSSPropertyTextDecoration);
> 
> This has been removed because we don't expect to have this property anymore, correct?

No, we expect it to be there but we'll include the effective value in wrappingStyleForSerialization instead of cloning all ancestors up to an element with text-decoration property.
Comment 10 Ryosuke Niwa 2011-10-26 13:15:42 PDT
Comment on attachment 112410 [details]
Added change log entry

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

>> LayoutTests/editing/pasteboard/19644-2-expected.txt:1
>> +<span style="background-color: rgb(187, 187, 187); ">This tests to make sure that we wrap copied markup in a div to hold a fully selected body's attributes and style when the that body has a background-color. If you copy and paste this text into Mail, it should have a grey background.</span>
> 
> Why is having a span now equivalent to the div before?

It's okay because this is the only content in body. This is a bit tricky case because div will change the background color of the entire block as supposed to span which changes the background color of the inline text. Even div  doesn't occupy the entire body so it'll have a different background color below the div anyway.

Also, I think most of users expect text color to be applied as inline style as done in TextEdit, Microsoft Word, etc..., not as a block style.

>> LayoutTests/platform/mac/editing/pasteboard/5134759-expected.txt:21
>> +              text run at (39,0) width 45: "World!"
> 
> similar question: why is it ok not to have a div anymore?

This was a redundant div. I guess I can clarify that in the change log. Or convert this to a dump-as-markup first so that the rebaseline is clearer.
Comment 11 Ryosuke Niwa 2011-10-28 14:23:45 PDT
Created attachment 112910 [details]
Updated per comment
Comment 12 WebKit Review Bot 2011-10-28 14:58:48 PDT
Comment on attachment 112910 [details]
Updated per comment

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

New failing tests:
editing/pasteboard/5134759.html
Comment 13 Enrica Casucci 2011-10-28 16:27:08 PDT
Comment on attachment 112910 [details]
Updated per comment

Looks good.
Comment 14 Ryosuke Niwa 2011-10-28 22:27:12 PDT
Comment on attachment 112910 [details]
Updated per comment

Clearing flags on attachment: 112910

Committed r98794: <http://trac.webkit.org/changeset/98794>
Comment 15 Ryosuke Niwa 2011-10-28 22:27:18 PDT
All reviewed patches have been landed.  Closing bug.