Bug 52221 - Autocorrection should respect undo.
Summary: Autocorrection should respect undo.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on: 53255
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-11 09:34 PST by Jia Pu
Modified: 2011-02-15 15:44 PST (History)
11 users (show)

See Also:


Attachments
Proposed patch (v1) (140.93 KB, patch)
2011-01-11 10:38 PST, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v2) (140.93 KB, patch)
2011-01-11 11:01 PST, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v3) (140.93 KB, patch)
2011-01-11 13:54 PST, Jia Pu
no flags Details | Formatted Diff | Diff
patch preview. (54.89 KB, patch)
2011-02-01 13:28 PST, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v4) (91.20 KB, patch)
2011-02-09 16:26 PST, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v5) (91.45 KB, patch)
2011-02-10 08:45 PST, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v6) (91.48 KB, patch)
2011-02-10 10:02 PST, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v7) (91.48 KB, patch)
2011-02-10 10:56 PST, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v8) (93.57 KB, patch)
2011-02-10 17:45 PST, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v9) (411.67 KB, patch)
2011-02-14 23:08 PST, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v10) (412.66 KB, patch)
2011-02-14 23:48 PST, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v11) (414.52 KB, patch)
2011-02-15 12:45 PST, Jia Pu
darin: 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 2011-01-11 09:34:05 PST
Steps to produce:

Type "NSDocument attempts to put a good revision in the genstore"
This is automatically (and incorrectly) corrected to 'gemstone'.
Cmd-z to undo this change.
Type another character, e.g. space.
genstore is corrected again to gemstone.
Cmd-z to undo this again
Use arrow keys to move past word
Type another character, e.g. f
genstore is again corrected to gemstone.

<rdar://problem/8663399>
Comment 1 Jia Pu 2011-01-11 10:38:33 PST
Created attachment 78548 [details]
Proposed patch (v1)
Comment 2 WebKit Review Bot 2011-01-11 10:42:04 PST
Attachment 78548 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7353164
Comment 3 Build Bot 2011-01-11 10:58:42 PST
Attachment 78548 [details] did not build on win:
Build output: http://queues.webkit.org/results/7402131
Comment 4 Jia Pu 2011-01-11 11:01:17 PST
Created attachment 78554 [details]
Proposed patch (v2)

Fixed build failure on non-Mac platforms.
Comment 5 mitz 2011-01-11 11:02:42 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=78548&action=review

> WebCore/editing/Editor.cpp:1125
> +    RefPtr<Range> range = Range::create(document, newSelection.start(), newSelection.end());
> +    // If the text is inserted by undo, it should be exempt from autocorrection.
> +    // This is especially important when user reverts an autocorrection by undo.
> +    document->markers()->addMarker(range.get(), DocumentMarker::Replacement);
> +#endif

Is this going to make all text inserted by undo marked as replacement? Are those marks ever cleared?

> WebCore/rendering/InlineTextBox.cpp:1033
> +                // Other markers are for intenal annotation, and don't require update on UI.

typo: “intenal”
Comment 6 Early Warning System Bot 2011-01-11 11:14:15 PST
Attachment 78548 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7404147
Comment 7 Jia Pu 2011-01-11 13:54:39 PST
Created attachment 78592 [details]
Proposed patch (v3)

Fixed a typo in comment.
Comment 8 Jia Pu 2011-01-11 14:07:33 PST
(In reply to comment #5)
> View in context: https://bugs.webkit.org/attachment.cgi?id=78548&action=review
> 
> > WebCore/editing/Editor.cpp:1125
> > +    RefPtr<Range> range = Range::create(document, newSelection.start(), newSelection.end());
> > +    // If the text is inserted by undo, it should be exempt from autocorrection.
> > +    // This is especially important when user reverts an autocorrection by undo.
> > +    document->markers()->addMarker(range.get(), DocumentMarker::Replacement);
> > +#endif
> 
> Is this going to make all text inserted by undo marked as replacement? Are those marks ever cleared?

Yes, the marker will be added to all inserted text. And they will not be cleared. 

The replacement mark is used to prevent text to be checked for spelling. The general on AppKit side is that we try not to re-correct what user has reverted. So if the undo'ed text contains misspelled word, we don't check it again. And if it doesn't contain misspelled word, we don't need to check it anyway.

However, having this discussion made me realize that this may not be the right place for adding marker, since the command we are undoing here may not modify text. It could simply be font change or something.

> 
> > WebCore/rendering/InlineTextBox.cpp:1033
> > +                // Other markers are for intenal annotation, and don't require update on UI.
> 
> typo: “intenal”

Fixed in new patch.
Comment 9 Jia Pu 2011-02-01 13:28:58 PST
Created attachment 80814 [details]
patch preview.
Comment 10 Jia Pu 2011-02-01 13:42:32 PST
Attachment 80814 [details] is a preview of proposed patch, since the patch will depend on the landing of patch for bug 53255. However, I'd like to get some feedback on the proposed changes.

This patch consists of following components:

1. To identify edit command that is generated by autocorrection, I added a new member variable EditCommand::m_createdBySpellCorrection. 

2. When undoing a command that is generated by autocorrection, we add Replacement and SpellCheckingExemption markers to undone range. This is achieved by calling Editor::unappliedSpellCorrection().

3. In order to avoid having undone range selected, I have created two new edit commands, SetSelectionCommand and ReplaceRangeCommand. SetSelectionCommand does nothing more than setting the selection. And ReplaceRangeCommand is a composition of SetSelectionCommand followed by ReplaceSelectionCommand.

4. Clean up the signatures of SelectionController::setSelection() and ReplaceSelectionCommand::ReplaceSelectionCommand(), since those bool arguments are become too difficult to follow. I have consolidated those bool arguments into enum bi masks.
Comment 11 Jia Pu 2011-02-09 16:26:26 PST
Created attachment 81893 [details]
Proposed patch (v4)
Comment 12 WebKit Review Bot 2011-02-09 16:42:17 PST
Attachment 81893 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7875081
Comment 13 Build Bot 2011-02-09 17:05:06 PST
Attachment 81893 [details] did not build on win:
Build output: http://queues.webkit.org/results/7870070
Comment 14 Early Warning System Bot 2011-02-09 17:24:32 PST
Attachment 81893 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7884070
Comment 15 Collabora GTK+ EWS bot 2011-02-09 20:38:18 PST
Attachment 81893 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7870130
Comment 16 Jia Pu 2011-02-10 08:45:41 PST
Created attachment 81986 [details]
Proposed patch (v5)

Fixed compiler error on some platforms.
Comment 17 Jia Pu 2011-02-10 10:02:00 PST
Created attachment 81997 [details]
Proposed patch (v6)

Resolved a conflict with TOT.
Comment 18 WebKit Review Bot 2011-02-10 10:17:16 PST
Attachment 81997 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7867388
Comment 19 Collabora GTK+ EWS bot 2011-02-10 10:17:53 PST
Attachment 81997 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7870379
Comment 20 Early Warning System Bot 2011-02-10 10:21:35 PST
Attachment 81997 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7869393
Comment 21 Build Bot 2011-02-10 10:36:44 PST
Attachment 81997 [details] did not build on win:
Build output: http://queues.webkit.org/results/7867392
Comment 22 Jia Pu 2011-02-10 10:56:59 PST
Created attachment 82006 [details]
Proposed patch (v7)

More compiler error fix.
Comment 23 WebKit Review Bot 2011-02-10 11:17:18 PST
Attachment 82006 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7869410
Comment 24 Collabora GTK+ EWS bot 2011-02-10 11:33:19 PST
Attachment 82006 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7874397
Comment 25 Build Bot 2011-02-10 12:07:14 PST
Attachment 82006 [details] did not build on win:
Build output: http://queues.webkit.org/results/7870407
Comment 26 Early Warning System Bot 2011-02-10 16:00:36 PST
Attachment 82006 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7868465
Comment 27 Jia Pu 2011-02-10 17:45:24 PST
Created attachment 82079 [details]
Proposed patch (v8)

More compiler error fix.
Comment 28 Darin Adler 2011-02-14 10:36:53 PST
Comment on attachment 82079 [details]
Proposed patch (v8)

View in context: https://bugs.webkit.org/attachment.cgi?id=82079&action=review

> Source/WebCore/editing/Editor.cpp:2415
> +#endif
> +            if (selectionToReplace != m_frame->selection()->selection()) {

There should be a blank line here.

> Source/WebCore/editing/Editor.cpp:3524
> +        bool doSpellGrammarChecking = true;
> +#if SUPPORT_AUTOCORRECTION_PANEL
> +        doSpellGrammarChecking = !(options & SelectionController::SpellCorrectionTriggered);
> +#endif // SUPPORT_AUTOCORRECTION_PANEL

The comment on the endif here is not helpful. I think there are less confusing ways to write this. I would name the boolean something more like shouldDoSpellingAndGrammerChecking. Naming a boolean with a verb phrase is not great.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:352
> +ReplaceSelectionCommand::ReplaceSelectionCommand(Document* document, PassRefPtr<DocumentFragment> fragment, CommandOptions options, EditAction editAction)
>      : CompositeEditCommand(document),
> -      m_selectReplacement(selectReplacement), 
> -      m_smartReplace(smartReplace),
> -      m_matchStyle(matchStyle),
> -      m_documentFragment(fragment),
> -      m_preventNesting(preventNesting),
> -      m_movingParagraph(movingParagraph),
> -      m_editAction(editAction),
> -      m_shouldMergeEnd(false)
> +    m_selectReplacement(options & SelectReplacement),
> +    m_smartReplace(options & SmartReplace),
> +    m_matchStyle(options & MatchStyle),
> +    m_documentFragment(fragment),
> +    m_preventNesting(options & PreventNesting),
> +    m_movingParagraph(options & MovingParagraph),
> +    m_editAction(editAction),
> +    m_shouldMergeEnd(false)

Neither the old nor the new code matches the normal WebKit style, which is:

    : CompositeEditCommand(document)
    , m_selectReplacement(options & SelectReplacement)

And so on.

> Source/WebCore/editing/ReplaceSelectionCommand.h:47
> +    enum CommandFlag {
> +        SelectReplacement = 1 << 0,
> +        SmartReplace = 1 << 1,
> +        MatchStyle = 1 << 2,
> +        PreventNesting = 1 << 3,
> +        MovingParagraph = 1 << 4
> +    };
> +
> +    typedef unsigned CommandOptions;

It would be better to name this enum and typedef so it was clearer they match. With one named flag and the other options, it’s a bit unclear.

> Source/WebCore/editing/SelectionController.h:63
> +    enum SetSelectionFlags {
> +        CloseTyping = 1 << 0,
> +        ClearTypingStyle = 1 << 1,
> +        UserTriggered = 1 << 2,
> +        SpellCorrectionTriggered = 1 << 3,
> +    };
> +    typedef unsigned SetSelectionOptions;

Same flags vs. options issue here.

> Source/WebCore/editing/SetSelectionCommand.cpp:58
> +void SetSelectionCommand::doApply()
> +{
> +    SelectionController* selectionController = document()->frame()->selection();
> +    ASSERT(selectionController);
> +
> +    if (selectionController->shouldChangeSelection(m_selectionToSet)) {
> +        selectionController->setSelection(m_selectionToSet, m_options);
> +        setEndingSelection(m_selectionToSet);
> +    }
> +}
> +
> +void SetSelectionCommand::doUnapply()
> +{
> +    SelectionController* selectionController = document()->frame()->selection();
> +    ASSERT(selectionController);
> +
> +    if (selectionController->shouldChangeSelection(startingSelection()))
> +        selectionController->setSelection(startingSelection(), m_options);
> +}

I think this command needs more checks in its undo and redo code paths. Commands like this need to work even if all the nodes involved in the original selection are no longer in the document or if they have been moved to unusual places. Most simple commands have checks for this type of thing, such as the checks you’ll see in commands like RemoveNodeCommand.

I believe that both starting selection and m_selectionToSet will still point to nodes even if they are no longer appropriate selection endpoints so this won’t be sufficiently bulletproof.

> Source/WebCore/editing/SetSelectionCommand.h:42
> +    SetSelectionCommand(const VisibleSelection&,  SelectionController::SetSelectionOptions);

Extra space after comma here.

> Source/WebCore/editing/SpellingCorrectionCommand.cpp:49
> +    SpellingCorrectionRecordUndoCommand(Document* document, const String& corrected, const String& correction)
> +    : SimpleEditCommand(document)
> +    , m_corrected(corrected)
> +    , m_correction(correction)
> +    {
> +    }

Member initialization is supposed to be indented in WebKit style.

> Source/WebCore/editing/SpellingCorrectionCommand.cpp:58
> +    virtual void doApply()
> +    {
> +    }
> +
> +    virtual void doUnapply()
> +    {
> +        document()->frame()->editor()->unappliedSpellCorrection(startingSelection(), m_corrected, m_correction);
> +    }

Needs a comment for why doApply isn’t needed. Does this work OK with redo?

> Source/WebCore/editing/SpellingCorrectionCommand.cpp:89
> +}
> +} // namespace WebCore

Need a blank line here.

> Source/WebCore/editing/SpellingCorrectionCommand.h:49
> +};
> +} // namespace WebCore

Need a blank line here.
Comment 29 Jia Pu 2011-02-14 23:08:58 PST
Created attachment 82419 [details]
Proposed patch (v9)

Updated patch according comment 28.
Comment 30 WebKit Review Bot 2011-02-14 23:23:54 PST
Attachment 82419 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7908821
Comment 31 Early Warning System Bot 2011-02-14 23:29:55 PST
Attachment 82419 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7912739
Comment 32 Jia Pu 2011-02-14 23:48:56 PST
Created attachment 82423 [details]
Proposed patch (v10)
Comment 33 Build Bot 2011-02-14 23:58:21 PST
Attachment 82419 [details] did not build on win:
Build output: http://queues.webkit.org/results/7909836
Comment 34 Jia Pu 2011-02-15 00:04:23 PST
(In reply to comment #28)
> (From update of attachment 82079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82079&action=review

> > Source/WebCore/editing/SetSelectionCommand.cpp:58
> > +void SetSelectionCommand::doApply()
> > +{
> > +    SelectionController* selectionController = document()->frame()->selection();
> > +    ASSERT(selectionController);
> > +
> > +    if (selectionController->shouldChangeSelection(m_selectionToSet)) {
> > +        selectionController->setSelection(m_selectionToSet, m_options);
> > +        setEndingSelection(m_selectionToSet);
> > +    }
> > +}
> > +
> > +void SetSelectionCommand::doUnapply()
> > +{
> > +    SelectionController* selectionController = document()->frame()->selection();
> > +    ASSERT(selectionController);
> > +
> > +    if (selectionController->shouldChangeSelection(startingSelection()))
> > +        selectionController->setSelection(startingSelection(), m_options);
> > +}
> 
> I think this command needs more checks in its undo and redo code paths. Commands like this need to work even if all the nodes involved in the original selection are no longer in the document or if they have been moved to unusual places. Most simple commands have checks for this type of thing, such as the checks you’ll see in commands like RemoveNodeCommand.
> 
> I believe that both starting selection and m_selectionToSet will still point to nodes even if they are no longer appropriate selection endpoints so this won’t be sufficiently bulletproof.

I have add check to make sure the selection isn't orphan.


> > Source/WebCore/editing/SpellingCorrectionCommand.cpp:49
> > +    SpellingCorrectionRecordUndoCommand(Document* document, const String& corrected, const String& correction)
> > +    : SimpleEditCommand(document)
> > +    , m_corrected(corrected)
> > +    , m_correction(correction)
> > +    {
> > +    }
> 
> Member initialization is supposed to be indented in WebKit style.
> 
> > Source/WebCore/editing/SpellingCorrectionCommand.cpp:58
> > +    virtual void doApply()
> > +    {
> > +    }
> > +
> > +    virtual void doUnapply()
> > +    {
> > +        document()->frame()->editor()->unappliedSpellCorrection(startingSelection(), m_corrected, m_correction);
> > +    }
> 
> Needs a comment for why doApply isn’t needed. Does this work OK with redo?

This command is really Mac specific. I have put in #if/#endif, and added comment to explain.

In the latest patch, I also had to rebaseline a few tests whose pixel tests have been broken probably by some recent changes.
Comment 35 WebKit Commit Bot 2011-02-15 11:12:21 PST
Comment on attachment 82423 [details]
Proposed patch (v10)

Rejecting attachment 82423 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2

Last 500 characters of output:
ource/WebKit/mac/WebView/WebFrame.mm
patching file Source/WebKit2/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h
Hunk #1 succeeded at 145 (offset 5 lines).
patching file Source/WebKit2/WebProcess/WebCoreSupport/mac/WebEditorClientMac.mm
patching file Source/WebKit2/WebProcess/WebPage/WebPage.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7908986
Comment 36 Jia Pu 2011-02-15 11:33:30 PST
Hmm, hard to tell from log where patching failed. I can only imagine it's on xcode project file. Should I create a new patch against TOT?
Comment 37 Darin Adler 2011-02-15 12:20:33 PST
(In reply to comment #36)
> Hmm, hard to tell from log where patching failed. I can only imagine it's on xcode project file. Should I create a new patch against TOT?

Yes.
Comment 38 Jia Pu 2011-02-15 12:45:26 PST
Created attachment 82507 [details]
Proposed patch (v11)

Same as previous patch. But it is generated against more recent TOT to resolve conflict.
Comment 39 Jia Pu 2011-02-15 12:51:34 PST
I noticed that patching often fails on efl. I hope that's a separate issue.
Comment 40 WebKit Commit Bot 2011-02-15 15:24:43 PST
Comment on attachment 82507 [details]
Proposed patch (v11)

Rejecting attachment 82507 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2

Last 500 characters of output:
eSupport/WebEditorClient.mm
patching file Source/WebKit/mac/WebView/WebFrame.mm
patching file Source/WebKit2/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h
patching file Source/WebKit2/WebProcess/WebCoreSupport/mac/WebEditorClientMac.mm
patching file Source/WebKit2/WebProcess/WebPage/WebPage.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7912937
Comment 41 Darin Adler 2011-02-15 15:27:11 PST
Comment on attachment 82507 [details]
Proposed patch (v11)

I tried to apply the patch and it failed on Android.mk.
Comment 42 Darin Adler 2011-02-15 15:27:57 PST
(In reply to comment #41)
> I tried to apply the patch and it failed on Android.mk.

LOL, wrong patch. Sorry!
Comment 43 Darin Adler 2011-02-15 15:30:33 PST
I’ll work on landing this.
Comment 44 Douglas Davidson 2011-02-15 15:33:41 PST
Thanks, Darin.
Comment 45 Jia Pu 2011-02-15 15:34:06 PST
(In reply to comment #43)
> I’ll work on landing this.

Thanks, Darin. If the issue is on project file. All you need to do is to add SetSelectionCommand.h/.cpp and SpellingCorrectionCommand.h/.cpp into the project.
Comment 46 Darin Adler 2011-02-15 15:44:23 PST
Committed r78632: <http://trac.webkit.org/changeset/78632>