NEW 65999
execCommand("SelectWord") isn't idempotent 
https://bugs.webkit.org/show_bug.cgi?id=65999
Summary execCommand("SelectWord") isn't idempotent
Alexey Proskuryakov
Reported 2011-08-10 10:57:52 PDT
It keeps expanding the selection each time it's called, which doesn't match NSTextView, and makes no sense in general.
Attachments
proposed fix (5.76 KB, patch)
2011-08-10 11:01 PDT, Alexey Proskuryakov 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2011-08-10 11:01:54 PDT
Created attachment 103505 [details] proposed fix
Alexey Proskuryakov
Comment 2 2011-08-10 11:03:25 PDT
It's not very easy to hit this manually. The only way I know is by adding a key binding which calls this method to ~/Library/Keybindings/DefaultKeyBinding.dict.
Ryosuke Niwa
Comment 3 2011-08-10 11:23:53 PDT
Comment on attachment 103505 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=103505&action=review > Source/WebCore/editing/VisibleSelection.cpp:298 > if (isEndOfDocument(start) || (isEndOfLine(start) && !isStartOfLine(start) && !isEndOfParagraph(start))) > side = LeftWordIfOnBoundary; Should we do the same here? > Source/WebCore/editing/VisibleSelection.cpp:306 > + side = RightWordIfOnBoundary; > + if (isEndOfDocument(originalEnd) || (isEndOfLine(originalEnd) && !isStartOfLine(originalEnd) && !isEndOfParagraph(originalEnd))) > + side = LeftWordIfOnBoundary; > + } else > side = LeftWordIfOnBoundary; These enum values should be renamed to PreviousWordIfOnBoundary and NextWordIfOnBoundary (i.e. left/right are reversed for RTL text as is). > 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 ":." > LayoutTests/editing/execCommand/select-word.html:30 > + [[3, 3], " "], Really? Shouldn't we select "123" in this case?
WebKit Review Bot
Comment 4 2011-08-10 12:29:34 PDT
Comment on attachment 103505 [details] proposed fix Attachment 103505 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9333911 New failing tests: editing/selection/shift-click.html editing/selection/extend-after-mouse-selection.html editing/selection/5195166-1.html
Alexey Proskuryakov
Comment 5 2011-08-10 12:36:48 PDT
Comment on attachment 103505 [details] proposed fix 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.
Ryosuke Niwa
Comment 6 2011-08-10 12:38:32 PDT
(In reply to comment #5) > >> 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. Agreed. > >> 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. We should also test what other browsers do.
Note You need to log in before you can comment on or make changes to this bug.