Selection extends beyond focus ring for some contentEditable divs

• Component: HTML Editing
• Status: RESOLVED FIXED

[Graham Dennis]

SUMMARY: If you have a contentEditable div enclosed in a div with left/right padding, selections in the contentEditable div that extend over more than one line go beyond the focus ring of the contentEditable div. I would have expected the selection to remain within the focus ring.

Does this mean the focus ring should be wider, or should the selection only extend to the border of the focus ring?

I'll attach a testcase.

[Graham Dennis]

Created attachment 6105 [details]
Testcase

To demonstrate the problem, select more than one line in the contentEditable div (the blue text). When a selection is made which extends beyond just the div into the text above or below, it makes sense (to me at least) that the selection should go all the way to the left and right. So what is the correct behaviour?

[Dan Wood]

My personal opinion is that the visual selection should *never* go outside the focus ring.  It just looks silly!

[Graham Dennis]

Created attachment 6457 [details]
patch

This patch makes the render associated with the root editable element the selection root for selections entirely contained in a contentEditable div.

[Justin Garcia]

Comment on attachment 6457 [details]
patch

We changed our coding style guidelines, the * now belongs adjacent to the type, instead of the identifier.
The FIXME above the code you added concerns me.  You'll return true for a table that is the root editable element, maybe you should return false if isTable() at the start of this function.
Optional suggestion:
Instead of getting element() over and over, you could get it once and put it into a local variable.

[Graham Dennis]

Created attachment 6666 [details]
patch 2

Patch addressing Justin's comments.

[Graham Dennis]

When I attached the second patch, I forgot to say that it passes the layout tests.

[Graham Dennis]

Created attachment 6672 [details]
patch 3

Justin: I've done more to this patch than just add a space. The previous patch was causing the following test cases to behave strangely:
editing/selection/3690703
editing/selection/3690703-2
editing/selection/3690719
They are all editable divs with tables inside. The problem was that the editable div was being set as the selection root for the tables, whereas the tables would previously have been set as the selection root.
In this patch, the editable div is only set to be the selection root for cases where the root or body elements would have been the selection root. With this patch, the previously mentioned layout tests are unchanged, but the following are changed (as expected):
editing/selection/extend-by-word-002
editing/selection/select-all-001
editing/selection/select-all-002

[Justin Garcia]

Comment on attachment 6672 [details]
patch 3

I need to look at the gap filling code more before I can review this.

[Darin Adler]

Comment on attachment 6672 [details]
patch 3

I'm not sure I understand the part where if element() is 0, we fall into the "isBody() || isRoot()" check. This otherwise looks fine to me.

[Graham Dennis]

Created attachment 6674 [details]
patch 4

I added an if (element()) return false; line at the start (and removed the later check on element()).

[Darin Adler]

Comment on attachment 6674 [details]
patch 4

Looks very good to me, but I think Justin still wants to look at the gap-filling code and make the final decision, so setting reviewer to him.

Did you run the layout tests?

[Graham Dennis]

I did run the layout tests, and the only changes were for the pixel results of the following tests:
editing/selection/extend-by-word-002
editing/selection/select-all-001
editing/selection/select-all-002
Where the changes are what is expected.

[Justin Garcia]

Comment on attachment 6674 [details]
patch 4

landed with small tweaks