Bug 110487 - Ctrl+Shift+Right in Windows should select the spacing after the word
Summary: Ctrl+Shift+Right in Windows should select the spacing after the word
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 111321 113383 113414 113424 113490
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-21 09:38 PST by Claudio Saavedra
Modified: 2013-04-23 13:38 PDT (History)
17 users (show)

See Also:


Attachments
Patch (1.99 KB, patch)
2013-02-21 10:02 PST, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (13.37 KB, patch)
2013-02-26 09:46 PST, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (15.73 KB, patch)
2013-02-27 06:10 PST, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (15.12 KB, patch)
2013-02-27 07:36 PST, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (14.99 KB, patch)
2013-02-27 08:29 PST, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (14.32 KB, patch)
2013-02-27 22:42 PST, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (348.01 KB, patch)
2013-03-21 15:16 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (348.80 KB, patch)
2013-03-22 12:21 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch without tests, in order to assess effects of code changes. (5.17 KB, patch)
2013-03-26 06:21 PDT, Claudio Saavedra
csaavedra: review-
Details | Formatted Diff | Diff
Patch without tests without format issues. (6.23 KB, patch)
2013-03-26 07:13 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (862.56 KB, application/zip)
2013-03-26 10:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64 (837.62 KB, application/zip)
2013-03-26 14:10 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-future (863.46 KB, application/zip)
2013-03-26 23:10 PDT, Build Bot
no flags Details
Patch without tests (6.23 KB, patch)
2013-04-02 08:06 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 for chromium-linux-x86_64 (1.72 MB, application/zip)
2013-04-03 05:24 PDT, WebKit Review Bot
no flags Details
Patch (88.38 KB, patch)
2013-04-17 10:33 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Claudio Saavedra 2013-02-21 09:38:27 PST
In Windows, moving (or extending the selection) to the right with Control (and Shift) + the right arrow should also select the spacing after the current word. WebKit currently only does this when moving to the right (Control+Right), not when extending the selection (Control+Shift+Right).

I have an initial patch that solves this but I'd appreciate some comments from someone more familiar with this area.
Comment 1 Claudio Saavedra 2013-02-21 10:02:44 PST
Created attachment 189549 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-21 10:47:12 PST
Comment on attachment 189549 [details]
Patch

Attachment 189549 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16696186

New failing tests:
editing/selection/extend-selection-enclosing-block.html
editing/selection/extend-selection-word.html
Comment 3 Tony Chang 2013-02-21 11:12:54 PST
Comment on attachment 189549 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review

r- due to lack of tests and failing tests.  Why did this change the results of a test on Linux?

> Source/WebCore/editing/FrameSelection.cpp:603
> +#if USE(ICU_UNICODE)

Why does this depend on ICU_UNICODE?
Comment 4 Ryosuke Niwa 2013-02-21 11:48:00 PST
Comment on attachment 189549 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review

> Source/WebCore/editing/FrameSelection.cpp:605
> +            bool skipsSpaceWhenMovingRight = m_frame && m_frame->editor()->behavior().shouldSkipSpaceWhenMovingRight();
> +            pos = rightWordPosition(pos, skipsSpaceWhenMovingRight);

This should be testable. Also, be sure to test this with bidirectional text.
Furthermore, there should be an equivalent change in modifyExtendingLeft.
Comment 5 Tony Chang 2013-02-21 11:53:39 PST
(In reply to comment #4)
> (From update of attachment 189549 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review
> 
> > Source/WebCore/editing/FrameSelection.cpp:605
> > +            bool skipsSpaceWhenMovingRight = m_frame && m_frame->editor()->behavior().shouldSkipSpaceWhenMovingRight();
> > +            pos = rightWordPosition(pos, skipsSpaceWhenMovingRight);
> 
> This should be testable. Also, be sure to test this with bidirectional text.
> Furthermore, there should be an equivalent change in modifyExtendingLeft.

I tested the ctrl+shift+left case on my Windows machine (notepad.exe) and it does not include the preceding space.
Comment 6 Claudio Saavedra 2013-02-21 13:45:43 PST
(In reply to comment #3)
> (From update of attachment 189549 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review
> 
> r- due to lack of tests and failing tests.  Why did this change the results of a test on Linux?
> 
> > Source/WebCore/editing/FrameSelection.cpp:603
> > +#if USE(ICU_UNICODE)
> 
> Why does this depend on ICU_UNICODE?

It is commented somewhere else in the same file. I think there's one method that is not available for all platforms.
Comment 7 Claudio Saavedra 2013-02-21 13:46:41 PST
(In reply to comment #5)
> (In reply to comment #4)

> > 
> > This should be testable. Also, be sure to test this with bidirectional text.
> > Furthermore, there should be an equivalent change in modifyExtendingLeft.
> 
> I tested the ctrl+shift+left case on my Windows machine (notepad.exe) and it does not include the preceding space.

It probably does if your text is RTL. Ryosuke is probably right.
Comment 8 Tony Chang 2013-02-21 14:06:59 PST
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> 
> > > 
> > > This should be testable. Also, be sure to test this with bidirectional text.
> > > Furthermore, there should be an equivalent change in modifyExtendingLeft.
> > 
> > I tested the ctrl+shift+left case on my Windows machine (notepad.exe) and it does not include the preceding space.
> 
> It probably does if your text is RTL. Ryosuke is probably right.

Ah, yes, if the text is RTL, it probably should select the trailing space when pressing ctrl+shift+left.
Comment 9 Claudio Saavedra 2013-02-26 01:45:10 PST
(In reply to comment #3)
> (From update of attachment 189549 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review
> 
> r- due to lack of tests and failing tests.  Why did this change the results of a test on Linux?

This changed the results of the tests in Linux because what my initial patch did was to use rightWordPosition() for the cases where the enclosing paragraph is LTR. Now, this fails because when the text is using RTL, this is wrong.

It seems to me that the proper fix would be to change nextWordPosition() to take into account whether spacing should be skipped, just like it was done with rightWordPosition().
Comment 10 Claudio Saavedra 2013-02-26 09:46:21 PST
Created attachment 190316 [details]
Patch
Comment 11 Early Warning System Bot 2013-02-26 10:10:11 PST
Comment on attachment 190316 [details]
Patch

Attachment 190316 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16770324
Comment 12 Early Warning System Bot 2013-02-26 10:13:10 PST
Comment on attachment 190316 [details]
Patch

Attachment 190316 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16773304
Comment 13 Build Bot 2013-02-26 11:16:02 PST
Comment on attachment 190316 [details]
Patch

Attachment 190316 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16778291
Comment 14 Build Bot 2013-02-26 12:11:30 PST
Comment on attachment 190316 [details]
Patch

Attachment 190316 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16780269
Comment 15 Build Bot 2013-02-26 13:12:33 PST
Comment on attachment 190316 [details]
Patch

Attachment 190316 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16775390
Comment 16 WebKit Review Bot 2013-02-26 14:39:47 PST
Comment on attachment 190316 [details]
Patch

Attachment 190316 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16774461

New failing tests:
editing/execCommand/remove-formatting-2.html
editing/execCommand/toggle-unlink.html
editing/execCommand/createLink.html
editing/execCommand/button.html
editing/pasteboard/8145-2.html
editing/inserting/insert-paragraph-03.html
editing/pasteboard/5006779.html
editing/selection/4932260-2.html
editing/inserting/4960120-2.html
editing/deleting/list-item-1.html
editing/deleting/smart-delete-across-editable-boundaries.html
editing/execCommand/remove-format-multiple-elements.html
editing/execCommand/toggle-link.html
editing/execCommand/hilitecolor.html
editing/inserting/insert-before-link-1.html
editing/pasteboard/4806874.html
editing/deleting/delete-cell-contents.html
editing/deleting/smart-editing-disabled.html
editing/execCommand/unlink.html
editing/execCommand/indent-pre.html
editing/deleting/delete-by-word-001.html
editing/deleting/non-smart-delete.html
editing/deleting/delete-by-word-002.html
editing/inserting/insert-paragraph-04.html
editing/deleting/smart-delete-002.html
editing/pasteboard/bad-placeholder.html
editing/pasteboard/4242293-1.html
http/tests/cache/subresource-failover-to-network.html
editing/execCommand/query-command-state.html
Comment 17 Claudio Saavedra 2013-02-27 06:10:25 PST
Created attachment 190503 [details]
Patch
Comment 18 Claudio Saavedra 2013-02-27 06:24:34 PST
(In reply to comment #17)
> Created an attachment (id=190503) [details]
> Patch

This will probably fail tests but it should build in Mac and Qt.
Comment 19 Build Bot 2013-02-27 07:02:42 PST
Comment on attachment 190503 [details]
Patch

Attachment 190503 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16789278
Comment 20 Claudio Saavedra 2013-02-27 07:36:29 PST
Created attachment 190519 [details]
Patch
Comment 21 Claudio Saavedra 2013-02-27 07:37:24 PST
(In reply to comment #20)
> Created an attachment (id=190519) [details]
> Patch

And another attempt at fixing the build in mac.
Comment 22 Build Bot 2013-02-27 08:12:26 PST
Comment on attachment 190519 [details]
Patch

Attachment 190519 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16807280
Comment 23 Claudio Saavedra 2013-02-27 08:29:28 PST
Created attachment 190528 [details]
Patch
Comment 24 Build Bot 2013-02-27 09:21:14 PST
Comment on attachment 190528 [details]
Patch

Attachment 190528 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16810045
Comment 25 Build Bot 2013-02-27 10:31:57 PST
Comment on attachment 190528 [details]
Patch

Attachment 190528 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16803101
Comment 26 WebKit Review Bot 2013-02-27 10:58:07 PST
Comment on attachment 190528 [details]
Patch

Attachment 190528 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16835089

New failing tests:
editing/execCommand/remove-formatting-2.html
editing/execCommand/toggle-unlink.html
editing/execCommand/createLink.html
editing/execCommand/button.html
editing/pasteboard/8145-2.html
editing/inserting/insert-paragraph-03.html
editing/pasteboard/5006779.html
editing/selection/4932260-2.html
editing/inserting/4960120-2.html
editing/deleting/list-item-1.html
editing/deleting/smart-delete-across-editable-boundaries.html
editing/execCommand/remove-format-multiple-elements.html
editing/execCommand/toggle-link.html
editing/inserting/paragraph-outside-nested-divs.html
editing/inserting/insert-before-link-1.html
editing/pasteboard/4806874.html
editing/deleting/delete-cell-contents.html
editing/deleting/smart-editing-disabled.html
editing/deleting/non-smart-delete.html
editing/execCommand/unlink.html
editing/execCommand/indent-pre.html
editing/deleting/delete-by-word-001.html
editing/execCommand/hilitecolor.html
editing/pasteboard/bad-placeholder.html
editing/inserting/insert-paragraph-04.html
editing/deleting/smart-delete-002.html
editing/deleting/delete-by-word-002.html
editing/pasteboard/4242293-1.html
http/tests/cache/subresource-failover-to-network.html
editing/execCommand/query-command-state.html
Comment 27 Build Bot 2013-02-27 11:59:09 PST
Comment on attachment 190528 [details]
Patch

Attachment 190528 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16779485
Comment 28 Claudio Saavedra 2013-02-27 22:42:51 PST
Created attachment 190658 [details]
Patch
Comment 29 Ryosuke Niwa 2013-02-27 23:14:12 PST
Comment on attachment 190658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review

> Source/WebCore/platform/text/TextBoundaries.cpp:65
> -int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward)
> +int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward, bool skipSpaces)

It's not a good idea to modify findNextWordFromIndex like this. Please follow what we did for visualWordPosition.
Comment 30 Claudio Saavedra 2013-02-27 23:31:29 PST
(In reply to comment #29)
> (From update of attachment 190658 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review
> 
> > Source/WebCore/platform/text/TextBoundaries.cpp:65
> > -int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward)
> > +int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward, bool skipSpaces)
> 
> It's not a good idea to modify findNextWordFromIndex like this. Please follow what we did for visualWordPosition.

Thanks for the comments. Not sure I understand though. visualWordPosition() is used from left/rightWordPosition(). For the platforms where the wordbreak is not available, the fallback is going to lead to findNextWordFromIndex() too.. would you mind clarifying what exactly you mean?
Comment 31 WebKit Review Bot 2013-02-28 00:01:45 PST
Comment on attachment 190658 [details]
Patch

Attachment 190658 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16822318

New failing tests:
editing/execCommand/remove-formatting-2.html
editing/execCommand/toggle-unlink.html
editing/execCommand/createLink.html
editing/execCommand/button.html
editing/pasteboard/8145-2.html
editing/inserting/insert-paragraph-03.html
editing/pasteboard/5006779.html
editing/selection/4932260-2.html
editing/inserting/4960120-2.html
editing/deleting/list-item-1.html
editing/deleting/smart-delete-across-editable-boundaries.html
editing/execCommand/remove-format-multiple-elements.html
editing/execCommand/toggle-link.html
editing/execCommand/hilitecolor.html
editing/pasteboard/copy-text-with-backgroundcolor.html
editing/inserting/insert-before-link-1.html
editing/pasteboard/4806874.html
editing/deleting/delete-cell-contents.html
editing/deleting/smart-editing-disabled.html
editing/execCommand/unlink.html
editing/execCommand/indent-pre.html
editing/deleting/delete-by-word-001.html
editing/deleting/non-smart-delete.html
editing/deleting/delete-by-word-002.html
editing/inserting/insert-paragraph-04.html
editing/deleting/smart-delete-002.html
editing/pasteboard/bad-placeholder.html
editing/pasteboard/4242293-1.html
http/tests/cache/subresource-failover-to-network.html
editing/execCommand/query-command-state.html
Comment 32 Ryosuke Niwa 2013-02-28 00:22:32 PST
(In reply to comment #30)
> (In reply to comment #29)
> > (From update of attachment 190658 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review
> > 
> > > Source/WebCore/platform/text/TextBoundaries.cpp:65
> > > -int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward)
> > > +int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward, bool skipSpaces)
> > 
> > It's not a good idea to modify findNextWordFromIndex like this. Please follow what we did for visualWordPosition.
> 
> Thanks for the comments. Not sure I understand though. visualWordPosition() is used from left/rightWordPosition(). For the platforms where the wordbreak is not available, the fallback is going to lead to findNextWordFromIndex() too.. would you mind clarifying what exactly you mean?

The technique we used there is to move forward twice and go back.
Comment 33 Claudio Saavedra 2013-02-28 00:44:01 PST
(In reply to comment #32)
> (In reply to comment #30)
> > (In reply to comment #29)

> The technique we used there is to move forward twice and go back.

I might be overlooking something but that's not what I read in the code. What I see is that in visualWordPosition() it is iterated until finding the start or end of the next word, depending on whether we're in LTR/RTL and whether it is desired to skip spaces or not. I don't think there's any moving forward twice and then back..
Comment 34 Claudio Saavedra 2013-03-04 05:35:55 PST
(In reply to comment #3)
> (From update of attachment 189549 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review
> 
> r- due to lack of tests and failing tests.  Why did this change the results of a test on Linux?

Some of the tests break in cr-linux because Chromium's TestRunner uses WebSettings::EditingBehaviorWin in all platforms but Linux. Since this changes the outcome of extending the selection in Windows, many test expectations will need to be updated.
Comment 35 Claudio Saavedra 2013-03-04 06:04:45 PST
(In reply to comment #34)
> (In reply to comment #3)
> > (From update of attachment 189549 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review
> > 
> > r- due to lack of tests and failing tests.  Why did this change the results of a test on Linux?
> 
> Chromium's TestRunner uses WebSettings::EditingBehaviorWin in all platforms but Linux.

Excuse my thinko -- in all platforms but Mac.
Comment 36 Ryosuke Niwa 2013-03-04 15:06:24 PST
Comment on attachment 190658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review

> Source/WebCore/editing/FrameSelection.cpp:609
> +        if (directionOfEnclosingBlock() == LTR) {
> +#if PLATFORM(MAC)
>              pos = nextWordPosition(pos);
> -        else
> +#else
> +            bool skipsSpaceWhenMovingForward = m_frame && m_frame->editor()->behavior().shouldSkipSpaceWhenMovingRight();
> +            pos = nextWordPosition(pos, skipsSpaceWhenMovingForward);
> +#endif

I don't want to see all these if-defs here.
Comment 37 Claudio Saavedra 2013-03-05 12:12:49 PST
(In reply to comment #36)
> (From update of attachment 190658 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review
> 
> > Source/WebCore/editing/FrameSelection.cpp:609
> > +        if (directionOfEnclosingBlock() == LTR) {
> > +#if PLATFORM(MAC)
> >              pos = nextWordPosition(pos);
> > -        else
> > +#else
> > +            bool skipsSpaceWhenMovingForward = m_frame && m_frame->editor()->behavior().shouldSkipSpaceWhenMovingRight();
> > +            pos = nextWordPosition(pos, skipsSpaceWhenMovingForward);
> > +#endif
> 
> I don't want to see all these if-defs here.

I am not sure how to implement a version of findNextWordFromIndex() for mac that would take into account whether spaces should be skipped or not. The existing method seems to use NSAttributedString attributes. Since Mac doesn't follow the Windows behavior, this doesn't seem absolutely mandatory, but if some dev from the Mac port is kind enough to jump in we might not need the ifdef's.
Comment 38 Alexey Proskuryakov 2013-03-05 12:53:43 PST
Comment on attachment 190658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review

I think that Ryosuke's request is not to implement Windows behavior using NSString APIs, but to make sure that editing behavior is not determined at compile time.

We want to test all platforms behavior in regression tests, using a window.internals switch to choose editing behavior in each test. This way, running tests locally would be a better indication that a patch is correct.

Since the patch still does not have tests, it's automatically r-.

> Source/WTF/wtf/unicode/icu/UnicodeIcu.h:179
> +inline bool isSpace(UChar32 c)
> +{
> +    return !!u_isWhitespace(c);
> +}

This is not a good name for this function (isWhitespace is not so great either, but it's OK in ICU context).

There are multiple definitions of whitespace used in the Web platform. They may or may not include Unicode spaces, form feeds etc.
Comment 39 Claudio Saavedra 2013-03-05 22:44:49 PST
Thanks for your comments!

(In reply to comment #38)
> (From update of attachment 190658 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review
> 
> I think that Ryosuke's request is not to implement Windows behavior using NSString APIs, but to make sure that editing behavior is not determined at compile time.

Editing behavior is not determined at compile-time. I'll explain why there are ifdef's more carefully: the way the next stop is determined, in all platforms, is through a call to the findNextWordFromIndex() method. What my patch is doing is to add a boolean parameter to the signature of this method, so that it looks like this:

  findNextWordFromIndex(UChar const* buffer, int len, int position, bool forward, bool skipSpaces);

This way, the calling methods can decide whether to request from this method to skip spaces when searching for the next word. Calling methods, of course, rely on the run-time property EditingBehavior. As you can see, the editing behavior is never determined at run-time.

As to the ifdefs: This method has different implementations: a generic one, one for the Qt port (using QString), and one for the Mac one (using NSString). My patch modifies both the generic implementation and the one for the Qt port. To make the patch complete one would need to modify the Mac implementation as well so that it works as we now need. Admittedly, since I am not familiar with NSString at all I've left this out. I am open to suggestions as to how to complete this patch so that findNextWordFromIndex() in Mac works in the same way and the ifdefs are not necessary.


> 
> We want to test all platforms behavior in regression tests, using a window.internals switch to choose editing behavior in each test. This way, running tests locally would be a better indication that a patch is correct.
> 
> Since the patch still does not have tests, it's automatically r-.

I will add tests soon, of course. However, at this point I am mostly concerned in what we're discussing above.

> 
> > Source/WTF/wtf/unicode/icu/UnicodeIcu.h:179
> > +inline bool isSpace(UChar32 c)
> > +{
> > +    return !!u_isWhitespace(c);
> > +}
> 
> This is not a good name for this function (isWhitespace is not so great either, but it's OK in ICU context).
> 
> There are multiple definitions of whitespace used in the Web platform. They may or may not include Unicode spaces, form feeds etc.

Do you have any suggestion? Maybe just isWhitespace() or isSpacing()?
Comment 40 Ryosuke Niwa 2013-03-06 12:06:46 PST
(In reply to comment #39)
> Thanks for your comments!
>
> Editing behavior is not determined at compile-time. I'll explain why there are ifdef's more carefully: the way the next stop is determined, in all platforms, is through a call to the findNextWordFromIndex() method. What my patch is doing is to add a boolean parameter to the signature of this method, so that it looks like this:
> 
>   findNextWordFromIndex(UChar const* buffer, int len, int position, bool forward, bool skipSpaces);
> 
> This way, the calling methods can decide whether to request from this method to skip spaces when searching for the next word. Calling methods, of course, rely on the run-time property EditingBehavior. As you can see, the editing behavior is never determined at run-time.

What I've been telling you ever since this this bug is filed is that we shouldn't do this. Instead, call nextWordPosition twice and call previousWordPosition that'll move the caret to the position after the whitespace. This way, new behavior can be implemented and exposed in DRT everywhere.
Comment 41 Claudio Saavedra 2013-03-21 15:16:22 PDT
Created attachment 194359 [details]
Patch
Comment 42 Claudio Saavedra 2013-03-22 11:05:58 PDT
Comment on attachment 194359 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194359&action=review

> Source/WebCore/editing/FrameSelection.cpp:598
> +            pos = nextWordPosition(pos);

There is a bug here if c is in the middle of the word. In that case pos will be in a previous position and this won't work. I will attach a new patch correcting this.
Comment 43 Claudio Saavedra 2013-03-22 12:21:29 PDT
Created attachment 194615 [details]
Patch
Comment 44 Ryosuke Niwa 2013-03-25 17:19:48 PDT
Comment on attachment 194615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194615&action=review

I would have preferred test changes to be submitted separately on a separate bug so that we can assess the effect of the code change more closely.

> Source/WebCore/editing/FrameSelection.cpp:584
> +VisiblePosition FrameSelection::nextWordPositionForPlatform(const VisiblePosition &c)

Please don't use one-letter variables like c.

> Source/WebCore/editing/FrameSelection.cpp:586
> +    VisiblePosition pos = nextWordPosition(c);

Please don't use abbreviations like "pos". Also, "position" is a very generic name that doesn't really tell us the semantics.
How about "currentPosition"?

> Source/WebCore/editing/FrameSelection.cpp:590
> +    // In Windows, moving by word moves until the beginning of the
> +    // next word. We emulate that behavior here by moving twice
> +    // forward (intermediatePosition) and then once back.

I'm not sure if this comment is all that helpful. Given that this is an editing behavior, it's no longer tied to Windows or that it may change in the future.
Given that the code already tells us we're moving forward twice and going backwards once, we're probably repeating ourselves needlessly.
In WebKit, we try to avoid adding comments like this that merely explains what the code does as much as possible.
If we could explain *why* we do it, that's useful.

> Source/WebCore/editing/FrameSelection.cpp:592
> +        VisiblePosition intermediatePosition = nextWordPosition(pos);

"intermediate" is not a descriptive name either. Something like positionAfterCurrentWord would better. In particular, this emphasizes the fact it's not after the whitespace.

> Source/WebCore/editing/FrameSelection.cpp:597
> +        // When moving back, it is possible that we end up in the
> +        // start of the current word. In that case, just move until
> +        // the intermediate position.

Instead of adding a long comment line this, we can give the variable a more describe name.

> Source/WebCore/editing/FrameSelection.cpp:599
> +        VisiblePosition wordStart = previousWordPosition(nextWordPosition(c));
> +        if (pos == wordStart)

e.g. we can do something like
bool movingBackwardsTwiceMovedPositionToStartOfCurrentWord = intermiediatePosition == previousWordPosition(nextWordPosition(c));

> LayoutTests/ChangeLog:8
> +        This is a massive review of all tests that use word selection for

Massive review? or massive rebaseline?

> LayoutTests/ChangeLog:28
> +        uncovered a bug in execCommand(), reported now as wkb112240, that

Please use a regular URL like http://webkit.org/b/112240. "wkb" is not a thing.

> LayoutTests/ChangeLog:32
> +        * editing/deleting/smart-editing-disabled-mac-expected.txt: Copied from LayoutTests/editing/deleting/smart-editing-disabled-expected.txt.
> +        * editing/deleting/smart-editing-disabled-mac.html: Copied from LayoutTests/editing/deleting/smart-editing-disabled.html.

These lines are really long. Please line break somewhere. And ditto for the following lines.

> LayoutTests/TestExpectations:12
> +# pending bug fix
> +webkit.org/b/112240 editing/execCommand/remove-format-multiple-elements-win.html [ Skip ]

Please don't skip an existing test on all platforms. r- because of this change.
You may add [ Failure ] expectation on some platform where this test fails but it's not acceptable to disable a test on all platforms.
If we need to fix a test, then please fix it before submitting a patch on this bug.
Comment 45 Claudio Saavedra 2013-03-26 06:10:27 PDT
Comment on attachment 194615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194615&action=review

Thanks for the review. A few comments follow.

>> Source/WebCore/editing/FrameSelection.cpp:599
>> +        if (pos == wordStart)
> 
> e.g. we can do something like
> bool movingBackwardsTwiceMovedPositionToStartOfCurrentWord = intermiediatePosition == previousWordPosition(nextWordPosition(c));

It would be movingBackwardsMovedPositionToStasrtOfCurrentWord, as we never move backward twice, only once.

>> LayoutTests/ChangeLog:8
>> +        This is a massive review of all tests that use word selection for
> 
> Massive review? or massive rebaseline?

Some tests have been rebaselined, others fixed differently.

>> LayoutTests/TestExpectations:12
>> +webkit.org/b/112240 editing/execCommand/remove-format-multiple-elements-win.html [ Skip ]
> 
> Please don't skip an existing test on all platforms. r- because of this change.
> You may add [ Failure ] expectation on some platform where this test fails but it's not acceptable to disable a test on all platforms.
> If we need to fix a test, then please fix it before submitting a patch on this bug.

It is not an existing test. It is a new test (see the ChangeLog entry fot that test.) It fails because of a completely different bug I have found that is unrelated to this one (bug 112240, already existing and affecting many editor commands). The test fails in all platforms because, as specified by the filename, the test forces a windows editing behavior regardless of the platform where it runs.

Does Failure runs the test anyway? I think that would make sense, but as I said this will fail in all platforms until the bug is addressed. Another option is not to add it or to add an -expected file that takes into consideration the failure.
Comment 46 Claudio Saavedra 2013-03-26 06:21:21 PDT
Created attachment 195074 [details]
Patch without tests, in order to assess effects of code changes.

This patch has no tests so I don't expect it to be committed. It addresses your comments regarding variable naming and code comments. We can check with this what breaks.
Comment 47 Claudio Saavedra 2013-03-26 07:13:19 PDT
Created attachment 195080 [details]
Patch without tests without format issues.
Comment 48 Build Bot 2013-03-26 10:15:27 PDT
Comment on attachment 195080 [details]
Patch without tests without format issues.

Attachment 195080 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17309297
Comment 49 Ryosuke Niwa 2013-03-26 10:16:49 PDT
(In reply to comment #45)
> >> LayoutTests/TestExpectations:12
> >> +webkit.org/b/112240 editing/execCommand/remove-format-multiple-elements-win.html [ Skip ]
> > 
> > Please don't skip an existing test on all platforms. r- because of this change.
> > You may add [ Failure ] expectation on some platform where this test fails but it's not acceptable to disable a test on all platforms.
> > If we need to fix a test, then please fix it before submitting a patch on this bug.
> 
> It is not an existing test. It is a new test (see the ChangeLog entry fot that test.) It fails because of a completely different bug I have found that is unrelated to this one (bug 112240, already existing and affecting many editor commands). The test fails in all platforms because, as specified by the filename, the test forces a windows editing behavior regardless of the platform where it runs.

No. The test was passing in Windows editing behavior prior to this patch. This patch introduces new behavior and reveals the bug that had previously not encountered on Windows. Unfortunately, that's not acceptable in WebKit. We need to fix the said bug prior to fixing this bug.
Comment 50 Build Bot 2013-03-26 10:31:10 PDT
Comment on attachment 195080 [details]
Patch without tests without format issues.

Attachment 195080 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17318227

New failing tests:
editing/selection/selection-extend-should-not-move-across-caret-on-mac.html
fast/repaint/japanese-rl-selection-repaint-in-regions.html
editing/execCommand/query-command-state.html
Comment 51 Build Bot 2013-03-26 10:31:15 PDT
Created attachment 195110 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 52 WebKit Review Bot 2013-03-26 14:10:00 PDT
Comment on attachment 195080 [details]
Patch without tests without format issues.

Attachment 195080 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17112914

New failing tests:
editing/execCommand/remove-formatting-2.html
editing/inserting/paragraph-separator-01.html
editing/execCommand/createLink.html
editing/execCommand/button.html
editing/pasteboard/8145-2.html
editing/inserting/insert-paragraph-03.html
editing/pasteboard/5006779.html
editing/selection/4932260-2.html
editing/inserting/4960120-2.html
editing/deleting/list-item-1.html
editing/deleting/smart-delete-across-editable-boundaries.html
editing/execCommand/remove-format-multiple-elements.html
editing/execCommand/toggle-link.html
editing/inserting/paragraph-outside-nested-divs.html
editing/inserting/insert-before-link-1.html
editing/pasteboard/4806874.html
editing/deleting/delete-cell-contents.html
editing/deleting/smart-editing-disabled.html
editing/deleting/non-smart-delete.html
editing/execCommand/unlink.html
editing/execCommand/indent-pre.html
editing/deleting/delete-by-word-001.html
editing/execCommand/hilitecolor.html
editing/pasteboard/bad-placeholder.html
editing/inserting/insert-paragraph-04.html
editing/deleting/smart-delete-002.html
editing/deleting/delete-by-word-002.html
editing/pasteboard/4242293-1.html
http/tests/cache/subresource-failover-to-network.html
editing/execCommand/query-command-state.html
Comment 53 WebKit Review Bot 2013-03-26 14:10:05 PDT
Created attachment 195158 [details]
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 54 Build Bot 2013-03-26 23:10:25 PDT
Comment on attachment 195080 [details]
Patch without tests without format issues.

Attachment 195080 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17221808

New failing tests:
editing/selection/selection-extend-should-not-move-across-caret-on-mac.html
editing/execCommand/query-command-state.html
Comment 55 Build Bot 2013-03-26 23:10:30 PDT
Created attachment 195230 [details]
Archive of layout-test-results from webkit-ews-04 for mac-future

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-future  Platform: Mac OS X 10.8.2
Comment 56 Claudio Saavedra 2013-04-01 23:47:09 PDT
Comment on attachment 195080 [details]
Patch without tests without format issues.

Flipping to get updated test results after the patches to tests that have landed.
Comment 57 Claudio Saavedra 2013-04-02 08:06:34 PDT
Created attachment 196141 [details]
Patch without tests

Reuploaded to get EWS to check it again.
Comment 58 WebKit Review Bot 2013-04-03 05:24:30 PDT
Comment on attachment 196141 [details]
Patch without tests

Attachment 196141 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17304705

New failing tests:
editing/style/remove-underline-across-paragraph-in-bold.html
editing/selection/selection-extend-should-not-move-across-caret-on-mac.html
editing/execCommand/createLink.html
editing/style/remove-underline-from-stylesheet.html
editing/execCommand/button.html
editing/pasteboard/merge-end-table-2.html
editing/pasteboard/page-zoom.html
editing/style/remove-underline-across-paragraph.html
editing/inserting/insert-before-link-1.html
editing/style/remove-underline.html
editing/deleting/delete-cell-contents.html
editing/undo/replace-text-in-node-preserving-markers-crash.html
editing/execCommand/unlink.html
editing/execCommand/indent-pre.html
editing/style/unbold-in-bold.html
editing/style/apply-through-end-of-document.html
editing/selection/extend-selection-word.html
editing/pasteboard/merge-end-list-2.html
editing/deleting/delete-by-word-002.html
editing/pasteboard/display-block-on-spans.html
editing/style/remove-underline-in-bold.html
editing/execCommand/query-command-state.html
Comment 59 WebKit Review Bot 2013-04-03 05:24:37 PDT
Created attachment 196331 [details]
Archive of layout-test-results from gce-cr-linux-07 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 60 Claudio Saavedra 2013-04-17 05:55:34 PDT
(In reply to comment #49)
> (In reply to comment #45)

> > It is not an existing test. It is a new test (see the ChangeLog entry fot that test.) It fails because of a completely different bug I have found that is unrelated to this one (bug 112240, already existing and affecting many editor commands). The test fails in all platforms because, as specified by the filename, the test forces a windows editing behavior regardless of the platform where it runs.
> 
> No. The test was passing in Windows editing behavior prior to this patch. This patch introduces new behavior and reveals the bug that had previously not encountered on Windows. Unfortunately, that's not acceptable in WebKit. We need to fix the said bug prior to fixing this bug.

Bug 112240 was fixed! I'll prepare a new patch updating the tests now. It might take a bit of time with all the changes in the WK tree post-blink.
Comment 61 Claudio Saavedra 2013-04-17 10:33:31 PDT
Created attachment 198583 [details]
Patch
Comment 62 WebKit Commit Bot 2013-04-23 13:38:20 PDT
Comment on attachment 198583 [details]
Patch

Clearing flags on attachment: 198583

Committed r148987: <http://trac.webkit.org/changeset/148987>
Comment 63 WebKit Commit Bot 2013-04-23 13:38:28 PDT
All reviewed patches have been landed.  Closing bug.