Bug 24953 - Add automatic spell correction support in WebKit
Summary: Add automatic spell correction support in WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Siddhartha Chattopadhyay
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-30 17:07 PDT by Siddhartha Chattopadhyay
Modified: 2009-05-21 16:26 PDT (History)
3 users (show)

See Also:


Attachments
Patch (16.58 KB, patch)
2009-03-30 17:42 PDT, Siddhartha Chattopadhyay
justin.garcia: review-
Details | Formatted Diff | Diff
Support for automatic spelling corrections (19.38 KB, patch)
2009-04-13 17:53 PDT, Siddhartha Chattopadhyay
no flags Details | Formatted Diff | Diff
New Patch (19.13 KB, patch)
2009-04-28 15:09 PDT, Siddhartha Chattopadhyay
no flags Details | Formatted Diff | Diff
Updated patch to take care of the caret position (19.24 KB, patch)
2009-05-05 16:02 PDT, Siddhartha Chattopadhyay
no flags Details | Formatted Diff | Diff
New Patch according to Douglas's suggestions (15.90 KB, patch)
2009-05-08 17:12 PDT, Siddhartha Chattopadhyay
no flags Details | Formatted Diff | Diff
New Patch (15.87 KB, patch)
2009-05-11 12:09 PDT, Siddhartha Chattopadhyay
no flags Details | Formatted Diff | Diff
Patch 6 (16.33 KB, patch)
2009-05-14 12:04 PDT, Siddhartha Chattopadhyay
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Siddhartha Chattopadhyay 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.
Comment 1 Siddhartha Chattopadhyay 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).
Comment 2 Eric Seidel (no email) 2009-03-30 18:26:24 PDT
Adding Justin who is much more familiar with this part of the editing code than I.
Comment 3 Justin Garcia 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.
Comment 4 Justin Garcia 2009-04-07 22:15:01 PDT
Finally, where are the changes to mac/WebCoreSupport/WebEditorClient.mm?
Comment 5 Siddhartha Chattopadhyay 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.
Comment 6 Eric Seidel (no email) 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.
Comment 7 Justin Garcia 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
Comment 8 Justin Garcia 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.
Comment 9 Siddhartha Chattopadhyay 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.
Comment 10 Eric Seidel (no email) 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).
Comment 11 Eric Seidel (no email) 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.
Comment 12 Siddhartha Chattopadhyay 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.
Comment 13 Siddhartha Chattopadhyay 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.
Comment 14 Siddhartha Chattopadhyay 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.
Comment 15 Siddhartha Chattopadhyay 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.
Comment 16 Justin Garcia 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?
Comment 17 Siddhartha Chattopadhyay 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.
Comment 18 Justin Garcia 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
Comment 19 Darin Fisher (:fishd, Google) 2009-05-21 13:43:18 PDT
I tested this patch out on my Mac, and I didn't observe any layout test regressions.
Comment 20 Darin Fisher (:fishd, Google) 2009-05-21 14:03:38 PDT
Landed as:  http://trac.webkit.org/changeset/43980
Comment 21 mitz 2009-05-21 16:26:42 PDT
I accidentally removed the review flag from the patch. It has been reviewed.