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.
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.
"\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.
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?
(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.
Created attachment 193006 [details] Patch
Created attachment 193011 [details] Patch
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.
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.
Created attachment 193167 [details] Patch
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.
Created attachment 193182 [details] Patch
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
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
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
Please investigate the failing tests.
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
Created attachment 193216 [details] Patch
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.
Created attachment 193224 [details] Patch
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());
Created attachment 193226 [details] Patch
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.
Created attachment 193321 [details] Patch
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.
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?
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.
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?
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.
Created attachment 193346 [details] Patch
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.
Created attachment 193370 [details] Patch
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.
Comment on attachment 193370 [details] Patch Clearing flags on attachment: 193370 Committed r145954: <http://trac.webkit.org/changeset/145954>
All reviewed patches have been landed. Closing bug.