[webkit-reviews] review denied: [Bug 50564] End/Home keys do not work in a contentEditable element : [Attachment 90622] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
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.
More information about the webkit-reviews
mailing list