Bug 24953 – Add automatic spell correction support in WebKit

• Component: HTML Editing
• Status: RESOLVED FIXED

[Siddhartha Chattopadhyay]

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.

[Siddhartha Chattopadhyay]

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

Adding Justin who is much more familiar with this part of the editing code than I.

[Justin Garcia]

-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]

Finally, where are the changes to mac/WebCoreSupport/WebEditorClient.mm?

[Siddhartha Chattopadhyay]

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 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 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]

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]

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 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 on attachment 29450 [details]
Support for automatic spelling corrections

Clearing review flag to remove from commit queue.

[Siddhartha Chattopadhyay]

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]

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]

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]

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]

                 // 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]

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

I tested this patch out on my Mac, and I didn't observe any layout test regressions.

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/43980

I accidentally removed the review flag from the patch. It has been reviewed.