[webkit-reviews] review denied: [Bug 36359] Double clicking page's last editable inline element doesn't select a word. : [Attachment 56673] took the feedback

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied MORITA Hajime <morrita at google.com>'s
request for review:
Bug 36359: Double clicking page's last editable inline element doesn't select a
word.
https://bugs.webkit.org/show_bug.cgi?id=36359

Attachment 56673: took the feedback
https://bugs.webkit.org/attachment.cgi?id=56673&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
LayoutTests/editing/selection/doubleclick-inline-first-contenteditable.html:14
 +	while (n) {
You only need this code if you have positioned elements. For this test, you can
just directly use target.offsetLeft, target.clientLeft, etc. No while loop
needed.

LayoutTests/editing/selection/doubleclick-inline-first-contenteditable.html:30
 +	eventSender.leapForward(50);
You shouldn't need this leapFoward for a doubleclick. Did you find it didn't
work without it?

Can you combine the two tests into one using HTML like the following:
<div><span id="target" contentEditable="true">first</span> double click <span
id="target" contentEditable="true">last</span></div>

I tested that locally and it reproduced the bug for both cases.

WebCore/editing/visible_units.cpp:164
 +		// editing boundary. So we lookup editable node from the
candidates.
I think this works correctly, but is a bit messy to fix it up after the fact.
The best fix I can think of would be to modify TextIterator to have a
TextIteratorBehavior that tells it to avoid crossing editing boundaries. I'm
sure this won't be the only case where we'll want TextIterators to respect
editing boundaries. That's a complicated change though. I'm OK with just adding
FIXME here for now, but I'd like feedback from other reviewers. r- for now for
the test changes and adding this fixme.

Enrica, Mitz, does this seem OK to you? Do you agree that long-term we should
make TextIterator editing-boundary-aware?