Bug 50564 - End/Home keys do not work in a contentEditable element
Summary: End/Home keys do not work in a contentEditable element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 55565
  Show dependency treegraph
 
Reported: 2010-12-06 05:48 PST by Alexander Pavlov (apavlov)
Modified: 2011-04-22 16:18 PDT (History)
9 users (show)

See Also:


Attachments
Reduced test case (610 bytes, text/html)
2010-12-06 05:48 PST, Alexander Pavlov (apavlov)
no flags Details
[HTML] A more specific reduced test case (101 bytes, text/html)
2011-03-03 06:38 PST, Alexander Pavlov (apavlov)
no flags Details
Patch (6.46 KB, patch)
2011-04-21 14:27 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (6.74 KB, patch)
2011-04-21 15:08 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (7.49 KB, patch)
2011-04-21 16:01 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (6.33 KB, patch)
2011-04-21 17:09 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (7.58 KB, patch)
2011-04-22 13:38 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2010-12-06 05:48:48 PST
Created attachment 75683 [details]
Reduced test case

Open the attached test case, click inside the editable field (TTTeeesssttt) for the text cursor to appear, and hit "End". The cursor will NOT move towards the end of the element text. Ctrl-End works for me. "Home" works fine for bringing the cursor to the beginning of the line.

It seems there are some problems with the target range calculation.
Comment 1 Alexander Pavlov (apavlov) 2010-12-09 06:18:58 PST
This bug manifests itself when editing CSS properties in Web Inspector with the bug 50565 patch applied (to be landed shortly), so it would be nice to have no UX issues there...
Comment 2 Alexander Pavlov (apavlov) 2011-01-28 09:16:19 PST
If the "Value" SPAN (from the test case) is given the "editing" class, neither "Home", nor "End" work inside the "Value" text. Ctrl+Home and Ctrl+End work as expected.

This dramatically breaks the Web Inspector CSS editing experience.
Comment 3 Alexander Pavlov (apavlov) 2011-02-27 03:21:20 PST
Downstream Chromium issue: http://code.google.com/p/chromium/issues/detail?id=74304
Comment 4 Alexander Pavlov (apavlov) 2011-03-03 06:38:08 PST
Created attachment 84553 [details]
[HTML] A more specific reduced test case
Comment 5 Levi Weintraub 2011-04-21 14:27:12 PDT
Created attachment 90601 [details]
Patch
Comment 6 Ryosuke Niwa 2011-04-21 14:35:04 PDT
Comment on attachment 90601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90601&action=review

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

You can do: editableRoot  && !editableRoot->contains(visPos.deepEquivalent().containerNode()).

But shouldn't we do this inside honorEditableBoundaryAtOrBefore/honorEditableBoundaryAtOrAfter?
Comment 7 Levi Weintraub 2011-04-21 14:42:30 PDT
(In reply to comment #6)
> But shouldn't we do this inside honorEditableBoundaryAtOrBefore/honorEditableBoundaryAtOrAfter?

honorEditableBoundary___ is used in various places throughout the code and the contract now is that it'll return an empty VisiblePosition if the position it's supposed to change to honor the boundary isn't actually inside the highest root: "Return empty position if pos is not somewhere inside the editable region containing this position"

Changing this breaks other call sites. The other option is to add a paramater to those functions to support this different expectation.
Comment 8 Enrica Casucci 2011-04-21 14:43:18 PDT
Comment on attachment 90601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90601&action=review

> LayoutTests/editing/selection/modify-by-lineboundary-in-inline-editable-contexts.html:14
> +</html>

In general, I like to see tests, specially for editing, that can be run manually, with instructions on how to do so.
Comment 9 Enrica Casucci 2011-04-21 14:45:41 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > But shouldn't we do this inside honorEditableBoundaryAtOrBefore/honorEditableBoundaryAtOrAfter?
> 
> honorEditableBoundary___ is used in various places throughout the code and the contract now is that it'll return an empty VisiblePosition if the position it's supposed to change to honor the boundary isn't actually inside the highest root: "Return empty position if pos is not somewhere inside the editable region containing this position"
> 
> Changing this breaks other call sites. The other option is to add a paramater to those functions to support this different expectation.

I think this makes the code more confusing. I'd rather have it in your initial version, with the use of the contains method the Ryosuke suggested.
Comment 10 Levi Weintraub 2011-04-21 14:46:59 PDT
(In reply to comment #9)
> I think this makes the code more confusing. I'd rather have it in your initial version, with the use of the contains method the Ryosuke suggested.

I agree. I'll make that change and add more description to the test, as it can, in its current state, be ran manually.
Comment 11 Levi Weintraub 2011-04-21 15:08:52 PDT
Created attachment 90613 [details]
Patch
Comment 12 Levi Weintraub 2011-04-21 16:01:28 PDT
Created attachment 90622 [details]
Patch
Comment 13 Levi Weintraub 2011-04-21 16:02:37 PDT
(In reply to comment #11)
> Created an attachment (id=90613) [details]
> Patch

Added additional cases for regression testing purposes (thanks Ryosuke).
Comment 14 Ryosuke Niwa 2011-04-21 16:31:11 PDT
Comment on attachment 90622 [details]
Patch

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.
Comment 15 Levi Weintraub 2011-04-21 17:09:19 PDT
Created attachment 90638 [details]
Patch
Comment 16 Levi Weintraub 2011-04-22 10:48:58 PDT
I believe I've addressed your concerns. Could I get a review?
Comment 17 Levi Weintraub 2011-04-22 13:38:28 PDT
Created attachment 90749 [details]
Patch
Comment 18 Levi Weintraub 2011-04-22 13:39:30 PDT
(In reply to comment #17)
> Created an attachment (id=90749) [details]
> Patch

Ryosuke pointed out it's important to generate the script tests with the wrapper script to avoid unnecessary conflicts later. Fixed.
Comment 19 Levi Weintraub 2011-04-22 13:58:42 PDT
Thanks for the reviews!
Comment 20 Levi Weintraub 2011-04-22 14:05:19 PDT
Comment on attachment 90749 [details]
Patch

Clearing flags on attachment: 90749

Committed r84678: <http://trac.webkit.org/changeset/84678>
Comment 21 WebKit Review Bot 2011-04-22 16:18:38 PDT
http://trac.webkit.org/changeset/84678 might have broken GTK Linux 64-bit Debug