Bug 106815 – [Chromium] Tests and fixes for spell checker behavior

• Component: HTML Editing
• Status: RESOLVED FIXED

[Rouslan Solomakhin]

Need to add test to for how spell checker behaves with various selections:
 - No selection
 - Sub-word selection
 - Selection with underscore
 - Selection with punctuation
 - Selection with trailing whitespace
 - Selection with multiple words

[Rouslan Solomakhin]

Created attachment 184278 [details]
Proposed patch

Please review the attached patch. Here is the behavior for Chromium that this patch sets:

- Chrome selects the misspelled word on context click. 
- Spelling should work when the user selects the misspelled word exactly. 
- If the word is not misspelled, there should be no spelling options in the context menu. 
- Chrome should select multi-word misspellings on context click. 
- Spelling should work when the user selects a multi-word misspelling exactly. 
- Spelling should work for double-clicked misspellings. 
- Spelling should ignore whitespace. 
- Underscores should be treated as whitespace: spelling should ignore them. 
- Punctuation marks should be treated as whitespace: spelling should ignore them. 
- Spelling should be disabled when user selects a part of misspelling. 
- Spelling should be disabled when user selects multiple words that are not a single misspelling.

[Rouslan Solomakhin]

Hi Anders,

I see that you have reviewed changes to Source/WebKit/chromium/src/ContextMenuClientImpl.cpp before. Are you the right person to ask for a review on the attached patch? Thank you.

Cheers,
Rouslan

[Rouslan Solomakhin]

Created attachment 184581 [details]
Proposed patch

[WebKit Review Bot]

Attachment 184581 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/chromium/editing/spelling/spelling-double-clicked-word-expected.txt', u'LayoutTests/platform/chromium/editing/spelling/spelling-double-clicked-word-with-underscores-expected.txt', u'LayoutTests/platform/chromium/editing/spelling/spelling-double-clicked-word-with-underscores.html', u'LayoutTests/platform/chromium/editing/spelling/spelling-double-clicked-word.html', u'LayoutTests/platform/chromium/editing/spelling/spelling-exactly-selected-multiple-words-expected.txt', u'LayoutTests/platform/chromium/editing/spelling/spelling-exactly-selected-multiple-words.html', u'LayoutTests/platform/chromium/editing/spelling/spelling-exactly-selected-word-expected.txt', u'LayoutTests/platform/chromium/editing/spelling/spelling-exactly-selected-word.html', u'LayoutTests/platform/chromium/editing/spelling/spelling-multiword-selection-expected.txt', u'LayoutTests/platform/chromium/editing/spelling/spelling-multiword-selection.html', u'LayoutTests/platform/chromium/editing/spelling/spelling-should-select-multiple-words-expected.txt', u'LayoutTests/platform/chromium/editing/spelling/spelling-should-select-multiple-words.html', u'LayoutTests/platform/chromium/editing/spelling/spelling-should-select-single-word-expected.txt', u'LayoutTests/platform/chromium/editing/spelling/spelling-should-select-single-word.html', u'LayoutTests/platform/chromium/editing/spelling/spelling-subword-selection-expected.txt', u'LayoutTests/platform/chromium/editing/spelling/spelling-subword-selection.html', u'LayoutTests/platform/chromium/editing/spelling/spelling-with-punctuation-selection-expected.txt', u'LayoutTests/platform/chromium/editing/spelling/spelling-with-punctuation-selection.html', u'LayoutTests/platform/chromium/editing/spelling/spelling-with-underscore-selection-expected.txt', u'LayoutTests/platform/chromium/editing/spelling/spelling-with-underscore-selection.html', u'LayoutTests/platform/chromium/editing/spelling/spelling-with-whitespace-selection-expected.txt', u'LayoutTests/platform/chromium/editing/spelling/spelling-with-whitespace-selection.html', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', u'Source/WebKit/chromium/src/ContextMenuClientImpl.cpp', u'Source/WebKit/chromium/src/WebFrameImpl.cpp']" exit_code: 1
Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:81:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rouslan Solomakhin]

Created attachment 184588 [details]
Proposed patch

[Build Bot]

Comment on attachment 184588 [details]
Proposed patch

Attachment 184588 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16097512

[Build Bot]

Comment on attachment 184588 [details]
Proposed patch

Attachment 184588 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16123006

[WebKit Review Bot]

Comment on attachment 184588 [details]
Proposed patch

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

New failing tests:
platform/chromium/editing/spelling/spelling-should-select-single-word.html
platform/chromium/editing/spelling/spelling-double-clicked-word.html
platform/chromium/editing/spelling/spelling-double-clicked-word-with-underscores.html
platform/chromium/editing/spelling/spelling-should-select-multiple-words.html
platform/chromium/editing/spelling/spelling-exactly-selected-word.html
platform/chromium/editing/spelling/spelling-with-punctuation-selection.html
platform/chromium/editing/spelling/spelling-with-underscore-selection.html
platform/chromium/editing/spelling/spelling-with-whitespace-selection.html

[Build Bot]

Comment on attachment 184588 [details]
Proposed patch

Attachment 184588 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16114215

[Tony Chang]

Comment on attachment 184588 [details]
Proposed patch

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

You need a ChangeLog.  If you use "webkit-patch upload" it should create one for you and open it in a text editor for you.  Otherwise, you can use prepare-ChangeLog.

Why are these tests only run on Chromium?  Shouldn't the result be the same on all WebKit ports?  Does this depend on features that are Chromium specific?  If so, you could put them in a subdirectory (e.g., editing/spelling/unified-text-checker) and skip that subdir on other platforms since they may enable it in the future.

> LayoutTests/platform/chromium/editing/spelling/spelling-double-clicked-word-with-underscores.html:16
> +    internals.settings.setUnifiedTextCheckerEnabled(false);

This shouldn't be necessary. The test harness (DRT) should automatically undo any settings you set. If you see a different behavior, please file a bug about it and cc me.

> LayoutTests/platform/chromium/editing/spelling/spelling-double-clicked-word-with-underscores.html:76
> +description("Spelling should work for double-clicked misspelled words with underscores.");
> +if (window.internals)
> +    test();

It would be nice if there was some text describing how to run the test manually.  You could write this in the else branch of "if (window.internals)" or just put it in the description.

[Tony Chang]

I think Morrita knows this code and can review it.

The tests probably fail depend on what platform you run it on.  You might have to setSelectTrailingWhitespaceEnabled and setSmartInsertDeleteEnabled in each test to get consistent results on each platform.

[Tony Chang]

Hironori is also a good person to do an informal review of this.

[Rouslan Solomakhin]

Created attachment 184808 [details]
Proposed patch

Added Changelog.
Added description of how to test manually.
Added setSelectTrailingWhitespaceEnabled and setSmartInsertDeleteEnabled in all tests.
Removed unnecessary test cleanup calls.

Tony: Thank you for the review. I've placed the tests in platform/chromium/editing/spelling because I am testing and fixing the behavior of Source/WebKit/chromium/src/ContextMenuClientImpl.cpp specifically. Chromium spell check behaves differently from other WebKit ports. For example, Chromium selects the misspelled word on context click, but Safari does not.

Calls to setSelectTrailingWhitespaceEnabled should be necessary only when double-clicks are involved, but I've put it in all the tests for good measure.

Calls to setSmartInsertDeleteEnabled should be necessary only when we test replacing the misspelling with a suggestion. These tests do not exercise that functionality, but I've put it in all the tests for good measure, too.

[Build Bot]

Comment on attachment 184808 [details]
Proposed patch

Attachment 184808 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16097943

[Rouslan Solomakhin]

(In reply to comment #10)
> Why are these tests only run on Chromium?  Shouldn't the result be the same on all WebKit ports?  Does this depend on features that are Chromium specific?  If so, you could put them in a subdirectory (e.g., editing/spelling/unified-text-checker) and skip that subdir on other platforms since they may enable it in the future.

If we decide to put the tests in a subdirectory of editing/spelling, then we probably should have three subdirectories for the three different features that Chromium implements:

1) editing/spelling/select-on-context-click/ - Select misspelled word on context-click.

2) editing/spelling/selection/ - Spell check selected word.

3) editing/spelling/ignore-whitespace/ - Ignore selected whitespace in spell check.

4) editing/spelling/ignore-punctuation/ - Ignore selected punctuation in spell check.

[Rouslan Solomakhin]

(In reply to comment #15)
> three subdirectories
four, that is.

[kov's GTK+ EWS bot]

Comment on attachment 184808 [details]
Proposed patch

Attachment 184808 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16123486

[WebKit Review Bot]

Comment on attachment 184808 [details]
Proposed patch

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

New failing tests:
platform/chromium/editing/spelling/spelling-should-select-single-word.html
platform/chromium/editing/spelling/spelling-double-clicked-word.html
platform/chromium/editing/spelling/spelling-double-clicked-word-with-underscores.html
platform/chromium/editing/spelling/spelling-should-select-multiple-words.html
platform/chromium/editing/spelling/spelling-exactly-selected-word.html
platform/chromium/editing/spelling/spelling-with-punctuation-selection.html
platform/chromium/editing/spelling/spelling-with-underscore-selection.html
platform/chromium/editing/spelling/spelling-with-whitespace-selection.html

[Build Bot]

Comment on attachment 184808 [details]
Proposed patch

Attachment 184808 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16113612

[Tony Chang]

I think you need to update the symbol exports for the platforms that are failing:
http://trac.webkit.org/wiki/ExportingSymbols

The tests are still failing on the cr-linux box.  What platform are you running the tests on?

There's a lot of common boilerplate in the tests.  You might want to factor that into a shared .js file that the tests all include.  You would put the shared .js file in a resources subdirectory.

I thought Safari (and OS X in general) always selected the word under the cursor when you right click?

We normally only put tests in a platform specific subdirectory when we're testing platform specific behavior (e.g., there are some Apple Script tests in platform/mac).  None of these behaviors seem platform specific to me, although there might be slightly different results on different platforms, in which case, we can check in multiple results files or disable the failing tests on the platforms they fail on.

[Rouslan Solomakhin]

Created attachment 184825 [details]
Proposed patch

Added symbol exports. Want to see if this fixes any of the build bots.

[Rouslan Solomakhin]

(In reply to comment #20)
> I think you need to update the symbol exports for the platforms that are failing:
> http://trac.webkit.org/wiki/ExportingSymbols

Thank you for pointing that out. I am testing the patch with exported symbols now.

> The tests are still failing on the cr-linux box.  What platform are you running the tests on?

I have been running the following command in Chromium.

$ ~/src/webkit/tools/layout_tests/run_webkit_tests.sh --no-pixel-tests platform/chromium/editing/spelling

Although this is a Chromium tool, my WebKit in src/third_party/WebKit is checked out directly from git://git.webkit.org/WebKit.git.

> There's a lot of common boilerplate in the tests.  You might want to factor that into a shared .js file that the tests all include.  You would put the shared .js file in a resources subdirectory.

Will do.

> I thought Safari (and OS X in general) always selected the word under the cursor when you right click?

I double-checked and you are correct. Chrome matches Safari in that misspelled words are selected when you right click. Safari also selects correctly spelled words when you right click. Chrome does not, however.

> We normally only put tests in a platform specific subdirectory when we're testing platform specific behavior (e.g., there are some Apple Script tests in platform/mac).  None of these behaviors seem platform specific to me, although there might be slightly different results on different platforms, in which case, we can check in multiple results files or disable the failing tests on the platforms they fail on.

Sounds good. I am working on it.

[WebKit Review Bot]

Comment on attachment 184825 [details]
Proposed patch

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

New failing tests:
platform/chromium/editing/spelling/spelling-should-select-single-word.html
platform/chromium/editing/spelling/spelling-double-clicked-word.html
platform/chromium/editing/spelling/spelling-double-clicked-word-with-underscores.html
platform/chromium/editing/spelling/spelling-should-select-multiple-words.html
platform/chromium/editing/spelling/spelling-exactly-selected-word.html
platform/chromium/editing/spelling/spelling-with-punctuation-selection.html
platform/chromium/editing/spelling/spelling-with-underscore-selection.html
platform/chromium/editing/spelling/spelling-with-whitespace-selection.html

[Build Bot]

Comment on attachment 184825 [details]
Proposed patch

Attachment 184825 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16111885

[Hajime Morrita]

Comment on attachment 184825 [details]
Proposed patch

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

I'm wondering whether we can get rid of addSpellingMarker().
In general, Internals object is for inspecting hidden state from JS and
issuing a action with side effect through the Internals object isn't a good idea.
It can be a signal of testing something that never happens in real Web.

One possible workaround is to tell MockSpellChecker a multi-word misspelling candidate.
http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp
So that it marks the text as mis-spelled. This approach is more like end-to-end than unit testing, which WebKit traditionally prefers.

> Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:289
> +                RefPtr<Range> markerRange = selectionRange->cloneRange(exceptionCode);

Nit: Can this fail?  If not, we can use ASSERT_NO_EXCEPTION macro to clarify that fact.
https://lists.webkit.org/pipermail/webkit-dev/2011-December/018908.html

[Rouslan Solomakhin]

Created attachment 185091 [details]
Proposed patch

[Rouslan Solomakhin]

Trying out a patch without addSpellingMarker and with ASSERT_NO_EXCEPTION.

[Hajime Morrita]

Comment on attachment 185091 [details]
Proposed patch

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

As Tony pointed, The large part of the test code is a boilerplate. Can we get rid of it?

> LayoutTests/editing/spelling/spelling-double-clicked-word-with-underscores.html:16
> +        finishJSTest();

It looks we have no "FAIL" output when the test is timed out.
Let's explicitly fail here. Same for other tests.

> Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:300
> +                        selectionRange.clear();

Nit: It looks we don't need this clear() call.

[Rouslan Solomakhin]

Created attachment 185121 [details]
Patch

[Rouslan Solomakhin]

(In reply to comment #28)
> (From update of attachment 185091 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185091&action=review
> 
> As Tony pointed, The large part of the test code is a boilerplate. Can we get rid of it?

Moved boilerplate into LayoutTests/editing/spelling/resources/util.js

> 
> > LayoutTests/editing/spelling/spelling-double-clicked-word-with-underscores.html:16
> > +        finishJSTest();
> 
> It looks we have no "FAIL" output when the test is timed out.
> Let's explicitly fail here. Same for other tests.
> 
> > Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:300
> > +                        selectionRange.clear();
> 
> Nit: It looks we don't need this clear() call.

Removed the clear() call.

[Rouslan Solomakhin]

(In reply to comment #28)
> It looks we have no "FAIL" output when the test is timed out.
> Let's explicitly fail here. Same for other tests.

Added log("FAIL: spell check timed outed") in LayoutTests/editing/spelling/resources/util.js.

[Hajime Morrita]

Comment on attachment 185121 [details]
Patch

Looks good to me. Do you want me to land this?

[Rouslan Solomakhin]

Comment on attachment 185121 [details]
Patch

Requesting to use the commit-queue

[Build Bot]

Comment on attachment 185121 [details]
Patch

Attachment 185121 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16200066

New failing tests:
editing/spelling/spelling-exactly-selected-multiple-words.html
editing/spelling/spelling-should-select-multiple-words.html
editing/spelling/spelling-with-punctuation-selection.html
editing/spelling/spelling-double-clicked-word.html
editing/spelling/spelling-should-select-single-word.html
editing/spelling/spelling-exactly-selected-word.html
editing/spelling/spelling-double-clicked-word-with-underscores.html
editing/spelling/spelling-with-whitespace-selection.html
editing/spelling/spelling-with-underscore-selection.html

[Build Bot]

Comment on attachment 185121 [details]
Patch

Attachment 185121 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16201085

New failing tests:
editing/spelling/spelling-exactly-selected-multiple-words.html
editing/spelling/spelling-should-select-multiple-words.html
editing/spelling/spelling-with-punctuation-selection.html
editing/spelling/spelling-double-clicked-word.html
editing/spelling/spelling-should-select-single-word.html
editing/spelling/spelling-exactly-selected-word.html
editing/spelling/spelling-double-clicked-word-with-underscores.html
editing/spelling/spelling-with-whitespace-selection.html
editing/spelling/spelling-with-underscore-selection.html

[Tony Chang]

My suggestion for the Mac failures is to add the tests to platform/mac/TestExpectations marked as [ Failure ].  Then when you commit, the tests will run and you can grab the results from the bots and try to figure out why they failed.

The other option is to build the Apple Mac port locally and run the tests (which you may need to do anyway to figure out why these tests are failing).

[Rouslan Solomakhin]

(In reply to comment #36)
> My suggestion for the Mac failures is to add the tests to platform/mac/TestExpectations marked as [ Failure ].  Then when you commit, the tests will run and you can grab the results from the bots and try to figure out why they failed.
> 
> The other option is to build the Apple Mac port locally and run the tests (which you may need to do anyway to figure out why these tests are failing).

I am building the Apple Mac port locally on my Mac.

[Rouslan Solomakhin]

Created attachment 185303 [details]
Patch

[Rouslan Solomakhin]

Comment on attachment 185303 [details]
Patch

The fix for layout tests on Mac is to set "win" editing behavior.

[Build Bot]

Comment on attachment 185303 [details]
Patch

Attachment 185303 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16196318

New failing tests:
editing/spelling/spelling-exactly-selected-multiple-words.html
editing/spelling/spelling-should-select-multiple-words.html
editing/spelling/spelling-with-punctuation-selection.html
editing/spelling/spelling-double-clicked-word.html
editing/spelling/spelling-should-select-single-word.html
editing/spelling/spelling-multiword-selection.html
editing/spelling/spelling-exactly-selected-word.html
editing/spelling/spelling-double-clicked-word-with-underscores.html
editing/spelling/spelling-with-whitespace-selection.html
editing/spelling/spelling-with-underscore-selection.html
editing/spelling/spelling-subword-selection.html

[Build Bot]

Comment on attachment 185303 [details]
Patch

Attachment 185303 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16195339

New failing tests:
editing/spelling/spelling-exactly-selected-multiple-words.html
editing/spelling/spelling-should-select-multiple-words.html
editing/spelling/spelling-with-punctuation-selection.html
editing/spelling/spelling-double-clicked-word.html
editing/spelling/spelling-should-select-single-word.html
editing/spelling/spelling-exactly-selected-word.html
editing/spelling/spelling-double-clicked-word-with-underscores.html
editing/spelling/spelling-with-whitespace-selection.html
editing/spelling/spelling-with-underscore-selection.html

[Rouslan Solomakhin]

The failures on Mac are due to differences in misspelling marker placement. In the word "wellcome_", for example,  the Mac port marks the text "wellcome_" as misspelled, including the underline. Other ports, however, only mark the text "wellcome", without the underline.

[Tony Chang]

(In reply to comment #42)
> The failures on Mac are due to differences in misspelling marker placement. In the word "wellcome_", for example,  the Mac port marks the text "wellcome_" as misspelled, including the underline. Other ports, however, only mark the text "wellcome", without the underline.

What happens in TextEdit on a Mac?  Safari tries to mimic the behavior of TextEdit.

[Rouslan Solomakhin]

(In reply to comment #43)
> (In reply to comment #42)
> > The failures on Mac are due to differences in misspelling marker placement. In the word "wellcome_", for example,  the Mac port marks the text "wellcome_" as misspelled, including the underline. Other ports, however, only mark the text "wellcome", without the underline.
> 
> What happens in TextEdit on a Mac?  Safari tries to mimic the behavior of TextEdit.

TextEdit also marks the text "wellcome_" as misspelled, including the underline.

[Rouslan Solomakhin]

(In reply to comment #44)
> (In reply to comment #43)
> > (In reply to comment #42)
> > > The failures on Mac are due to differences in misspelling marker placement. In the word "wellcome_", for example,  the Mac port marks the text "wellcome_" as misspelled, including the underline. Other ports, however, only mark the text "wellcome", without the underline.
> > 
> > What happens in TextEdit on a Mac?  Safari tries to mimic the behavior of TextEdit.
> 
> TextEdit also marks the text "wellcome_" as misspelled, including the underline.

Is there a setting like editing behavior that I can flip in layout tests to alter this behavior?

[Tony Chang]

(In reply to comment #45)
> (In reply to comment #44)
> > (In reply to comment #43)
> > > (In reply to comment #42)
> > > > The failures on Mac are due to differences in misspelling marker placement. In the word "wellcome_", for example,  the Mac port marks the text "wellcome_" as misspelled, including the underline. Other ports, however, only mark the text "wellcome", without the underline.
> > > 
> > > What happens in TextEdit on a Mac?  Safari tries to mimic the behavior of TextEdit.
> > 
> > TextEdit also marks the text "wellcome_" as misspelled, including the underline.
> 
> Is there a setting like editing behavior that I can flip in layout tests to alter this behavior?

Not that I know of.  It seems OK to me to check in a separate expected file for platform/mac and file a bug for the differing behavior.

[Rouslan Solomakhin]

(In reply to comment #46)
> It seems OK to me to check in a separate expected file for platform/mac and file a bug for the differing behavior.

Sounds good. Would the new bug ask for the same behavior on all platforms or for new tests on the mac platform?

[Tony Chang]

(In reply to comment #47)
> (In reply to comment #46)
> > It seems OK to me to check in a separate expected file for platform/mac and file a bug for the differing behavior.
> 
> Sounds good. Would the new bug ask for the same behavior on all platforms or for new tests on the mac platform?

I'm not sure since I'm not sure if the behavior in Safari/TextEdit is intentional or not.  Maybe Ryosuke can find out?  For now, I would just file a bug saying what the different behaviors are.

[Rouslan Solomakhin]

Created attachment 185537 [details]
Patch

[Rouslan Solomakhin]

Comment on attachment 185537 [details]
Patch

Added platform-specific expected test outputs for mac and mac-wk2 platforms.

[Rouslan Solomakhin]

Filed https://bugs.webkit.org/show_bug.cgi?id=108370 - "Different spellcheck behavior among platforms"

[Tony Chang]

Comment on attachment 185537 [details]
Patch

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

It would be helpful if on bug 108370 you listed what the differences are for each test.

> LayoutTests/platform/mac-wk2/editing/spelling/spelling-double-clicked-word-expected.txt:7
> +FAIL Incomplete test environment
> +PASS successfullyParsed is true

I see, we should skip these tests in platform/mac-wk2/TestExpectations since it doesn't implement setAsynchronousSpellCheckingEnabled.

[Rouslan Solomakhin]

Created attachment 185547 [details]
Patch

[Rouslan Solomakhin]

Comment on attachment 185547 [details]
Patch

Skipping the new spell checker tests on Mac WK2.

[Rouslan Solomakhin]

Specified the mac platform differences in bug 108370.

[Rouslan Solomakhin]

All tests passed. Can someone with correct permissions please flip the review and commit-queue flags to "+" ?

[Tony Chang]

Comment on attachment 185547 [details]
Patch

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

Some minor style nits, but this looks OK overall.  Note that the Qt, Gtk and Efl bots don't run tests, so these tests may fail on those ports too.  You can either suppress the errors in TestExpectations or land them and watch the bots on build.webkit.org.

> Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:296
> +                    // If there is no selection, then select the misspelling.

WebKit style is to avoid comments that only describe the code.  Most comments are for answering 'why' the code is there. I would remove this comment.

> Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp:76
> +        int maxWordLength = stringText.length() - wordOffset;
> +        int wordLength;

Don't you need to static_cast length() to an int?  I think some compilers will complain about this signed/unsigned conversion.  Should maxWordLength and wordLength be unsigned?

> Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp:77
> +        WTF::String word;

Nit: No WTF:: needed.

> Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp:85
> +            wordLength = m_misspelledWords.at(i).length() > maxWordLength ? maxWordLength : m_misspelledWords.at(i).length();

I'm surprised this doesn't complain about signed/unsigned either.

[Rouslan Solomakhin]

(In reply to comment #57)
> Note that the Qt, Gtk and Efl bots don't run tests, so these tests may fail on those ports too.  You can either suppress the errors in TestExpectations or land them and watch the bots on build.webkit.org.

Is there a way to make these bots run the tests?

I am hopeful that the tests will pass on those platforms. If not, we should add TestExpecations after the build bots fail.

[Tony Chang]

(In reply to comment #58)
> (In reply to comment #57)
> > Note that the Qt, Gtk and Efl bots don't run tests, so these tests may fail on those ports too.  You can either suppress the errors in TestExpectations or land them and watch the bots on build.webkit.org.
> 
> Is there a way to make these bots run the tests?

Nope.

> I am hopeful that the tests will pass on those platforms. If not, we should add TestExpecations after the build bots fail.

That's fine as well.  Just make sure you're around when the patch lands.

[Rouslan Solomakhin]

Created attachment 185594 [details]
Patch

[Rouslan Solomakhin]

Comment on attachment 185594 [details]
Patch

- Removed the unnecessary comment in ContextMenuClentImpl.cpp
- Casting length() to int in MockSpellCheck.cpp.

[Rouslan Solomakhin]

Posted this on #webkit IRC channel:

"This is a heads up: I plan to land http://webkit.org/b/106815, which might break layout test on qt, gtk, and efl. In case of breakage, I will land the appropriate changes to TestExpectations."

[Ryosuke Niwa]

Comment on attachment 185594 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:19
> +        Fix spellcheck to behave as follows:
> +        - Spellcheck selects the misspelled word on context click. 
> +        - Spelling should work when the user selects the misspelled word exactly. 
> +        - If the word is not misspelled, there should be no spelling options in the context menu. 
> +        - Spellcheck should select multi-word misspellings on context click. 
> +        - Spelling should work when the user selects a multi-word misspelling exactly. 
> +        - Spelling should work for double-clicked misspellings. 
> +        - Spelling should ignore whitespace. 
> +        - Underscores should be treated as whitespace: spelling should ignore them. 
> +        - Punctuation marks should be treated as whitespace: spelling should ignore them. 
> +        - Spelling should be disabled when user selects a part of misspelling. 
> +        - Spelling should be disabled when user selects multiple words that are not a single misspelling. 

That's a lot of bug fixes in one patch! Can we somehow break this up into smaller pieces?
In WebKit, we try to break up patches into small pieces as much as possible.

> LayoutTests/ChangeLog:19
> +        - Spellcheck selects the misspelled word on context click. 
> +        - Spelling should work when the user selects the misspelled word exactly. 
> +        - If the word is not misspelled, there should be no spelling options in the context menu. 
> +        - Spellcheck should select multi-word misspellings on context click. 
> +        - Spelling should work when the user selects a multi-word misspelling exactly. 
> +        - Spelling should work for double-clicked misspellings. 
> +        - Spelling should ignore whitespace. 
> +        - Underscores should be treated as whitespace: spelling should ignore them. 
> +        - Punctuation marks should be treated as whitespace: spelling should ignore them. 
> +        - Spelling should be disabled when user selects a part of misspelling. 
> +        - Spelling should be disabled when user selects multiple words that are not a single misspelling. 

Please add these comments after "Added." below.

[Rouslan Solomakhin]

Created attachment 185625 [details]
Patch

[Rouslan Solomakhin]

Comment on attachment 185625 [details]
Patch

Trying to attach this patch to https://bugs.webkit.org/show_bug.cgi?id=108405...

[Rouslan Solomakhin]

(In reply to comment #63)
> That's a lot of bug fixes in one patch! Can we somehow break this up into smaller pieces?

Filed https://webkit.org/b/108405 and attached LayoutTests there. The tests are marked as [Skip] in TestExpectations for all platforms for now.

[Rouslan Solomakhin]

(In reply to comment #66)
> (In reply to comment #63)
> > That's a lot of bug fixes in one patch! Can we somehow break this up into smaller pieces?
> 
> Filed https://webkit.org/b/108405 and attached LayoutTests there. The tests are marked as [Skip] in TestExpectations for all platforms for now.

Filed https://webkit.org/b/108498 (Add two-word misspelling to mock spellchecker) and attached the patch.

[Rouslan Solomakhin]

Filed https://webkit.org/b/108509 (Select two-word misspelling on context-click) and attached the patch.

[Rouslan Solomakhin]

Thank you for all your help!