[webkit-reviews] review denied: [Bug 50564] End/Home keys do not work in a contentEditable element : [Attachment 90622] Patch

Thu Apr 21 16:31:11 PDT 2011

Ryosuke Niwa <rniwa at webkit.org> has denied Levi Weintraub
<leviw at chromium.org>'s request for review:
Bug 50564: End/Home keys do not work in a contentEditable element
https://bugs.webkit.org/show_bug.cgi?id=50564

Attachment 90622: Patch
https://bugs.webkit.org/attachment.cgi?id=90622&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=90622&action=review

r- because you probably want to address at least one of the things I listed
below

> Source/WebCore/editing/visible_units.cpp:1160
> +	   if (editableRoot &&
!editableRoot->contains(visPos.deepEquivalent().containerNode()))

You don't have to check editableRoot for the second time here.	Can we extract
all of this as an inline function (maybe highestEditableRootContains)?

>
LayoutTests/editing/selection/modify-by-lineboundary-in-inline-editable-context
s-expected.txt:7
> +PASS Modify moving forward in adjactent, editable spans
> +PASS Modify moving backward in adjactent, editable spans

Typo: adjactent

>
LayoutTests/editing/selection/modify-by-lineboundary-in-inline-editable-context
s-expected.txt:14
> +adjacent editablespans
> +This is a non-editable paragraph with an editable span inside it.

It'll be nice to get rid of these before the test ends.

>
LayoutTests/editing/selection/modify-by-lineboundary-in-inline-editable-context
s.html:2
> +<!DOCTYPE html>
> +

Did you use the script to generate this file?  If not, you should.

>
LayoutTests/editing/selection/script-tests/modify-by-lineboundary-in-inline-edi
table-contexts.js:26
> +var name = "adjactent, editable spans";

Did you mean adjacent?	I would have called them consecutive spans but it's up
to your decision.

>
LayoutTests/editing/selection/script-tests/modify-by-lineboundary-in-inline-edi
table-contexts.js:29
> +var root = document.getElementById('root1');
> +testModifyByLine(root, "forward", 17, name);
> +testModifyByLine(root, "backward", 0, name);

I would have done:
testModifyByLine(container.firstChild, "forward", 17, name);
testModifyByLine(container.firstChild, "backward", 0, name);

>
LayoutTests/editing/selection/script-tests/modify-by-lineboundary-in-inline-edi
table-contexts.js:40
> +
> +    

You can do container.innerHTML = ''; here.