Bug 44958 – Add support for autocorrection UI on Mac OS X.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jia Pu]

Add support for autocorrection UI that is used by other text editing UI on Mac OS X, so that the experience of text editing in WebView is consistent with the rest of the OS.

[Jia Pu]

Created attachment 66075 [details]
Proposed patch

[Early Warning System Bot]

Attachment 66075 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3883181

[WebKit Review Bot]

Attachment 66075 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3873209

[WebKit Review Bot]

Attachment 66075 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3877182

[Jia Pu]

Created attachment 66092 [details]
Proposed patch

Second version of the patch, which resolves build failure on non-macosx platforms.

[Early Warning System Bot]

Attachment 66092 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3853181

[Jia Pu]

Created attachment 66118 [details]
Proposed patch (v3)

Fixed more build issues on non-mac platforms.

Comment on attachment 66118 [details]
Proposed patch (v3)

> WebCore/ChangeLog:9
> +        No new tests. (OOPS!)
We cannot check in change log entries with the word OOPS in them. Please remove this line. 

> WebCore/ChangeLog:11
> +        Several new member methods are added to EditorClient for communication between WebCore and WebKit. A new handler, executeCancelOperation(), is added to EditorCommand.cpp so that WebCore can intercept the ESC key event to dismiss autocorrection UI. A new DocumentMarker value, RejectedCorrection, is added to keep track of the corrections that user has rejected, so that it will not be suggested again later. The autocorrection is driven by a timer. Every time the editor inserts a new letter, the timer is reset. If the timer fires, it means neither has user entered any new letter for current word, nor has he entered whitespace or punctuation to complete the word. In this case, we query for autocorrection.
Please hard-wrap this at the 80-100 column mark.

> WebCore/ChangeLog:19
> +        (WebCore::Editor::~Editor): Make sure autocorrection UI is dismiss before destroying Editor object.
typo: dismiss -> dismissed

> WebCore/ChangeLog:21
> +        (WebCore::Editor::markAllMisspellingsAndBadGrammarInRanges): Added an argument to indicate whether we want to show autocorrection UI. Added code to show autocorrection UI if it is necessary. Replaced all four boolean arguments with enum to improve readability.
I think a single enum-based bitfield would have been even better. Please wrap this long line.

> WebCore/editing/Editor.cpp:2373
> +        markAllMisspellingsAndBadGrammarInRanges(MarkSpelling, adjacentWords.toNormalizedRange().get(), markGrammar, selectedSentence.toNormalizedRange().get(), performTextCheckingReplacements, DoNotShowCorrectionPanel);
Might be better to use MarkGrammar instead of markGrammar here.

> WebCore/editing/Editor.cpp:2685
> +                        // We only the show correction panel on the last word.
typo: the show -> show the

> WebCore/editing/Editor.cpp:2701
> +            if (doReplacement) {
I think I may have misled you about this in an earlier review, but it seems like this block could now be entered in cases where previously it wasn’t, because the result-type check and the canEdit() and shouldInsertText() checks aren’t applied here.

> WebCore/editing/Editor.cpp:2720
> +    if (selectionChanged && !shouldShowCorrectionPanel) {
Can you explain this change? How could selectionChanged be true if we are just showing the correction UI?

> WebCore/editing/Editor.h:351
> +    void correctionPanelTimerFired(Timer<Editor>* timer);
Please omit the name “timer” here.

> WebCore/platform/graphics/GraphicsContext.h:276
> +            TextCheckingInvalidLineStyle = -1,
The invalid value is not needed.

> WebCore/platform/graphics/GraphicsContext.h:284
> +        void drawLineForTextChecking(const IntPoint&, int width, TextCheckingLineStyle);
> +#else
>          void drawLineForMisspellingOrBadGrammar(const IntPoint&, int width, bool grammar);
> -
> +#endif
It’s really bad the have different GraphicsContext methods for Mac and all the other ports. It shouldn’t be hard to blindly change all other ports to adopt the new method.

> WebCore/rendering/InlineTextBox.cpp:785
> +#if PLATFORM(MAC)
> +static GraphicsContext::TextCheckingLineStyle textCheckingLineStyleForMarkerType(DocumentMarker::MarkerType markerType)
> +{
> +    switch (markerType) {
> +    case DocumentMarker::Spelling:
> +        return GraphicsContext::TextCheckingSpellingLineStyle;
> +    case DocumentMarker::Grammar:
> +        return GraphicsContext::TextCheckingGrammarLineStyle;
> +    case DocumentMarker::Replacement:
> +        return GraphicsContext::TextCheckingReplacementLineStyle;
> +    default:
> +        return GraphicsContext::TextCheckingInvalidLineStyle;
> +    }
> +}
> +#endif
In the default case you should ASSERT_NOT_REACHED() and return one of the valid values. It is a programming error to call this method with anything besides Spelling, Geammar or Replacement.

I r-ed because of the change log issues and what I think is a logic mistake in markAllMisspellingsAndBadGrammarInRanges().

[WebKit Review Bot]

Attachment 66092 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3880217

[Jia Pu]

Created attachment 66168 [details]
Proposed patch (v4)

Code changes based on comment #8.

[WebKit Review Bot]

Attachment 66168 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jia Pu]

Created attachment 66169 [details]
Proposed patch (v5)

code change based on comment #8.

[Early Warning System Bot]

Attachment 66168 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3958007

[Jia Pu]

Created attachment 66171 [details]
Proposed patch (v6)

Code changes based on comment #8.
Fixed a build failure on InlineTextBox.cpp.

[WebKit Review Bot]

Attachment 66169 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3901016

[Jia Pu]

> 
> > WebCore/ChangeLog:21
> > +        (WebCore::Editor::markAllMisspellingsAndBadGrammarInRanges): Added an argument to indicate whether we want to show autocorrection UI. Added code to show autocorrection UI if it is necessary. Replaced all four boolean arguments with enum to improve readability.
> I think a single enum-based bitfield would have been even better. Please wrap this long line.

Replaced all boolean arguments with a bitfield.


> > WebCore/editing/Editor.cpp:2701
> > +            if (doReplacement) {
> I think I may have misled you about this in an earlier review, but it seems like this block could now be entered in cases where previously it wasn’t, because the result-type check and the canEdit() and shouldInsertText() checks aren’t applied here.

Yes, I should have reviewed your review more closely. :)

> 
> > WebCore/platform/graphics/GraphicsContext.h:284
> > +        void drawLineForTextChecking(const IntPoint&, int width, TextCheckingLineStyle);
> > +#else
> >          void drawLineForMisspellingOrBadGrammar(const IntPoint&, int width, bool grammar);
> > -
> > +#endif
> It’s really bad the have different GraphicsContext methods for Mac and all the other ports. It shouldn’t be hard to blindly change all other ports to adopt the new method.

This is indeed unwieldy given the different platforms we need to build on. I have replaced all calls to drawLineForMisspellingOrBadGrammar() with drawLineForTextChecking().

I think this was closed by accident.

[Jia Pu]

(In reply to comment #18)
> I think this was closed by accident.

Ah, yes, I was confused by the interface. Thanks for correcting this.

Comment on attachment 66171 [details]
Proposed patch (v6)

One last comment:

> +    enum TextCheckingOptions {
> +        MarkSpelling = 1 << 0,
> +        MarkGrammar = 1 << 1,
> +        PerformReplacement = 1 << 2,
> +        ShowCorrectionPanel = 1 << 3,
> +    };
> +    void markAllMisspellingsAndBadGrammarInRanges(int textCheckingOptions, Range* spellingRange, Range* grammarRange);

The way these flags enums usually work in WebKit is that in addition to an enum for the flags, there is also a typedef for the bitmask, which is used as the parameter type:

    enum TextCheckingOptionFlags {
        MarkSpelling = 1 << 0,
        MarkGrammar = 1 << 1,
        PerformReplacement = 1 << 2,
        ShowCorrectionPanel = 1 << 3,
    };
    typedef unsigned TextCheckingOptions;

    void markAllMisspellingsAndBadGrammarInRanges(TextCheckingOptions, Range* spellingRange, Range* grammarRange);

[Jia Pu]

Created attachment 66241 [details]
Proposed patch (v7)

Added typedef for the bit mask per comment #20.

Comment on attachment 66241 [details]
Proposed patch (v7)

r=me

Please file follow-up bugs on the things we’ve discussed.

[Eric Seidel (no email)]

Comment on attachment 66241 [details]
Proposed patch (v7)

Marking this cq+ since jpu is not a committer.

[WebKit Commit Bot]

Comment on attachment 66241 [details]
Proposed patch (v7)

Rejecting patch 66241 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 66241]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=66241&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=44958&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 66241 from bug 44958.
Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Dan Bernstein', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4432032

[Jia Pu]

(In reply to comment #24)
> (From update of attachment 66241 [details])
> Rejecting patch 66241 from commit-queue.
> 
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 66241]" exit_code: 2
> Cleaning working directory
> Updating working directory
> Logging in as [e-mail]...
> Fetching: https://bugs.webkit.org/attachment.cgi?id=66241&action=edit
> Fetching: https://bugs.webkit.org/show_bug.cgi?id=44958&ctype=xml
> Processing 1 patch from 1 bug.
> Processing patch 66241 from bug 44958.
> Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Dan Bernstein', u'--force']" exit_code: 1
> 
> Full output: http://queues.webkit.org/results/4432032

Dan, I thought this patch has been landed long time ago.

[Eric Seidel (no email)]

It probably was, the bug was just never updated or closed.