Bug 108370

Summary:

Some spellcheck tests require asynchronous spellcheck

Product:

WebKit

Reporter:

Rouslan Solomakhin <rouslan+webkit>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

UNCONFIRMED

Severity:

Normal

CC:

ap, enrica, groby, jiapu.mail, jochen, jonlee, morrita, rniwa, rouslan+webkit, sam, simon.fraser, tony

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

112362, 122414

Bug Blocks:

Attachments:

Description	Flags
Patch	tony: review-

Rouslan Solomakhin

Reported 2013-01-30 12:32:52 PST

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.

Attachments
Patch (75.28 KB, patch) 2013-03-04 15:40 PST, Rouslan Solomakhin	tony: review-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Rouslan Solomakhin

Comment 1 2013-01-30 13:20:14 PST

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

Comment 2 2013-01-30 13:38:00 PST

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

Comment 3 2013-02-22 13:43:10 PST

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

Comment 4 2013-02-22 13:53:28 PST

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

Comment 5 2013-02-22 13:54:33 PST

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

Rouslan Solomakhin

Comment 6 2013-02-22 13:57:20 PST

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

Tony Chang

Comment 7 2013-02-22 15:13:44 PST

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

Comment 8 2013-02-22 15:37:42 PST

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

Comment 9 2013-02-22 15:59:19 PST

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

Comment 10 2013-02-22 16:08:25 PST

(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

Comment 11 2013-02-22 16:49:48 PST

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

Comment 12 2013-02-22 17:13:30 PST

(In reply to comment #11) > Sounds like the right plan? SGTM.

Rouslan Solomakhin

Comment 13 2013-03-04 15:40:46 PST

Created attachment 191329 [details] Patch

Rouslan Solomakhin

Comment 14 2013-03-04 15:43:16 PST

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 15 2013-03-04 16:21:44 PST

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

Comment 16 2013-03-04 16:26:16 PST

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

Comment 17 2013-03-18 14:12:36 PDT

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 18 2013-03-18 14:14:56 PDT

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

Comment 19 2013-03-18 14:52:06 PDT

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

Note You need to log in before you can comment on or make changes to this bug.