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.
Changed the patch in bug 106815 to skip the new spell checker tests on Mac WK2 platform, because it does not implement setAsynchronousSpellCheckingEnabled.
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.
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.
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.
I'll defer to Tony then. But aren't we _modifying_ that behavior for Chromium?
That's true, some behavior on Chromium is different from other ports. Some tests should go into platform/chromium.
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?
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.
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.
(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.
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?
(In reply to comment #11) > Sounds like the right plan? SGTM.
Created attachment 191329 [details] Patch
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.
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.
(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?
Skipping editing/spelling/spellcheck-async-mutation.html editing/spelling/spellcheck-async.html editing/spelling/spellcheck-sequencenum.html editing/style/iframe-onload-crash-mac.html
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.
(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?