Bug 70799 – The copy and paste result in nested scrollbars on http://dojotoolkit.org/widgets

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 112398 [details]
fixes the bug

[Enrica Casucci]

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.

[Ryosuke Niwa]

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

[WebKit Review Bot]

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

[Ryosuke Niwa]

Created attachment 112410 [details]
Added change log entry

[WebKit Review Bot]

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

[Ryosuke Niwa]

Ping reviewers.

[Enrica Casucci]

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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 112910 [details]
Updated per comment

[WebKit Review Bot]

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

[Enrica Casucci]

Comment on attachment 112910 [details]
Updated per comment

Looks good.

[Ryosuke Niwa]

Comment on attachment 112910 [details]
Updated per comment

Clearing flags on attachment: 112910

Committed r98794: <http://trac.webkit.org/changeset/98794>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.