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
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.
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
Created attachment 184581 [details] Proposed patch
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.
Created attachment 184588 [details] Proposed patch
Comment on attachment 184588 [details] Proposed patch Attachment 184588 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16097512
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
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
Comment on attachment 184588 [details] Proposed patch Attachment 184588 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16114215
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.
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.
Hironori is also a good person to do an informal review of this.
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.
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
(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.
(In reply to comment #15) > three subdirectories four, that is.
Comment on attachment 184808 [details] Proposed patch Attachment 184808 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/16123486
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
Comment on attachment 184808 [details] Proposed patch Attachment 184808 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16113612
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.
Created attachment 184825 [details] Proposed patch Added symbol exports. Want to see if this fixes any of the build bots.
(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.
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
Comment on attachment 184825 [details] Proposed patch Attachment 184825 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16111885
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
Created attachment 185091 [details] Proposed patch
Trying out a patch without addSpellingMarker and with ASSERT_NO_EXCEPTION.
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.
Created attachment 185121 [details] Patch
(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.
(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.
Comment on attachment 185121 [details] Patch Looks good to me. Do you want me to land this?
Comment on attachment 185121 [details] Patch Requesting to use the commit-queue
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
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
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).
(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.
Created attachment 185303 [details] Patch
Comment on attachment 185303 [details] Patch The fix for layout tests on Mac is to set "win" editing behavior.
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
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
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.
(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.
(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.
(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?
(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.
(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?
(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.
Created attachment 185537 [details] Patch
Comment on attachment 185537 [details] Patch Added platform-specific expected test outputs for mac and mac-wk2 platforms.
Filed https://bugs.webkit.org/show_bug.cgi?id=108370 - "Different spellcheck behavior among platforms"
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.
Created attachment 185547 [details] Patch
Comment on attachment 185547 [details] Patch Skipping the new spell checker tests on Mac WK2.
Specified the mac platform differences in bug 108370.
All tests passed. Can someone with correct permissions please flip the review and commit-queue flags to "+" ?
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.
(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.
(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.
Created attachment 185594 [details] Patch
Comment on attachment 185594 [details] Patch - Removed the unnecessary comment in ContextMenuClentImpl.cpp - Casting length() to int in MockSpellCheck.cpp.
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."
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.
Created attachment 185625 [details] Patch
Comment on attachment 185625 [details] Patch Trying to attach this patch to https://bugs.webkit.org/show_bug.cgi?id=108405...
(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.
(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.
Filed https://webkit.org/b/108509 (Select two-word misspelling on context-click) and attached the patch.
Thank you for all your help!