Bug 113240 – TextIterator emits LF for a br element inside an empty textarea element

• Component: Text
• Status: UNCONFIRMED

[Aurimas Liutikas]

TextIterator is emitting new line characters for <br> elements inside empty <textarea> elements causing issues in Chrome for Android text input.

[Aurimas Liutikas]

Created attachment 194921 [details]
Patch

[WebKit Review Bot]

Comment on attachment 194921 [details]
Patch

Attachment 194921 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17251530

New failing tests:
editing/text-iterator/basic-iteration.html

[WebKit Review Bot]

Created attachment 194937 [details]
Archive of layout-test-results from gce-cr-linux-07 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Aurimas Liutikas]

Created attachment 194939 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 194939 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:757
> -    return emitsOriginalText || !(node->isInShadowTree() && node->shadowHost()->toInputElement());
> +    return emitsOriginalText || !(node->isInShadowTree() && (node->shadowHost()->toInputElement() || isHTMLTextAreaElement(node->shadowHost())));

How does this work? This code change will make it so that we never emit new lines for BRs inside textarea.
We need to emit BRs between lines.

[Aurimas Liutikas]

Comment on attachment 194939 [details]
Patch

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

>> Source/WebCore/editing/TextIterator.cpp:757
>> +    return emitsOriginalText || !(node->isInShadowTree() && (node->shadowHost()->toInputElement() || isHTMLTextAreaElement(node->shadowHost())));
> 
> How does this work? This code change will make it so that we never emit new lines for BRs inside textarea.
> We need to emit BRs between lines.

In Chromium each new line seems to be a new text node. Is that not the case for Safari?

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 194939 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194939&action=review
> 
> >> Source/WebCore/editing/TextIterator.cpp:757
> >> +    return emitsOriginalText || !(node->isInShadowTree() && (node->shadowHost()->toInputElement() || isHTMLTextAreaElement(node->shadowHost())));
> > 
> > How does this work? This code change will make it so that we never emit new lines for BRs inside textarea.
> > We need to emit BRs between lines.
> 
> In Chromium each new line seems to be a new text node. Is that not the case for Safari?

That's not always true even in Chromium. I mean… that's the entire reason we have this bug, right?

[Ryosuke Niwa]

By the way, it'll be great if we can refactor the code in HTMLTextFormControlElement and HTMLTextAreaElement to use TextIterator while fixing this bug since they have a special logic to convert DOM to text, which is basically duplicating the functionality of TextIterator.

[Aurimas Liutikas]

Comment on attachment 194939 [details]
Patch

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

>>>> Source/WebCore/editing/TextIterator.cpp:757
>>>> +    return emitsOriginalText || !(node->isInShadowTree() && (node->shadowHost()->toInputElement() || isHTMLTextAreaElement(node->shadowHost())));
>>> 
>>> How does this work? This code change will make it so that we never emit new lines for BRs inside textarea.
>>> We need to emit BRs between lines.
>> 
>> In Chromium each new line seems to be a new text node. Is that not the case for Safari?
> 
> That's not always true even in Chromium. I mean… that's the entire reason we have this bug, right?

Every time I saw a <br> element in Chromium, it did not represent a new line and instead was just a place holder to prevent the <div> in the shadow DOM from collapsing (e.g. when you delete the last character). Do you have specific cases in mind where <br> should become a "\n"?

On the side note, I am really confused why <br> are used in the first place. It does not seem appropriate at all, it would be some much cleaner if we had empty text node inside the <div> in the shadow tree instead of a <br> that should not be emitting.

[Ryosuke Niwa]

(In reply to comment #9)
> On the side note, I am really confused why <br> are used in the first place. It does not seem appropriate at all, it would be some much cleaner if we had empty text node inside the <div> in the shadow tree instead of a <br> that should not be emitting.

That's not going to work. The div is just going to collapse without br.