Bug 6933 - Selection extends beyond focus ring for some contentEditable divs
Summary: Selection extends beyond focus ring for some contentEditable divs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-01-30 06:18 PST by Graham Dennis
Modified: 2006-02-24 19:00 PST (History)
5 users (show)

See Also:


Attachments
Testcase (969 bytes, text/html)
2006-01-30 06:22 PST, Graham Dennis
no flags Details
patch (1.13 KB, patch)
2006-02-13 01:59 PST, Graham Dennis
justin.garcia: review-
Details | Formatted Diff | Diff
patch 2 (1.28 KB, patch)
2006-02-22 05:23 PST, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 3 (1.51 KB, patch)
2006-02-22 15:33 PST, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 4 (1.54 KB, patch)
2006-02-23 00:53 PST, Graham Dennis
justin.garcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Graham Dennis 2006-01-30 06:18:48 PST
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.
Comment 1 Graham Dennis 2006-01-30 06:22:00 PST
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?
Comment 2 Dan Wood 2006-02-12 10:22:22 PST
My personal opinion is that the visual selection should *never* go outside the focus ring.  It just looks silly!
Comment 3 Graham Dennis 2006-02-13 01:59:08 PST
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 4 Justin Garcia 2006-02-21 14:21:22 PST
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.
Comment 5 Graham Dennis 2006-02-22 05:23:04 PST
Created attachment 6666 [details]
patch 2

Patch addressing Justin's comments.
Comment 6 Graham Dennis 2006-02-22 05:23:46 PST
When I attached the second patch, I forgot to say that it passes the layout tests.
Comment 7 Graham Dennis 2006-02-22 15:33:21 PST
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 8 Justin Garcia 2006-02-22 16:42:07 PST
Comment on attachment 6672 [details]
patch 3

I need to look at the gap filling code more before I can review this.
Comment 9 Darin Adler 2006-02-22 22:23:10 PST
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.
Comment 10 Graham Dennis 2006-02-23 00:53:33 PST
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 11 Darin Adler 2006-02-23 08:39:30 PST
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?
Comment 12 Graham Dennis 2006-02-23 13:36:18 PST
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 13 Justin Garcia 2006-02-24 17:48:43 PST
Comment on attachment 6674 [details]
patch 4

landed with small tweaks