Bug 110487 – Ctrl+Shift+Right in Windows should select the spacing after the word

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Claudio Saavedra]

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.

[Claudio Saavedra]

Created attachment 189549 [details]
Patch

[WebKit Review Bot]

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

[Tony Chang]

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?

[Ryosuke Niwa]

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.

[Tony Chang]

(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.

[Claudio Saavedra]

(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.

[Claudio Saavedra]

(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.

[Tony Chang]

(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.

[Claudio Saavedra]

(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().

[Claudio Saavedra]

Created attachment 190316 [details]
Patch

[Early Warning System Bot]

Comment on attachment 190316 [details]
Patch

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

[Early Warning System Bot]

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

[Build Bot]

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

[Build Bot]

Comment on attachment 190316 [details]
Patch

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

[Build Bot]

Comment on attachment 190316 [details]
Patch

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

[WebKit Review Bot]

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

[Claudio Saavedra]

Created attachment 190503 [details]
Patch

[Claudio Saavedra]

(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.

[Build Bot]

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

[Claudio Saavedra]

Created attachment 190519 [details]
Patch

[Claudio Saavedra]

(In reply to comment #20)
> Created an attachment (id=190519) [details]
> Patch

And another attempt at fixing the build in mac.

[Build Bot]

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

[Claudio Saavedra]

Created attachment 190528 [details]
Patch

[Build Bot]

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

[Build Bot]

Comment on attachment 190528 [details]
Patch

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

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 190528 [details]
Patch

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

[Claudio Saavedra]

Created attachment 190658 [details]
Patch

[Ryosuke Niwa]

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.

[Claudio Saavedra]

(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?

[WebKit Review Bot]

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

[Ryosuke Niwa]

(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.

[Claudio Saavedra]

(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..

[Claudio Saavedra]

(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.

[Claudio Saavedra]

(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.

[Ryosuke Niwa]

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.

[Claudio Saavedra]

(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.

[Alexey Proskuryakov]

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.

[Claudio Saavedra]

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()?

[Ryosuke Niwa]

(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.

[Claudio Saavedra]

Created attachment 194359 [details]
Patch

[Claudio Saavedra]

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.

[Claudio Saavedra]

Created attachment 194615 [details]
Patch

[Ryosuke Niwa]

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.

[Claudio Saavedra]

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.

[Claudio Saavedra]

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.

[Claudio Saavedra]

Created attachment 195080 [details]
Patch without tests without format issues.

[Build Bot]

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

[Ryosuke Niwa]

(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.

[Build Bot]

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

[Build Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Build Bot]

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

[Claudio Saavedra]

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.

[Claudio Saavedra]

Created attachment 196141 [details]
Patch without tests

Reuploaded to get EWS to check it again.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Claudio Saavedra]

(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.

[Claudio Saavedra]

Created attachment 198583 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 198583 [details]
Patch

Clearing flags on attachment: 198583

Committed r148987: <http://trac.webkit.org/changeset/148987>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.