Bug 64423 – Line-breaks are disappeared from textarea.value if user pastes a text into a no-wrap-ed textarea.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Hajime Morrita]

Originally reported at http://crbug.com/88736.
A fix will come shortly.

[Hajime Morrita]

Created attachment 100608 [details]
Patch

[Kent Tamura]

(In reply to comment #1)
> Created an attachment (id=100608) [details]
> Patch

I think your patch will fix the problem. But can we avoid to insert <div> or something into the inner text of <textarea> and <input>?

[Ryosuke Niwa]

Comment on attachment 100608 [details]
Patch

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

> LayoutTests/editing/pasteboard/paste-textarea-wrap.html:36
> +description("This tests if pasted text preserves original line-breaks regardless their destination styles.");

Nit: regardless *of* their destination styles.

[Ryosuke Niwa]

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=100608) [details] [details]
> > Patch
> 
> I think your patch will fix the problem. But can we avoid to insert <div> or something into the inner text of <textarea> and <input>?

Yeah, we specifically avoid inserting div into the shadow DOM in various places.  We should figure out where this div is coming from and replace them with br's.

[Hajime Morrita]

> > I think your patch will fix the problem. But can we avoid to insert <div> or something into the inner text of <textarea> and <input>?
> 
> Yeah, we specifically avoid inserting div into the shadow DOM in various places.  We should figure out where this div is coming from and replace them with br's.

Kent-san, Ryosuke, thanks for taking a look!
I'm not sure what should happen there. So I'll investigate how text insertion works at first.

[Hajime Morrita]

Created attachment 102122 [details]
Patch

[Hajime Morrita]

> > I think your patch will fix the problem. But can we avoid to insert <div> or something into the inner text of <textarea> and <input>?
> 
> Yeah, we specifically avoid inserting div into the shadow DOM in various places.  We should figure out where this div is coming from and replace them with br's.

I updated the patch to use <br>.
Niwa-san, could you  take a look?

[Ryosuke Niwa]

Comment on attachment 102122 [details]
Patch

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

> Source/WebCore/editing/markup.cpp:772
> +                element = createHTMLElement(document, spanTag);
> +                fillContainerFromString(element.get(), s);
> +                fragment->appendChild(element.release(), ASSERT_NO_EXCEPTION);
> +                element = createBreakElement(document);
> +                fragment->appendChild(element.release(), ASSERT_NO_EXCEPTION);

This seems like a wrong layer to fix this.  I'd suspect where we need to fix is ReplaceSelectionCommand.

[Hajime Morrita]

Comment on attachment 102122 [details]
Patch

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

>> Source/WebCore/editing/markup.cpp:772
>> +                fragment->appendChild(element.release(), ASSERT_NO_EXCEPTION);
> 
> This seems like a wrong layer to fix this.  I'd suspect where we need to fix is ReplaceSelectionCommand.

I don't think so.
ReplaceSelectionCommand just emits a textInput event,
And Editor handles it, then calls this method.
Actually this createFragmentFromText() has similar trick at the beginning of the function for preserveNewLine().

Hopefully we have FragmentBuilder to make these code more organized, like MarkupAccumulator does.
But that's another story.

[Hajime Morrita]

Comment on attachment 102122 [details]
Patch

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

>>> Source/WebCore/editing/markup.cpp:772
>>> +                fragment->appendChild(element.release(), ASSERT_NO_EXCEPTION);
>> 
>> This seems like a wrong layer to fix this.  I'd suspect where we need to fix is ReplaceSelectionCommand.
> 
> I don't think so.
> ReplaceSelectionCommand just emits a textInput event,
> And Editor handles it, then calls this method.
> Actually this createFragmentFromText() has similar trick at the beginning of the function for preserveNewLine().
> 
> Hopefully we have FragmentBuilder to make these code more organized, like MarkupAccumulator does.
> But that's another story.

Actually, the fragment is build _before_ ReplaceSelectionCommand_ is instantiated, 
The created fragment is passed to ReplaceSelectionCommand constructor instead.
So there is nothing to do for ReplaceSelectionCommand.

[Ryosuke Niwa]

Comment on attachment 102122 [details]
Patch

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

>>>> Source/WebCore/editing/markup.cpp:772
>>>> +                fragment->appendChild(element.release(), ASSERT_NO_EXCEPTION);
>>> 
>>> This seems like a wrong layer to fix this.  I'd suspect where we need to fix is ReplaceSelectionCommand.
>> 
>> I don't think so.
>> ReplaceSelectionCommand just emits a textInput event,
>> And Editor handles it, then calls this method.
>> Actually this createFragmentFromText() has similar trick at the beginning of the function for preserveNewLine().
>> 
>> Hopefully we have FragmentBuilder to make these code more organized, like MarkupAccumulator does.
>> But that's another story.
> 
> Actually, the fragment is build _before_ ReplaceSelectionCommand_ is instantiated, 
> The created fragment is passed to ReplaceSelectionCommand constructor instead.
> So there is nothing to do for ReplaceSelectionCommand.

What I'm saying that createFragment changing behavior depending on whether we're inside a shadow DOM or not seems wrong.

[Ryosuke Niwa]

For example, are you sure that this bug doesn't reproduce in a contenteditable area that has the exactly same CSS style as the one in the shadow DOM of textarea?

[Hajime Morrita]

Comment on attachment 102122 [details]
Patch

Clearing r? for now.

[Ryosuke Niwa]

Oops, I had totally forgot about this bug and made a very similar change in http://trac.webkit.org/changeset/102392. Sorry about that :(