Bug 39801 – REGRESSION (r45962): Pasted newlines are stripped on form submit at paste.nerv.fi

• Component: Forms
• Status: NEW

[Ojan Vafai]

1. Paste text with newlines.
2. Press the "Laheta" button"

The newlines get stripped. If you typing in text with newlines, they don't get stripped.

Tested on WebKit nightly Mac and Chrome 6.0.408.1 (Official Build 47574) dev.

[Alexey Proskuryakov]

Works in Safari/WebKit 4.0.5, fails in r60308.

[Alexey Proskuryakov]

<rdar://problem/8040778>

[Ojan Vafai]

Regression range: 45980:46042

[Alexey Proskuryakov]

My testing says it's r45962.

[Ojan Vafai]

Got into this in a debugger. The problem is that we now propagate the white-space:no-wrap. On paste, we use divs for line-breaks instead of BRs if white-space:no-wrap. If white-space:pre-wrap, then we insert BRs.

There's a simple fix to RenderTextControl::text to detect divs and insert newlines. The deeper fix would be to figure out why we insert divs instead of BRs.

I'm surprised we don't have the same bug with wrap=OFF though. With wrap=OFF, we still insert newlines on paste.

[Ojan Vafai]

What's curious is that if I paste the same thing into a contentEditable=plaintext-only div, I get a BR for the newline, not divs.

[Ojan Vafai]

Oh, nevermind, I was setting whitespace, not white-space. contentEditable=plaintext-only has the same bug. Paste inserts divs if white-space:no-wrap instead of BRs.

[Ojan Vafai]

OK. The behavior is correct for white-space:no-wrap. In createFragmentFromText we check if the node preserves newlines. If it does, then we use BRs, otherwise we use divs. The bug is that the text control's innerBlock has white-space:no-wrap instead of white-space:pre. I don't quite get why that's the case though. Looking at the code, it looks like it should be white-space:pre.

[Ojan Vafai]

Oh, duh. We let the white-space property inherit now. Instead, we should make sure that white-space:no-wrap translates to white-space:pre, perhaps in RenderTextControl::updateFromElement?

[Ojan Vafai]

Created attachment 57802 [details]
Patch

[Shinichiro Hamaji]

Created attachment 57848 [details]
attempt to preserve newlines

[Shinichiro Hamaji]

Sorry for the latency and thanks for investigations.

It's unfortunate we break fast/forms/basic-textareas with Ojan's patch. This means the hixie's test will fail again, right?

http://www.hixie.ch/tests/evil/mixed/wraptextarea.html

I've uploaded a patch with another approach and I'm guessing this approach is better for this issue, but I'm not sure. I should admit I'm not good at editing stuff.

With my patch, RenderTextControl::text() and RenderTextControl::textWithHardLineBreaks() inserts newlines when we find </div>. In this way we can preserve newlines when we submit a form.

We might be able to fix this issue by modifying createFragmentFromText so it doesn't emit <div> anymore and uses <br> instead. I didn't choose this way because I guessed we may emit <div> in future for another purpose.

By the way, I've noticed a similar issue when I'm creating this patch. If we input "foo   bar" into nowrap textarea and submit it, the expected query string is "foo+++bar" but the actual value was "foo+%A0+bar" (%A0 is NBSP). I think we can easily fix this issue by translating a NBSP into a space in RenderTextControl::text(). If we decide to go with my approach, I'll file another bug and fix it soon later.

Ojan, what do you think? Are there any big issues with my approach? Thanks!

[Ojan Vafai]

Comment on attachment 57848 [details]
attempt to preserve newlines

Overall, I like this approach better. Just a couple questions. Not r-'ing since I just don't know if this is correct.

WebCore/rendering/RenderTextControl.cpp:331
 +          if (n && n->previousSibling() && n->previousSibling()->hasTagName(divTag))
I don't understand the code that generates the DOM inside the text control well enough to know if this if-statement is sufficient. For example, can you get nested div tags through pasting? Also, can you get a BR as the last node inside the previousSibling div tag? If so, we'd get multiple newlines in both cases where the user would only see one.

WebCore/rendering/RenderTextControl.cpp:357
 +      if (node->hasTagName(divTag)) {
Why do we need to walk inside the first div tag? Not saying it's wrong, I just don't understand. :)

[Ojan Vafai]

(In reply to comment #12)
> We might be able to fix this issue by modifying createFragmentFromText so it doesn't emit <div> anymore and uses <br> instead. I didn't choose this way because I guessed we may emit <div> in future for another purpose.

I think I would prefer this approach for plain text controls. For rich-text, we'd want to maintain the current behavior.

I would like it even more if we added an ASSERT to text and textWithHardLineBreaks that the text control has no divs in it. It will keep programming for text controls much simpler if we can assume text controls only contain text nodes and BRs.

It's hard to say without seeing the code it would take to make this work though.

[Shinichiro Hamaji]

Thanks for comments, Ojan!

(In reply to comment #14)
> (In reply to comment #12)
> > We might be able to fix this issue by modifying createFragmentFromText so it doesn't emit <div> anymore and uses <br> instead. I didn't choose this way because I guessed we may emit <div> in future for another purpose.
> 
> I think I would prefer this approach for plain text controls. For rich-text, we'd want to maintain the current behavior.
> 
> I would like it even more if we added an ASSERT to text and textWithHardLineBreaks that the text control has no divs in it. It will keep programming for text controls much simpler if we can assume text controls only contain text nodes and BRs.

OK, it seems we were actually using <br> long time ago and Darin made a change so we are using <div> now.

http://trac.webkit.org/changeset/8229

I couldn't see the reason of the change, but I'm sure there were some reasons :) Darin, could you tell us why you made this change?

[Ojan Vafai]

(In reply to comment #15)
> OK, it seems we were actually using <br> long time ago and Darin made a change so we are using <div> now.
> 
> http://trac.webkit.org/changeset/8229
> 
> I couldn't see the reason of the change, but I'm sure there were some reasons :) Darin, could you tell us why you made this change?

This is a little different. I think createFragmentFromText is used for creating a DOM from a plain-text string. It's not for creating a DOM within a plain-text element.

I think we might want to do something similar to what the code did before r8229 though. The difference is that we want to do it if we're creating a DOM for a plain-text context. We kind of do this already for the preserveNewline cases at http://trac.webkit.org/browser/trunk/WebCore/editing/markup.cpp#L1170.

My intuition is that we should add another case. If we're in a contentEditable=plaintext-only element (i.e. renderer->style->userModify == READ_WRITE_PLAINTEXT_ONLY), then the newlines should all be converted to BRs and then we return that DOM. That should fix textareas and, more generally, plaintext contentEditable regions.

[Ojan Vafai]

Comment on attachment 57848 [details]
attempt to preserve newlines

r- per my previous comment.

[Alexey Proskuryakov]

What's the status of this bug?