RESOLVED FIXED 24953
Add automatic spell correction support in WebKit
https://bugs.webkit.org/show_bug.cgi?id=24953
Summary Add automatic spell correction support in WebKit
Siddhartha Chattopadhyay
Reported 2009-03-30 17:07:28 PDT
Add support in WebKit for automatic spelling corrections, so that misspelled words typed in a text-box area get automatically correct if autocorrection word is provided.
Attachments
Patch (16.58 KB, patch)
2009-03-30 17:42 PDT, Siddhartha Chattopadhyay
justin.garcia: review-
Support for automatic spelling corrections (19.38 KB, patch)
2009-04-13 17:53 PDT, Siddhartha Chattopadhyay
no flags
New Patch (19.13 KB, patch)
2009-04-28 15:09 PDT, Siddhartha Chattopadhyay
no flags
Updated patch to take care of the caret position (19.24 KB, patch)
2009-05-05 16:02 PDT, Siddhartha Chattopadhyay
no flags
New Patch according to Douglas's suggestions (15.90 KB, patch)
2009-05-08 17:12 PDT, Siddhartha Chattopadhyay
no flags
New Patch (15.87 KB, patch)
2009-05-11 12:09 PDT, Siddhartha Chattopadhyay
no flags
Patch 6 (16.33 KB, patch)
2009-05-14 12:04 PDT, Siddhartha Chattopadhyay
no flags
Siddhartha Chattopadhyay
Comment 1 2009-03-30 17:42:36 PDT
Created attachment 29100 [details] Patch This is a patch for automatic spell correction support in WebKit. This patch essentially replaces the previous (misspelled) word typed by a correct word only if an auto-correct word is provided - nothing happens if auto-correct word provided is empty. The method to actually compute the corrected word is NotImplemented in WebKit. This patch is a requirement for Chromium to add an auto spell correction feature (http://code.google.com/p/chromium/issues/detail?id=7624).
Eric Seidel (no email)
Comment 2 2009-03-30 18:26:24 PDT
Adding Justin who is much more familiar with this part of the editing code than I.
Justin Garcia
Comment 3 2009-04-07 22:13:11 PDT
-static String findFirstMisspellingInRange(EditorClient* client, Range* searchRange, int& firstMisspellingOffset, bool markAll) +static String findFirstMisspellingInRange(EditorClient* client, Range* searchRange, int& firstMisspellingOffset, bool markAll, int& firstMisspellingCharCount) { Why do we need to introduce firstMisspellingCharCount? Can't callers just check the length of the returned string? + Frame* frame = document()->frame(); + if (frame) + frame->editor()->markMisspellingsAfterTypingToPosition(p1, firstMisspelledCharOffset, firstMisspelledCharCount); + + // Autocorrect the misspelled word. + if (firstMisspelledCharCount > 0 && frame) { Not sure what the surrounding code does but can frame really be null here? If it really can and we need to check it it would be nice if you used early returns in the event that the frame is null, to avoid so many levels of if nesting. + WebCore::VisibleSelection selection = VisibleSelection(startOfWord(p1, LeftWordIfOnBoundary), endOfWord(p1, RightWordIfOnBoundary)); You're already inside the WebCore namespace, you don't need the WebCore:: + // Find the exact selection of the misspelled word. + WebCore::VisibleSelection selection = VisibleSelection(startOfWord(p1, LeftWordIfOnBoundary), endOfWord(p1, RightWordIfOnBoundary)); + RefPtr<Range> searchRange(selection.toNormalizedRange()); + RefPtr<Range> misspellingRange = TextIterator::subrange(searchRange.get(), firstMisspelledCharOffset, firstMisspelledCharCount); + + // Get the misspelled word. + const String misspelledWord = plainText(misspellingRange.get()); This is work that we've already done when we call markMisspellingsAfterTypingToPosition. Inside findFirstMisspellingInRange we have a) the characters for the misspelled word and b) the misspelling range. frame->editor()->client()->getAutoCorrectSuggestionForMisspelledWord(misspelledWord, autocorrectedString); Why not have this function return the autocorrected string? + frame->editor()->replaceSelectionWithText(autocorrectedString, true, true); I don't think you want to select the autocorrected word. The idea I think is that autocorrect should not be disruptive, the user should be able to continue typing without stopping.
Justin Garcia
Comment 4 2009-04-07 22:15:01 PDT
Finally, where are the changes to mac/WebCoreSupport/WebEditorClient.mm?
Siddhartha Chattopadhyay
Comment 5 2009-04-13 17:53:18 PDT
Created attachment 29450 [details] Support for automatic spelling corrections Thank you for your detailed reviews. I made changes as per your suggestions. Please have a look again.
Eric Seidel (no email)
Comment 6 2009-04-22 11:15:26 PDT
Comment on attachment 29450 [details] Support for automatic spelling corrections Drive-by nit: 1394 // Remember first encountered misspelling range. 1395 if (!firstMisspelling) { 1396 firstMisspelling = String(chars + misspellingLocation, misspellingLength); 1397 firstMisspellingRange = misspellingRange; 1398 } is indented incorrectly.
Justin Garcia
Comment 7 2009-04-22 11:42:24 PDT
Comment on attachment 29450 [details] Support for automatic spelling corrections #endif // Editor_h + + + Index: WebCore/editing/TypingCommand.cpp ======= Looks like you accidently added some unnecessary newlines to the end of this file. - // If the selection starts inside a table, just insert the paragraph separator normally - // Breaking the blockquote would also break apart the table, which is unecessary when inserting a newline - if (enclosingNodeOfType(endingSelection().start(), &isTableStructureNode)) { - insertParagraphSeparator(); - return; - } - Please remove this before you check in. If it's a workaround for a bug you're seeing please file it. - if (p1 != p2) - document()->frame()->editor()->markMisspellingsAfterTypingToPosition(p1); + if (p1 != p2) { + RefPtr<Range> misspellingRange; + Frame* frame = document()->frame(); Would be nice if you turned this into an early return to avoid the extra level of if-nesting. + // Autocorrect the misspelled word. + if (misspellingRange.get() != NULL) { + // Get the misspelled word. In WebCore the convention is to use != 0, and you don't need the .get() here. And again, you could turn this into an early return. + frame->editor()->replaceSelectionWithText(autocorrectedString, false, true); You actually don't need smart replace here, but it won't matter because smart replace will only do anything if the selection being replaced is between is flanked by non-whitespace, which isn't the case when replacing a word. Your use of it makes me think that smart replace needs to be renamed or commented better. Other than that r=me
Justin Garcia
Comment 8 2009-04-22 12:50:17 PDT
Hold off for now, we're about to post a patch that does generalized text checking. Handles autocorrection of misspelled words, automatic URLification, etc.
Siddhartha Chattopadhyay
Comment 9 2009-04-28 15:09:41 PDT
Created attachment 29865 [details] New Patch Thanks for your reviews, Justin. I saw that the other patch (https://bugs.webkit.org/show_bug.cgi?id=25330) has been checked in. In this latest patch, I made the changes that you suggested. If this patch is alright with you, I will be obliged if you could please submit it, as I do not have commit access.
Eric Seidel (no email)
Comment 10 2009-04-29 13:18:39 PDT
Comment on attachment 29450 [details] Support for automatic spelling corrections Marking this patch obsolete, since it seems this is not ready for commit (since there is a new version posted).
Eric Seidel (no email)
Comment 11 2009-04-29 14:56:44 PDT
Comment on attachment 29450 [details] Support for automatic spelling corrections Clearing review flag to remove from commit queue.
Siddhartha Chattopadhyay
Comment 12 2009-05-04 15:53:27 PDT
Hold off on this patch - please dont commit this yet. New changes to WebKit has introduced a new behavior when autocorrected word causes the caret to overlook the space typed after typing a word. I will upload a new patch for your review. Thank you very much.
Siddhartha Chattopadhyay
Comment 13 2009-05-05 16:02:29 PDT
Created attachment 30039 [details] Updated patch to take care of the caret position Updated the patch slightly to now place the caret in the correct position after the previous word formed has been auto-replaced (if required). If this patch is alright with you, I will be obliged if you could please commit it, since I do not have commit assess.
Siddhartha Chattopadhyay
Comment 14 2009-05-08 17:12:47 PDT
Created attachment 30148 [details] New Patch according to Douglas's suggestions Updated the patch as per Douglas's suggestions. If this patch is alright with you, I will be obliged if you could please commit it.
Siddhartha Chattopadhyay
Comment 15 2009-05-11 12:09:59 PDT
Created attachment 30196 [details] New Patch Removed the argument name "misspelledWord" from getAutoCorrectSuggestionForMisspelledWord in WebCore/loader/EmptyClients.h to enable compilation in Snow Leopard.
Justin Garcia
Comment 16 2009-05-13 19:46:28 PDT
// Remember first-encountered misspelling and its offset if (!firstMisspelling) { - firstMisspellingOffset = currentChunkOffset + misspellingLocation; - firstMisspelling = String(chars + misspellingLocation, misspellingLength); + firstMisspellingOffset = currentChunkOffset + misspellingLocation; } I think you should move all of this... // Mark this instance if we're marking all instances. Otherwise bail out because we found the first one. if (!markAll) break; - // Compute range of misspelled word + // Compute range of misspelled word + RefPtr<Range> misspellingRange = TextIterator::subrange(searchRange, currentChunkOffset + misspellingLocation, misspellingLength); - + + // Remember first encountered misspelling range. + if (!firstMisspelling) { + firstMisspelling = String(chars + misspellingLocation, misspellingLength); + firstMisspellingRange = misspellingRange; + } to here. Also, this is not new with your patch (but it's relevant)...I am perplexed by the placement of: if (!markAll) break; It seems to break before we mark the misspelling. Shouldn't it break after?
Siddhartha Chattopadhyay
Comment 17 2009-05-14 12:04:35 PDT
Created attachment 30349 [details] Patch 6 Yes, the break seems early. I have moved it after the first misspelling is marked, and have also made changes as per your suggestions. Please have a look again.
Justin Garcia
Comment 18 2009-05-20 12:32:00 PDT
Comment on attachment 30349 [details] Patch 6 + Reviewed by Justin Garcia. + + Add automatic spell correction support in WebKit Looks like you've got tabs here. Moving the !markAll bailout definitely seems like the right thing to do, but maybe we're missing something subtle that is covered by a layout test. Did you run the layout tests on a mac? Other than that, r=me
Darin Fisher (:fishd, Google)
Comment 19 2009-05-21 13:43:18 PDT
I tested this patch out on my Mac, and I didn't observe any layout test regressions.
Darin Fisher (:fishd, Google)
Comment 20 2009-05-21 14:03:38 PDT
mitz
Comment 21 2009-05-21 16:26:42 PDT
I accidentally removed the review flag from the patch. It has been reviewed.
Note You need to log in before you can comment on or make changes to this bug.