Bug 52221 – Autocorrection should respect undo.

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

[Jia Pu]

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>

[Jia Pu]

Created attachment 78548 [details]
Proposed patch (v1)

[WebKit Review Bot]

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

[Build Bot]

Attachment 78548 [details] did not build on win:
Build output: http://queues.webkit.org/results/7402131

[Jia Pu]

Created attachment 78554 [details]
Proposed patch (v2)

Fixed build failure on non-Mac platforms.

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”

[Early Warning System Bot]

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

[Jia Pu]

Created attachment 78592 [details]
Proposed patch (v3)

Fixed a typo in comment.

[Jia Pu]

(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.

[Jia Pu]

Created attachment 80814 [details]
patch preview.

[Jia Pu]

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.

[Jia Pu]

Created attachment 81893 [details]
Proposed patch (v4)

[WebKit Review Bot]

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

[Build Bot]

Attachment 81893 [details] did not build on win:
Build output: http://queues.webkit.org/results/7870070

[Early Warning System Bot]

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

[Collabora GTK+ EWS bot]

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

[Jia Pu]

Created attachment 81986 [details]
Proposed patch (v5)

Fixed compiler error on some platforms.

[Jia Pu]

Created attachment 81997 [details]
Proposed patch (v6)

Resolved a conflict with TOT.

[WebKit Review Bot]

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

[Collabora GTK+ EWS bot]

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

[Early Warning System Bot]

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

[Build Bot]

Attachment 81997 [details] did not build on win:
Build output: http://queues.webkit.org/results/7867392

[Jia Pu]

Created attachment 82006 [details]
Proposed patch (v7)

More compiler error fix.

[WebKit Review Bot]

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

[Collabora GTK+ EWS bot]

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

[Build Bot]

Attachment 82006 [details] did not build on win:
Build output: http://queues.webkit.org/results/7870407

[Early Warning System Bot]

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

[Jia Pu]

Created attachment 82079 [details]
Proposed patch (v8)

More compiler error fix.

[Darin Adler]

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.

[Jia Pu]

Created attachment 82419 [details]
Proposed patch (v9)

Updated patch according comment 28.

[WebKit Review Bot]

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

[Early Warning System Bot]

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

[Jia Pu]

Created attachment 82423 [details]
Proposed patch (v10)

[Build Bot]

Attachment 82419 [details] did not build on win:
Build output: http://queues.webkit.org/results/7909836

[Jia Pu]

(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.

[WebKit Commit Bot]

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

[Jia Pu]

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?

[Darin Adler]

(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.

[Jia Pu]

Created attachment 82507 [details]
Proposed patch (v11)

Same as previous patch. But it is generated against more recent TOT to resolve conflict.

[Jia Pu]

I noticed that patching often fails on efl. I hope that's a separate issue.

[WebKit Commit Bot]

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

[Darin Adler]

Comment on attachment 82507 [details]
Proposed patch (v11)

I tried to apply the patch and it failed on Android.mk.

[Darin Adler]

(In reply to comment #41)
> I tried to apply the patch and it failed on Android.mk.

LOL, wrong patch. Sorry!

[Darin Adler]

I’ll work on landing this.

[Douglas Davidson]

Thanks, Darin.

[Jia Pu]

(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.

[Darin Adler]

Committed r78632: <http://trac.webkit.org/changeset/78632>