Bug 6933

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?

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

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.

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.

Created attachment 6666 [details] patch 2 Patch addressing Justin's comments.

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

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

Comment on attachment 6672 [details] patch 3 I need to look at the gap filling code more before I can review this.

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.

Created attachment 6674 [details] patch 4 I added an if (element()) return false; line at the start (and removed the later check on element()).

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?

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.

Comment on attachment 6674 [details] patch 4 landed with small tweaks