Bug 44958 - Add support for autocorrection UI on Mac OS X.
Summary: Add support for autocorrection UI on Mac OS X.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 45070 45071
  Show dependency treegraph
 
Reported: 2010-08-31 09:08 PDT by Jia Pu
Modified: 2010-10-14 11:56 PDT (History)
9 users (show)

See Also:


Attachments
Proposed patch (48.04 KB, patch)
2010-08-31 11:05 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (48.13 KB, patch)
2010-08-31 13:21 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v3) (47.97 KB, patch)
2010-08-31 14:56 PDT, Jia Pu
mitz: review-
Details | Formatted Diff | Diff
Proposed patch (v4) (55.57 KB, patch)
2010-08-31 22:33 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v5) (55.56 KB, patch)
2010-08-31 22:43 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v6) (55.54 KB, patch)
2010-08-31 23:18 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v7) (55.63 KB, patch)
2010-09-01 11:56 PDT, Jia Pu
mitz: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jia Pu 2010-08-31 09:08:18 PDT
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.
Comment 1 Jia Pu 2010-08-31 11:05:44 PDT
Created attachment 66075 [details]
Proposed patch
Comment 2 Early Warning System Bot 2010-08-31 11:30:46 PDT
Attachment 66075 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3883181
Comment 3 WebKit Review Bot 2010-08-31 13:06:11 PDT
Attachment 66075 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3873209
Comment 4 WebKit Review Bot 2010-08-31 13:16:52 PDT
Attachment 66075 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3877182
Comment 5 Jia Pu 2010-08-31 13:21:01 PDT
Created attachment 66092 [details]
Proposed patch

Second version of the patch, which resolves build failure on non-macosx platforms.
Comment 6 Early Warning System Bot 2010-08-31 13:54:12 PDT
Attachment 66092 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3853181
Comment 7 Jia Pu 2010-08-31 14:56:02 PDT
Created attachment 66118 [details]
Proposed patch (v3)

Fixed more build issues on non-mac platforms.
Comment 8 mitz 2010-08-31 16:16:19 PDT
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.
Comment 9 mitz 2010-08-31 16:18:00 PDT
I r-ed because of the change log issues and what I think is a logic mistake in markAllMisspellingsAndBadGrammarInRanges().
Comment 10 WebKit Review Bot 2010-08-31 16:49:30 PDT
Attachment 66092 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3880217
Comment 11 Jia Pu 2010-08-31 22:33:21 PDT
Created attachment 66168 [details]
Proposed patch (v4)

Code changes based on comment #8.
Comment 12 WebKit Review Bot 2010-08-31 22:36:13 PDT
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.
Comment 13 Jia Pu 2010-08-31 22:43:28 PDT
Created attachment 66169 [details]
Proposed patch (v5)

code change based on comment #8.
Comment 14 Early Warning System Bot 2010-08-31 23:01:05 PDT
Attachment 66168 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3958007
Comment 15 Jia Pu 2010-08-31 23:18:12 PDT
Created attachment 66171 [details]
Proposed patch (v6)

Code changes based on comment #8.
Fixed a build failure on InlineTextBox.cpp.
Comment 16 WebKit Review Bot 2010-09-01 00:34:59 PDT
Attachment 66169 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3901016
Comment 17 Jia Pu 2010-09-01 06:38:43 PDT
> 
> > 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().
Comment 18 mitz 2010-09-01 07:13:57 PDT
I think this was closed by accident.
Comment 19 Jia Pu 2010-09-01 09:00:40 PDT
(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 20 mitz 2010-09-01 09:16:58 PDT
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);
Comment 21 Jia Pu 2010-09-01 11:56:05 PDT
Created attachment 66241 [details]
Proposed patch (v7)

Added typedef for the bit mask per comment #20.
Comment 22 mitz 2010-09-01 13:17:27 PDT
Comment on attachment 66241 [details]
Proposed patch (v7)

r=me

Please file follow-up bugs on the things we’ve discussed.
Comment 23 Eric Seidel (no email) 2010-10-14 11:35:32 PDT
Comment on attachment 66241 [details]
Proposed patch (v7)

Marking this cq+ since jpu is not a committer.
Comment 24 WebKit Commit Bot 2010-10-14 11:37:29 PDT
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 commit-queue@webkit.org...
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
Comment 25 Jia Pu 2010-10-14 11:51:01 PDT
(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 commit-queue@webkit.org...
> 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.
Comment 26 Eric Seidel (no email) 2010-10-14 11:56:47 PDT
It probably was, the bug was just never updated or closed.