[Webkit-unassigned] [Bug 50564] End/Home keys do not work in a contentEditable element

Thu Apr 21 16:31:11 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=50564

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #90622|review?                     |review-
               Flag|                            |

--- Comment #14 from Ryosuke Niwa <rniwa at webkit.org>  2011-04-21 16:31:11 PST ---
(From update of attachment 90622)
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-contexts-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-contexts-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-contexts.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-editable-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-editable-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-editable-contexts.js:40
> +
> +    

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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.