[Webkit-unassigned] [Bug 112275] TextIterator emits LF for a br element inside an empty input element
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Mar 14 12:01:40 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=112275
Ryosuke Niwa <rniwa at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #193167|review? |review-
Flag| |
--- Comment #10 from Ryosuke Niwa <rniwa at webkit.org> 2013-03-14 12:04:05 PST ---
(From update of attachment 193167)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list