Bug 112275 – TextIterator emits LF for a br element inside an empty input element

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Aurimas Liutikas]

The shadow tree for an input element that is empty is:
<div></div>
Once a user enters character "a", the shadow tree element becomes:
<div>"a"</div>
Once the user deletes that character, the shadow tree element becomes:
<div><br>""</div>

The call below to plainText returns "\n" after adding and deleting a character:

Node* node = document()->frame()->selection()->selection().rootEditableElement();       
plainText(rangeOfContents(node).get());

This is incorrect as the field is actually empty and <br> is there just to keep input element's <div> from collapsing.

[Ryosuke Niwa]

It's perfectly fine to emit LF for a br element inside a content editable element. In fact, that's expected. It's not okay to emit LF inside an empty input element however since the fact we use shadow DOM to implement input element is an implementation detail and input should never have a LF in it.

[Aurimas Liutikas]

"\n" is causing a bug in Chrome for Android when using Japanese IME. When we send the keyboard update that there is a \n character for field that is supposed to be only one line.

[Aurimas Liutikas]

rniwa: Should plainText() function in TextIterator.cpp simply compare each element it is iterating over to see if it is "<br>" and if it is - skip it?

[Ryosuke Niwa]

(In reply to comment #3)
> rniwa: Should plainText() function in TextIterator.cpp simply compare each element it is iterating over to see if it is "<br>" and if it is - skip it?

Definitely not. We simply need a logic to skip <br> inside the shadow DOM of an input element.

[Aurimas Liutikas]

Created attachment 193006 [details]
Patch

[Aurimas Liutikas]

Created attachment 193011 [details]
Patch

[Aurimas Liutikas]

Comment on attachment 193011 [details]
Patch

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

How can I test TextIterator? Is there a similar LayoutTest somewhere?

> Source/WebCore/editing/TextIterator.cpp:758
>      return r->isBR();

Should I do a similar check here using node? I am not familiar what RenderObject means.

[Ryosuke Niwa]

Comment on attachment 193011 [details]
Patch

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

Look at tests inside LayoutTests/editing/text-iterator.

> Source/WebCore/editing/TextIterator.cpp:756
> +        return node->hasTagName(brTag) && !node->isInShadowTree() && !node->shadowHost()->toInputElement();

This is wrong. Being inside a shadow tree doesn't imply that the shadow tree is inside an input element.

>> Source/WebCore/editing/TextIterator.cpp:758
>>      return r->isBR();
> 
> Should I do a similar check here using node? I am not familiar what RenderObject means.

Yes. Having a renderer means that the node nor its ancestors don't have display: none.

[Aurimas Liutikas]

Created attachment 193167 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 193167 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:753
>      // br elements are represented by a single newline.
>      RenderObject* r = node->renderer();

While we're at it, please rename r to renderer, and remove this comment that repeats the self-evident fact.

> Source/WebCore/editing/TextIterator.cpp:755
> +    // Checking if this node is a <br> element within a shadow tree of an input element.

This comment repeats the code. Please remove it.

> Source/WebCore/editing/TextIterator.cpp:756
> +    bool isNodeInsideInputShadowTree = node->isInShadowTree() && node->shadowHost()->toInputElement();

The boolean shouldn't be a question. Rename it to nodeIsInsideInputElement.

> LayoutTests/editing/text-iterator/basic-iteration-expected.txt:20
> +PASS range.selectNodeContents(shadow); internals.rangeAsText(range) is "b"
> +PASS range.selectNodeContents(shadow); internals.rangeAsText(range) is "b"
> +PASS range.selectNodeContents(shadow); internals.rangeAsText(range) is ""
> +PASS range.selectNodeContents(shadow); internals.rangeAsText(range) is "\n"

From this output, it's not obvious why internals.rangeAsText's return value should be different in all four cases.

> LayoutTests/editing/text-iterator/script-tests/basic-iteration.js:35
> +var input = testDocument.body.children[0];

I would prefer doing testDocument.querySelector('input') instead.

> LayoutTests/editing/text-iterator/script-tests/basic-iteration.js:37
> +var shadow = internals.oldestShadowRoot(input);
> +shouldBe('range.selectNodeContents(shadow); internals.rangeAsText(range)', '"b"');

It would have been better if we called internals.oldestShadowRoot(input) inside shouldBe.

> LayoutTests/editing/text-iterator/script-tests/basic-iteration.js:41
> +var mydiv = shadow.childNodes[0];
> +var mybr = document.createElement('br');
> +mydiv.appendChild(mybr);

Please don't use variables names like mydiv and mybr.
Also, it would have been better if we added a helper function to create and append a br element and then called that inside shouldBe.

> LayoutTests/editing/text-iterator/script-tests/basic-iteration.js:46
> +testDocument.body.innerHTML = '<div>a</div>';
> +var maindiv = testDocument.body.childNodes[0];
> +var shadow = maindiv.webkitCreateShadowRoot();

We should create some helper function to do this and call it inside shouldBe.

> LayoutTests/editing/text-iterator/script-tests/basic-iteration.js:49
> +shadow.appendChild(document.createElement('br'));

This should be called inside shouldBe.

[Aurimas Liutikas]

Created attachment 193182 [details]
Patch

[Aurimas Liutikas]

Comment on attachment 193167 [details]
Patch

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

>> Source/WebCore/editing/TextIterator.cpp:753
>>      RenderObject* r = node->renderer();
> 
> While we're at it, please rename r to renderer, and remove this comment that repeats the self-evident fact.

Done

>> Source/WebCore/editing/TextIterator.cpp:755
>> +    // Checking if this node is a <br> element within a shadow tree of an input element.
> 
> This comment repeats the code. Please remove it.

Done

>> Source/WebCore/editing/TextIterator.cpp:756
>> +    bool isNodeInsideInputShadowTree = node->isInShadowTree() && node->shadowHost()->toInputElement();
> 
> The boolean shouldn't be a question. Rename it to nodeIsInsideInputElement.

Done

>> LayoutTests/editing/text-iterator/script-tests/basic-iteration.js:35
>> +var input = testDocument.body.children[0];
> 
> I would prefer doing testDocument.querySelector('input') instead.

Done

>> LayoutTests/editing/text-iterator/script-tests/basic-iteration.js:37
>> +shouldBe('range.selectNodeContents(shadow); internals.rangeAsText(range)', '"b"');
> 
> It would have been better if we called internals.oldestShadowRoot(input) inside shouldBe.

Done

>> LayoutTests/editing/text-iterator/script-tests/basic-iteration.js:41
>> +mydiv.appendChild(mybr);
> 
> Please don't use variables names like mydiv and mybr.
> Also, it would have been better if we added a helper function to create and append a br element and then called that inside shouldBe.

Done

>> LayoutTests/editing/text-iterator/script-tests/basic-iteration.js:46
>> +var shadow = maindiv.webkitCreateShadowRoot();
> 
> We should create some helper function to do this and call it inside shouldBe.

Done

>> LayoutTests/editing/text-iterator/script-tests/basic-iteration.js:49
>> +shadow.appendChild(document.createElement('br'));
> 
> This should be called inside shouldBe.

Done

[Build Bot]

Comment on attachment 193182 [details]
Patch

Attachment 193182 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17157148

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

[WebKit Review Bot]

Comment on attachment 193182 [details]
Patch

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

New failing tests:
fast/forms/8250.html

[Ryosuke Niwa]

Please investigate the failing tests.

[Build Bot]

Comment on attachment 193182 [details]
Patch

Attachment 193182 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17222046

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

[Aurimas Liutikas]

Created attachment 193216 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 193216 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:1139
> +    , m_emitsOriginalText(behavior & TextIteratorEmitsOriginalText)

SimplifiedBackwardsTextIterator doesn't support TextIteratorEmitsOriginalText. We need to initialize it with false.
r- because of this.

[Aurimas Liutikas]

Created attachment 193224 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 193224 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:754
> +    bool nodeIsInsideInputElement = node->isInShadowTree() && node->shadowHost()->toInputElement();

It seems not ideal that we check this condition first because the vast majority of tests aren't br.
Given that it's probably better to check br'ness first as in:
if ((!renderer || !renderer->isBR()) && !node->hasTagName(brTag))
    return false;
return emitsOriginalText || !(node->isInShadowTree() && node->shadowHost()->toInputElement());

[Aurimas Liutikas]

Created attachment 193226 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 193226 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:755
> +    if ((!renderer || !renderer->isBR()) && !node->hasTagName(brTag))
> +        return false;

Oh oops, sorry I lied. This is not identical :( This behaves differently than the old code when renderer->isBR() is false and node->hasTagName(brTag) is true.
We need to do renderer ? !renderer->isBR() : !node->hasTagName(brTag) instead.

[Aurimas Liutikas]

Created attachment 193321 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 193321 [details]
Patch

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

> LayoutTests/editing/text-iterator/basic-iteration-shadowdom.html:44
> +testDocument.body.innerHTML = '<div>a</div>';
> +var div = testDocument.body.childNodes[0];
> +shouldBe('addShadowTreeWithDivElement(div); range.selectNodeContents(internals.oldestShadowRoot(div)); internals.rangeAsText(range)', '"b"');
> +
> +shouldBe('appendBrElement(internals.oldestShadowRoot(div).childNodes[0]); range.selectNodeContents(internals.oldestShadowRoot(div)); internals.rangeAsText(range)', '"b\\n"');
> +

We should split this into a separate test so that test cases above can still be ran on ports that don't enable shadow DOM.

[Aurimas Liutikas]

Comment on attachment 193321 [details]
Patch

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

>> LayoutTests/editing/text-iterator/basic-iteration-shadowdom.html:44
>> +
> 
> We should split this into a separate test so that test cases above can still be ran on ports that don't enable shadow DOM.

Which ones of the four cases above do you think can be run without shadow DOM?

[Ryosuke Niwa]

Comment on attachment 193321 [details]
Patch

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

>>> LayoutTests/editing/text-iterator/basic-iteration-shadowdom.html:44
>>> +
>> 
>> We should split this into a separate test so that test cases above can still be ran on ports that don't enable shadow DOM.
> 
> Which ones of the four cases above do you think can be run without shadow DOM?

Everything up to line 40 should work everywhere.

[Aurimas Liutikas]

Comment on attachment 193321 [details]
Patch

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

>>>> LayoutTests/editing/text-iterator/basic-iteration-shadowdom.html:44
>>>> +
>>> 
>>> We should split this into a separate test so that test cases above can still be ran on ports that don't enable shadow DOM.
>> 
>> Which ones of the four cases above do you think can be run without shadow DOM?
> 
> Everything up to line 40 should work everywhere.

Doesn't internals.oldestShadowRoot(input) require shadow DOM support?

[Ryosuke Niwa]

Comment on attachment 193321 [details]
Patch

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

>>>>> LayoutTests/editing/text-iterator/basic-iteration-shadowdom.html:44
>>>>> +
>>>> 
>>>> We should split this into a separate test so that test cases above can still be ran on ports that don't enable shadow DOM.
>>> 
>>> Which ones of the four cases above do you think can be run without shadow DOM?
>> 
>> Everything up to line 40 should work everywhere.
> 
> Doesn't internals.oldestShadowRoot(input) require shadow DOM support?

No.

[Aurimas Liutikas]

Created attachment 193346 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 193346 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Test: editing/text-iterator/basic-iteration-shadowdom.html

Please add editing/text-iterator/script-tests/basic-iteration.html here as well.

[Aurimas Liutikas]

Created attachment 193370 [details]
Patch

[Ryosuke Niwa]

You can ask for r? and cq? at the same time. Also, once you've got r+, you can replace NOBODY (OOPS!) by my name and upload the patch with just cq?. You can do that easily with webkit-patch upload --no-review --request-commit.

[WebKit Review Bot]

Comment on attachment 193370 [details]
Patch

Clearing flags on attachment: 193370

Committed r145954: <http://trac.webkit.org/changeset/145954>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.