Bug 65999

Summary:

execCommand("SelectWord") isn't idempotent

Product:

WebKit

Reporter:

Alexey Proskuryakov <ap>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

NEW

Severity:

Normal

CC:

dglazkov, rniwa, vanlam, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
proposed fix	webkit.review.bot: commit-queue-

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-	Details Formatted Diff Diff
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.