Bug 79305

Summary: REGRESSION(r99076): WebKit pastes the trailing newline into a single-line text field
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, enrica, leviw, pkasting, tkent, tony
Priority: P2 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
demo
none
Fixes the bug tony: review+

Description Ryosuke Niwa 2012-02-22 16:53:11 PST
Created attachment 128334 [details]
demo

Reproduction steps:
1. Open the attachment
2. Select all in textarea
3. Copy
4. Paste in input

Expected result:
See "abc def " in the text field

Actual result:
Get "abc def\n" in the text field
Comment 2 Ryosuke Niwa 2012-02-22 17:37:10 PST
Created attachment 128348 [details]
Fixes the bug
Comment 3 Tony Chang 2012-02-23 09:58:08 PST
Comment on attachment 128348 [details]
Fixes the bug

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:188
> +        // New lines may have been pruned by text field's before text inserted event.
> +        m_hasInterchangeNewlineAtStart = false;
> +        m_hasInterchangeNewlineAtEnd = false;

It looks like these can be set to true by removeInterchangeNodes().  Do we need to reset these values after the call to removeInterchangeNodes() 2 lines down?  I guess it's not obvious why we set the values at this spot.

> LayoutTests/resources/dump-as-markup.js:242
> +Markup._getShadowHostIfPossible = function (node, depth)

Nit: Remove the space between function and open parenthesis.
Comment 4 Ryosuke Niwa 2012-02-23 10:16:51 PST
Comment on attachment 128348 [details]
Fixes the bug

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

Thanks for the review.

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:188
>> +        m_hasInterchangeNewlineAtEnd = false;
> 
> It looks like these can be set to true by removeInterchangeNodes().  Do we need to reset these values after the call to removeInterchangeNodes() 2 lines down?  I guess it's not obvious why we set the values at this spot.

No. The point is that removeInterchangeNodes sets these variables to true but never resets to false when they should.

Okay, on my second thought, these resets should happen inside removeInterchangeNodes. Will move the assignments before lading the patch.

>> LayoutTests/resources/dump-as-markup.js:242
>> +Markup._getShadowHostIfPossible = function (node, depth)
> 
> Nit: Remove the space between function and open parenthesis.

Will do.
Comment 5 Ryosuke Niwa 2012-02-23 13:16:57 PST
Committed r108668: <http://trac.webkit.org/changeset/108668>
Comment 6 Peter Kasting 2012-03-05 12:37:11 PST
*** Bug 78966 has been marked as a duplicate of this bug. ***