Bug 53070 – Cannot extend or modify forward by word over a non-contenteditable region

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin (Ben) Kalman]

Cannot extend or modify forward by word over a non-contenteditable region

[Benjamin (Ben) Kalman]

Created attachment 80029 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 80029 [details]
Patch

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

> LayoutTests/editing/selection/extend-forward-by-word-over-non-editable.html:37
> +  getSelection().modify("extend", "forward", "word");
> +  if (getSelection().toString() === noneditableChild.textContent)
> +    log("PASS for " + bidiName);
> +  else
> +    log("FAIL for " + bidiName + ", selection is \"" + getSelection() + "\" but should be \"" + noneditableChild.textContent + "\"");

So when we extend selection by word, we want to extend beyond the word boundary when the word was not editable?  Does that much other browsers?  Is it documented anywhere?

[Benjamin (Ben) Kalman]

rniwa: I was in the middle of writing this essay when you made your comment.  I'll address it in here :-)

---

I will paste the explanation from the changelog here, and attempt to justify it:

"
Change all instances of honorEditableBoundaryAtOrAfter to honorEditableBoundaryAtOrBefore and vice versa in the functions which determine the end/start of words/lines/sentences in visible_units.cpp.

This fixes the bug where moving forwards by a word over a non-contenteditable region would place the cursor inside that region, and then get moved back to the start of the word due to honorEditableBoundaryAtOrBefore. The cursor is now moved to the end of the region (which is effectively a noop in this case).
"

There are a few things to note about this:

(1) Perhaps surprisingly (initially), all layout tests still pass, and the test case passes when previously it didn't.

(2) Layout test passing aside, it won't result in incorrect behaviour caused by the cursor being places in a non-editable region.  How could it?  The functions still prevent this, I've just reversed them.

(3) It also won't let the cursor "escape" a contenteditable region; the text iterators still respect contenteditable boundaries.

(4) The change has a nice sideeffect of treating a non-contenteditable region e.g. <span contenteditable=false>foo bar baz</span> as a word for the purposes of selection.  Currently, "foo bar baz" is treated as an atomic unit for the purposes of moving by characters (i.e. arrow keys), but not for words.  Now it does.

Furthermore, this is what other browsers do.  Or attempt to do.  Both firefox and IE have slight bugs in this area.  Given the dom "foo bar <s editable=false>baz qux> quux asdf"
 - firefox jumps from "bar" over the "baz qux" all the way to the "asdf".
 - IE jumps from "foo" over "bar baz qux" to the "quux".
Clearly each is buggy and aiming for the behaviour that this patch results in.  I don't know if it's documented anywhere.

(5) Even though the bug only affects moving by words, I changed all of them for consistency.

[Ryosuke Niwa]

Comment on attachment 80029 [details]
Patch

sounds good to me.  you might want to re-upload your patch since the patch doesn't seem to apply on TOT.

[WebKit Commit Bot]

Comment on attachment 80029 [details]
Patch

Clearing flags on attachment: 80029

Committed r76610: <http://trac.webkit.org/changeset/76610>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.