End/Home keys do not work in a contentEditable element

• Component: HTML Editing
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

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.

[Alexander Pavlov (apavlov)]

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...

[Alexander Pavlov (apavlov)]

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.

[Alexander Pavlov (apavlov)]

Downstream Chromium issue: http://code.google.com/p/chromium/issues/detail?id=74304

[Alexander Pavlov (apavlov)]

Created attachment 84553 [details]
[HTML] A more specific reduced test case

[Levi Weintraub]

Created attachment 90601 [details]
Patch

[Ryosuke Niwa]

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?

[Levi Weintraub]

(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.

[Enrica Casucci]

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.

[Enrica Casucci]

(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.

[Levi Weintraub]

(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.

[Levi Weintraub]

Created attachment 90613 [details]
Patch

[Levi Weintraub]

Created attachment 90622 [details]
Patch

[Levi Weintraub]

(In reply to comment #11)
> Created an attachment (id=90613) [details]
> Patch

Added additional cases for regression testing purposes (thanks Ryosuke).

[Ryosuke Niwa]

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.

[Levi Weintraub]

Created attachment 90638 [details]
Patch

[Levi Weintraub]

I believe I've addressed your concerns. Could I get a review?

[Levi Weintraub]

Created attachment 90749 [details]
Patch

[Levi Weintraub]

(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.

[Levi Weintraub]

Thanks for the reviews!

[Levi Weintraub]

Comment on attachment 90749 [details]
Patch

Clearing flags on attachment: 90749

Committed r84678: <http://trac.webkit.org/changeset/84678>

[WebKit Review Bot]

http://trac.webkit.org/changeset/84678 might have broken GTK Linux 64-bit Debug