Bug 108370 – Some spellcheck tests require asynchronous spellcheck

• Component: HTML Editing
• Status: UNCONFIRMED

[Rouslan Solomakhin]

The patch for https://bugs.webkit.org/show_bug.cgi?id=106815 adds new tests for spellcheck behavior. The mac and mac-wk2 platforms differ in spellcheck behavior. The patch for 106815 adds platform-specific expected test outputs. (See the expected test outputs for the specific differences.)

Platform mac-wk2:
LayoutTests/platform/mac-wk2/editing/spelling/spelling-double-clicked-word-expected.txt
LayoutTests/platform/mac-wk2/editing/spelling/spelling-double-clicked-word-with-underscores-expected.txt
LayoutTests/platform/mac-wk2/editing/spelling/spelling-exactly-selected-multiple-words-expected.txt
LayoutTests/platform/mac-wk2/editing/spelling/spelling-exactly-selected-word-expected.txt
LayoutTests/platform/mac-wk2/editing/spelling/spelling-multiword-selection-expected.txt
LayoutTests/platform/mac-wk2/editing/spelling/spelling-should-select-multiple-words-expected.txt
LayoutTests/platform/mac-wk2/editing/spelling/spelling-should-select-single-word-expected.txt
LayoutTests/platform/mac-wk2/editing/spelling/spelling-subword-selection-expected.txt
LayoutTests/platform/mac-wk2/editing/spelling/spelling-with-punctuation-selection-expected.txt
LayoutTests/platform/mac-wk2/editing/spelling/spelling-with-underscore-selection-expected.txt
LayoutTests/platform/mac-wk2/editing/spelling/spelling-with-whitespace-selection-expected.txt

Platform mac:
LayoutTests/platform/mac/editing/spelling/spelling-double-clicked-word-expected.txt
LayoutTests/platform/mac/editing/spelling/spelling-double-clicked-word-with-underscores-expected.txt
LayoutTests/platform/mac/editing/spelling/spelling-exactly-selected-multiple-words-expected.txt
LayoutTests/platform/mac/editing/spelling/spelling-exactly-selected-word-expected.txt
LayoutTests/platform/mac/editing/spelling/spelling-should-select-multiple-words-expected.txt
LayoutTests/platform/mac/editing/spelling/spelling-should-select-single-word-expected.txt
LayoutTests/platform/mac/editing/spelling/spelling-with-punctuation-selection-expected.txt
LayoutTests/platform/mac/editing/spelling/spelling-with-underscore-selection-expected.txt
LayoutTests/platform/mac/editing/spelling/spelling-with-whitespace-selection-expected.txt

We need to either make spellcheck behave the same among all platforms or add platform-specific tests for mac and mac-wk2.

[Rouslan Solomakhin]

Changed the patch in bug 106815 to skip the new spell checker tests on Mac WK2 platform, because it does not implement setAsynchronousSpellCheckingEnabled.

[Rouslan Solomakhin]

Mac platform behavior:

[spelling-double-clicked-word.html]
Double-clicked misspelling does not show spelling suggestions. Other platforms show spelling suggestions for double-clicked misspellings.

[spelling-double-clicked-word-with-underscores.html]
[spelling-with-underscore-selection.html]
The spelling marker applies to the text "wellcome_" including the trailing underscore. Other platforms do not include the trailing underscore in the misspelling.

[spelling-exactly-selected-multiple-words.html]
[spelling-should-select-multiple-words.html]
Spellcheck times out for the two-word misspelling 'upper case'. Other platforms do not time out.

[spelling-exactly-selected-word.html]
Exactly selected misspelling does not show spelling suggestions. Other platforms show spelling suggestion for exactly-selected misspellings.

[spelling-should-select-single-word.html]
Context click on the misspelled word does not select misspelled word. Other platforms select misspelled word on context click.

[spelling-with-whitespace-selection.html]
Spellcheck is disabled for selection that includes whitespace. The patch for bug 106815 changes Chromium spellcheck behavior to ignore whitespace.

[spelling-with-punctuation-selection.html]
Spellcheck is disabled for selection that includes punctuation. The patch for bug 106815 changes Chromium spellcheck behavior to treat punctuation as whitespace and ignore it.

[Rachel Blum]

Can we move those to platform-chromium-linux and platform-chromium-win instead? Or platform-chromium, and manage via TestExpectations?

And in general, all tests that are chromium-specific should go to platform-chromium. The should not impact platform-mac/platform-mac-wk2. Or any of the other platforms.

[Rouslan Solomakhin]

Rachel: (In reply to comment #3)
> Can we move those to platform-chromium-linux and platform-chromium-win instead? Or platform-chromium, and manage via TestExpectations?
> 
> And in general, all tests that are chromium-specific should go to platform-chromium. The should not impact platform-mac/platform-mac-wk2. Or any of the other platforms.

Tony originally recommended that we put these tests into editing/spelling/unified-async folder and skip this folder and skip this folder on platforms that do not yet implement unified text checking and async spell checking.

[Rachel Blum]

I'll defer to Tony then. But aren't we _modifying_ that behavior for Chromium?

[Rouslan Solomakhin]

That's true, some behavior on Chromium is different from other ports. Some tests should go into platform/chromium.

[Tony Chang]

It's nice to put the tests in a cross port location because we're not testing Chromium specific behavior (are we?).

The fix is only in Chromium, but I imagine ideally the fix would be for all ports.

Another reason is that long term, I would imagine we want to move this code from WebKit/chromium into WebCore if it is cross platform behavior.

Maybe some of these tests are Chromium specific behavior?

[Rachel Blum]

These tests are for behavior we would like to see in Chromium. IIRC, they deviate from e.g. OSX normal behavior for the sake of making spellcheck work better. (rouslan, please correct me if I got this wrong)

I am not sure other ports will necessarily want to adopt this.

[Rouslan Solomakhin]

Some of the behaviors deviate from other platforms. For example, if you select "helllo_" together with underscore in Chrome and right-click on the selection, then Chrome will show the suggestion "hello". Other platforms would not show spelling suggestions. We changed this because we noticed that many users double-clicked words before context clicking them. Double-clicking words on Windows selects the trailing underscore.

[Tony Chang]

(In reply to comment #9)
> Some of the behaviors deviate from other platforms. For example, if you select "helllo_" together with underscore in Chrome and right-click on the selection, then Chrome will show the suggestion "hello". Other platforms would not show spelling suggestions. We changed this because we noticed that many users double-clicked words before context clicking them. Double-clicking words on Windows selects the trailing underscore.

Something like this I would expect to be a cross platform test because testing on GTK+ (gedit) and Qt (kate) shows suggestions.  Also, if we explicitly don't want suggestions on Mac, then having the test be in a shared location with the different expect result checked in makes that more clear.

If this code was in WebCore, you could use EditingBehavior.h to control which platforms get which behaviors.

[Rouslan Solomakhin]

I plann to move all of my  spell-checking tests to editing/spelling/unified-async and exclude this directory from other platforms using TestExpecations. (I assume TestExpecations knows how to handle directories.)

Then other platforms that implement unified async spellcheck can turn on these tests when necessary. Also adding new tests under editing/spelling/unified-async will not require changing test expectations for all of the platforms.

I also plan to close the following bugs as WontFix, because other platforms should enable tests for editing/spelling/unified-async only when/if they implement unified and async spell check:
 - https://bugs.webkit.org/show_bug.cgi?id=108528 - [Efl] Enable spellchecking tests added in r141354
 - https://bugs.webkit.org/show_bug.cgi?id=108529 - [Gtk] Enable spellchecking tests added in r141354
 - https://bugs.webkit.org/show_bug.cgi?id=108530 - [Qt] Enable spellchecking tests added in r141354
 - https://bugs.webkit.org/show_bug.cgi?id=108535 - [Win] Enable spellchecking tests added in r141354
 - https://bugs.webkit.org/show_bug.cgi?id=108536 - [Wincairo] Enable spellchecking tests added in r141354
 - https://bugs.webkit.org/show_bug.cgi?id=110526 - Change spelling tests from [ Skip ] to [ Failure ]
 - https://bugs.webkit.org/show_bug.cgi?id=108525 - [Mac] Enable spellchecking tests added in r141471

Sounds like the right plan?

[Tony Chang]

(In reply to comment #11)

> Sounds like the right plan?

SGTM.

[Rouslan Solomakhin]

Created attachment 191329 [details]
Patch

[Rouslan Solomakhin]

Comment on attachment 191329 [details]
Patch

As we discussed, it's best to modify the tests to work in all settings. This patch changes the tests to check for features before using them. In the process of changing the tests, I found that certain configurations of spellchecker do not work in Chromium. The patch includes the fix for that, too.

[Tony Chang]

Comment on attachment 191329 [details]
Patch

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

It would be better to split this into 2 patches: 1 patch that refactors sync and async to share code and 1 patch to change the tests.  It's hard to read the diff due to code moving around.

> Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.cpp:232
> +    bindMethod("setContinuousSpellCheckingEnabled", &TestRunner::setContinuousSpellCheckingEnabled);

We should implement this in Internals.{h,cpp,idl} so all the ports can share this.  Maybe we should do a patch to move setAsynchronousSpellCheckingEnabled to Internals first.

[Rouslan Solomakhin]

(In reply to comment #15)
> (From update of attachment 191329 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191329&action=review
> 
> It would be better to split this into 2 patches: 1 patch that refactors sync and async to share code and 1 patch to change the tests.  It's hard to read the diff due to code moving around.

Sounds good. I will split up the patch.

 
> > Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.cpp:232
> > +    bindMethod("setContinuousSpellCheckingEnabled", &TestRunner::setContinuousSpellCheckingEnabled);
> 
> We should implement this in Internals.{h,cpp,idl} so all the ports can share this.  Maybe we should do a patch to move setAsynchronousSpellCheckingEnabled to Internals first.

Ryosuke: Would you be okay with moving setAsynchronousSpellCheckingEnabled and setContinuousSpellCheckingEnabled to Internals?

[Simon Fraser (smfr)]

Skipping
editing/spelling/spellcheck-async-mutation.html
editing/spelling/spellcheck-async.html
editing/spelling/spellcheck-sequencenum.html
editing/style/iframe-onload-crash-mac.html

[Ryosuke Niwa]

Comment on attachment 191329 [details]
Patch

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

>>> Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.cpp:232
>>> +    bindMethod("setContinuousSpellCheckingEnabled", &TestRunner::setContinuousSpellCheckingEnabled);
>> 
>> We should implement this in Internals.{h,cpp,idl} so all the ports can share this.  Maybe we should do a patch to move setAsynchronousSpellCheckingEnabled to Internals first.
> 
> Ryosuke: Would you be okay with moving setAsynchronousSpellCheckingEnabled and setContinuousSpellCheckingEnabled to Internals?

It would be strange since only Chromium and EFL ports support asynchronous spellchecking, and it's not something you would switch at runtime.

[Rouslan Solomakhin]

(In reply to comment #18)
> (From update of attachment 191329 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191329&action=review
> 
> >>> Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.cpp:232
> >>> +    bindMethod("setContinuousSpellCheckingEnabled", &TestRunner::setContinuousSpellCheckingEnabled);
> >> 
> >> We should implement this in Internals.{h,cpp,idl} so all the ports can share this.  Maybe we should do a patch to move setAsynchronousSpellCheckingEnabled to Internals first.
> > 
> > Ryosuke: Would you be okay with moving setAsynchronousSpellCheckingEnabled and setContinuousSpellCheckingEnabled to Internals?
> 
> It would be strange since only Chromium and EFL ports support asynchronous spellchecking, and it's not something you would switch at runtime.

https://bugs.webkit.org/show_bug.cgi?id=112362 moved setAsynchronousSpellCheckingEnabled() to Internals already. Should we rollback that change?