Bug 25330 – Implement text checking/autocorrection with new SnowLeopard text checking API

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Justin Garcia]

Implement text checking/autocorrection with new SnowLeopard text checking API

<rdar://problem/6724106>

[Justin Garcia]

Created attachment 29688 [details]
patch

[Douglas Davidson]

Created attachment 29750 [details]
new patch taking into account suggestions from Justin

Following Justin's suggestions, I've simplified "resultType" to "type" and used an enum for the values.

[Darin Adler]

Comment on attachment 29750 [details]
new patch taking into account suggestions from Justin

Setting review? for Doug.

[Eric Seidel (no email)]

Comment on attachment 29750 [details]
new patch taking into account suggestions from Justin

Mostly style complaints:

I would think the Editor stuff should be in its own EditorMac.cpp file, since none of the rest of the ports care about it.

WebKit style normally uses:
if (!ec) instead:
+            if (ec == 0) {
Personally I find (ec == 0) more readable in some cases, but we should follow the style guidelines here.

     if (client()->substitutionsPanelIsShowing()) {
 1154         client()->showSubstitutionsPanel(false);
 1155     } else {

no { } according to the style guidelines.

Why is this one different from all the others:
2     if (!client())
 1163         return false;
 1164     return client()->substitutionsPanelIsShowing();

return client() && ... would fit the pattern.

Even though much of the editing code hasn't been updated to the modern style guidelines, we still should try to follow them here:
 2227 void Editor::markAllMisspellingsAndBadGrammarInRanges(Range *spellingRange, bool markGrammar, Range *grammarRange, bool performTextChecking)

for C++ * goes next to the type in WebKit.  In Obj-C the * goes next to the variable name (to match the rest of AppKit/Cocoa)

Looks like you have some tabs in the file:
 2273 		restoreSelectionAfterChange = true;
 2274 		adjustSelectionForParagraphBoundaries = (selectionOffset >= paragraphLength) ? true : false;
 2275 	    }
the commit will fail if there are tabs in the file.

I would think this whole block would make more sense in a small function:
 uint64_t checkingTypes = markGrammar ? (TextCheckingTypeSpelling | TextCheckingTypeGrammar) : TextCheckingTypeSpelling;
 2281     if (performTextChecking) {
 2282         if (isAutomaticLinkDetectionEnabled())
 2283

something like uint64_t checkingTypes =  typesToCheck(markGrammar) or some better name...

It looks like you changed the encoding of the strings file?

Seems TextCheckingResult result; could really use a constructor function :)

This should all be a static inline funtion:
    if (action == @selector(toggleAutomaticQuoteSubstitution:)) {
 2649         NSMenuItem *menuItem = (NSMenuItem *)item;
 2650         if ([menuItem isKindOfClass:[NSMenuItem class]])
 2651             [menuItem setState:[self isAutomaticQuoteSubstitutionEnabled] ? NSOnState : NSOffState];
 2652         return YES;
 2653     }
cause you repeat the same copy/paste code a bunch there...

AGain here:
} else if (action == @selector(toggleAutomaticLinkDetection:)) {
 3767         BOOL checkMark = [self isAutomaticLinkDetectionEnabled];
 3768         if ([(NSObject *)item isKindOfClass:[NSMenuItem class]]) {
 3769             NSMenuItem *menuItem = (NSMenuItem *)item;
 3770             [menuItem setState:checkMark ? NSOnState : NSOffState];
 3771         }
 3772         return YES;
 3773     } 

And here:
4655     if (automaticDashSubstitutionEnabled == flag)
 4656         return;
 4657     automaticDashSubstitutionEnabled = flag;
 4658     [[NSUserDefaults standardUserDefaults] setBool:automaticDashSubstitutionEnabled forKey:WebAutomaticDashSubstitutionEnabled];    
 4659     [[NSSpellChecker sharedSpellChecker] updatePanels];

I think you're going to need to go more rounds with Darin/Justin (folks who actually still work at Apple).   But hopefully you'll find the style comments useful.

[Douglas Davidson]

Much of this is Mac-specific now, but I would hope we'll be able to change that in the future.

The substitutionsPanel code you mention is the way it is because it is copied from the existing spellingPanel code elsewhere in the file.  I'll fix the extra braces by making that code match too.

I'll move the asterisks and remove the tabs.  Damn Xcode started putting in tabs when I told it not to.

The encoding of the .strings file is still UTF-16; I'm not sure why these diffs are showing up.

The repetitive copy/paste code is there to match corresponding existing code; I didn't really feel comfortable changing existing patterns, but if Darin or Justin want we can do it.

[Douglas Davidson]

I see what happened with the .strings file, and I've fixed it.  I still haven't been able to get the update-webkit-localizable-strings script to work.

[Douglas Davidson]

Created attachment 29761 [details]
new patch to improve conformance to style guidelines

[Justin Garcia]

 about to give this a look thanks for your patience

[Justin Garcia]

doug why the bit about storing the caret offset in order to restore the caret after the operation? are we not always one character after the autocorrection?  can't we then just rely on the selection set by the replacement operation + 1?  I guess autocorrection might kick in when we submit a form and in that case we're not + 1...

[Douglas Davidson]

Yeah, I tried letting the selection be set by the replacement but there were cases where it didn't work.  You really want it to end up where it was before the replacement operations took place, taking into account any change in length the replacements may have caused.  I'd be delighted to find a simpler way of restoring the selection but I haven't come up with one.

[Justin Garcia]

(In reply to comment #10)
> Yeah, I tried letting the selection be set by the replacement but there were
> cases where it didn't work.  You really want it to end up where it was before
> the replacement operations took place, taking into account any change in length
> the replacements may have caused.  I'd be delighted to find a simpler way of
> restoring the selection but I haven't come up with one.

what you did is fine although I'd like to know the cases where the replacement failed to restore a selection as they are bugs.

[Justin Garcia]

Comment on attachment 29761 [details]
new patch to improve conformance to style guidelines

+    if (performTextChecking) {
+        if (m_frame->selection()->selectionType() == VisibleSelection::CaretSelection) {
+            RefPtr<Range> offsetAsRange = Range::create(paragraphRange->startContainer(ec)->document(), paragraphRange->startPosition(), paragraphRange->startPosition());
+            offsetAsRange->setEnd(m_frame->selection()->end().containerNode(), m_frame->selection()->end().computeOffsetInContainerNode(), ec);
+            if (!ec) {
+                selectionOffset = TextIterator::rangeLength(offsetAsRange.get());
+                restoreSelectionAfterChange = true;         
+                adjustSelectionForParagraphBoundaries = (selectionOffset >= paragraphLength) ? true : false;
+            }
+        }
+    }

You only need to do this if there will be a replacement, but perhaps it would be cumbersome to find that out here/do this later once we've found that we'll be doing a replacement.


+ offsetAsRange->setEnd(m_frame->selection()->end().containerNode(), m_frame->selection()->end().computeOffsetInContainerNode(), ec)

Would look nice split out:

+ Position caretPosition = m_frame->selection()->end();
+ offsetAsRange->setEnd(caretPosition.containerNode(), caretPosition.computeOffsetInContainerNode(), ec);


"performTextChecking"

Is there any way we can make this bool more clearly describe that we're doing full text checking, not just spelling/grammar checking?  spelling/grammar checking sound like they'd also obey "performTextChecking".


+ } else if (performTextChecking && resultLocation + resultLength <= spellingRangeEndOffset && resultLocation + resultLength >= spellingRangeStartOffset &&

Should the check instead be resultLocation >= spellingRangeStartOffset?

I guess I'm slightly confused about what spellingRangeStartOffset actually is.  It looks like (in the !markGrammar case at least) to be the offset into the containing paragraph of the start of the spellingRange passed to this function.  Is that right?


+ RefPtr<Range> replacementRange = TextIterator::subrange(paragraphRange.get(), resultLocation, resultLength);
+ VisibleSelection replacementSelection(replacementRange.get(), DOWNSTREAM);

I think better names might be rangeToReplace and selectionToReplace.


+ } else {
+    m_frame->selection()->moveTo(m_frame->selection()->end());
+    m_frame->selection()->modify(SelectionController::MOVE, SelectionController::FORWARD, CharacterGranularity);
+ }

This else case deserves a comment I think.


+void Editor::markMisspellingsAfterTypingToPosition(const VisiblePosition &p)

Did you need to move this.


 void TypingCommand::typingAddedToOpenCommand()
 {
-    markMisspellingsAfterTyping();
     document()->frame()->editor()->appliedEditing(this);
+    markMisspellingsAfterTyping();
 }

I think remember you explaining this somewhere but I couldn't find it.


enum TextCheckingType { TextCheckingTypeSpelling = 1 << 1, TextCheckingTypeGrammar = 1 << 2, TextCheckingTypeLink = 1 << 5, TextCheckingTypeQuote = 1 << 6, TextCheckingTypeDash = 1 << 7, TextCheckingTypeReplacement = 1 << 8, TextCheckingTypeCorrection = 1 << 9 };

Please split this out onto separate lines like the other enum definitions in WebCore.


#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD)

Could we get a BUILDING_ON_SNOWLEOPARD?


Otherwise, r=me.

(In reply to comment #12)

> Could we get a BUILDING_ON_SNOWLEOPARD?

Not until Snow Leopard is in "the past".

[Justin Garcia]

What's up with this:

-    // FIXME 4811447: workaround for lack of API
+#ifndef BUILDING_ON_LEOPARD
+    [[NSSpellChecker sharedSpellChecker] updatePanels];
+#else
     NSSpellChecker *spellChecker = [NSSpellChecker sharedSpellChecker];
     if ([spellChecker respondsToSelector:@selector(_updateGrammar)])
         [spellChecker performSelector:@selector(_updateGrammar)];
+#endif

[Douglas Davidson]

I will split out the code you point out, and find a more descriptive name than "performTextChecking".  

Your understanding about the spellingRangeStartOffset is correct; the condition for doing the various replacements is that the end of the region being replaced should touch the spelling range.  This is because the region being replaced may be inter-word punctuation such as dashes and quotes.

rangeToReplace and selectionToReplace are reasonable names.  I'll add comments and split the enum.  The reason for changing the order of markMisspellingsAfterTyping() is to make sure that changes are ordered properly for undoing.

The updatePanels is new API that we have that makes it unnecessary to use the old SPI.  We can split that out into a separate fix if you want.

[Justin Garcia]

> Your understanding about the spellingRangeStartOffset is correct; the condition
> for doing the various replacements is that the end of the region being replaced
> should touch the spelling range.  This is because the region being replaced may
> be inter-word punctuation such as dashes and quotes.

Ah I understand now.  A comment there would be great.

> The reason for changing the order of markMisspellingsAfterTyping() is to make 
> sure that changes are ordered properly.

Ah of course.  Please add a comment.

> The updatePanels is new API that we have that makes it unnecessary to use the
> old SPI.  We can split that out into a separate fix if you want.

Up to you, either way please mention it in the ChangeLog.

Thanks!

[Douglas Davidson]

Created attachment 29779 [details]
new patch taking into account suggestions from Justin

[Adele Peterson]

Committed revision 42911.