[webkit-reviews] review canceled: [Bug 65999] execCommand("SelectWord") isn't idempotent : [Attachment 103505] proposed fix

Wed Aug 10 12:36:48 PDT 2011

Alexey Proskuryakov <ap at webkit.org> has canceled Alexey Proskuryakov
<ap at webkit.org>'s request for review:
Bug 65999: execCommand("SelectWord") isn't idempotent
https://bugs.webkit.org/show_bug.cgi?id=65999

Attachment 103505: proposed fix
https://bugs.webkit.org/attachment.cgi?id=103505&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103505&action=review

>> Source/WebCore/editing/VisibleSelection.cpp:298
>>		    side = LeftWordIfOnBoundary;
> 
> Should we do the same here?

I don't see the same kind of problem with start, so probably not. However, I
extended the test to check that, and found some other problems that I'd like to
address.

>> Source/WebCore/editing/VisibleSelection.cpp:306
>>		    side = LeftWordIfOnBoundary;
> 
> These enum values should be renamed to PreviousWordIfOnBoundary and
NextWordIfOnBoundary (i.e. left/right are reversed for RTL text as is).

Yup, but I don't think that it's something for this patch.

>> LayoutTests/editing/execCommand/select-word.html:1
>> +<p>Test behavior of SelectWord editor command. It should be compatible with
AppKit's selectWord:.</p>
> 
> Nit: missing DOCTYPE also ":."

I don't see a reason to add DOCTYPE (there's no <html> either, for that
matter). The colon is not a typo, but I expanded that to -[NSTextView
selectWord:] for clarity.

>> LayoutTests/editing/execCommand/select-word.html:30
>> +	[[3, 3], " "],
> 
> Really?  Shouldn't we select "123" in this case?

Ugh. Turns out that NSTextView doesn't seem to have a well specified behavior.
If you position the insertion point with arrow keys, it selects the space, and
if you position it with a mouse, it selects preceding text. I've filed
<rdar://problem/9931134> to find out what the correct behavior is.