Summary: | Autocorrection should respect undo. | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jia Pu <jiapu.mail> | ||||||||||||||||||||||||||
Component: | HTML Editing | Assignee: | Darin Adler <darin> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, darin, ddavidso, dglazkov, gustavo.noronha, gustavo, mitz, webkit-ews, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||||||
OS: | OS X 10.6 | ||||||||||||||||||||||||||||
Bug Depends on: | 53255 | ||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||
Attachments: |
|
Description
Jia Pu
2011-01-11 09:34:05 PST
Created attachment 78548 [details]
Proposed patch (v1)
Attachment 78548 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7353164 Attachment 78548 [details] did not build on win: Build output: http://queues.webkit.org/results/7402131 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” Attachment 78548 [details] did not build on qt: Build output: http://queues.webkit.org/results/7404147 Created attachment 78592 [details]
Proposed patch (v3)
Fixed a typo in comment.
(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. Created attachment 80814 [details]
patch preview.
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. Created attachment 81893 [details]
Proposed patch (v4)
Attachment 81893 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7875081 Attachment 81893 [details] did not build on win: Build output: http://queues.webkit.org/results/7870070 Attachment 81893 [details] did not build on qt: Build output: http://queues.webkit.org/results/7884070 Attachment 81893 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7870130 Created attachment 81986 [details]
Proposed patch (v5)
Fixed compiler error on some platforms.
Created attachment 81997 [details]
Proposed patch (v6)
Resolved a conflict with TOT.
Attachment 81997 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7867388 Attachment 81997 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7870379 Attachment 81997 [details] did not build on qt: Build output: http://queues.webkit.org/results/7869393 Attachment 81997 [details] did not build on win: Build output: http://queues.webkit.org/results/7867392 Created attachment 82006 [details]
Proposed patch (v7)
More compiler error fix.
Attachment 82006 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7869410 Attachment 82006 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7874397 Attachment 82006 [details] did not build on win: Build output: http://queues.webkit.org/results/7870407 Attachment 82006 [details] did not build on qt: Build output: http://queues.webkit.org/results/7868465 Created attachment 82079 [details]
Proposed patch (v8)
More compiler error fix.
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. Created attachment 82419 [details] Proposed patch (v9) Updated patch according comment 28. Attachment 82419 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7908821 Attachment 82419 [details] did not build on qt: Build output: http://queues.webkit.org/results/7912739 Created attachment 82423 [details]
Proposed patch (v10)
Attachment 82419 [details] did not build on win: Build output: http://queues.webkit.org/results/7909836 (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 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 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? (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. Created attachment 82507 [details]
Proposed patch (v11)
Same as previous patch. But it is generated against more recent TOT to resolve conflict.
I noticed that patching often fails on efl. I hope that's a separate issue. 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 on attachment 82507 [details]
Proposed patch (v11)
I tried to apply the patch and it failed on Android.mk.
(In reply to comment #41) > I tried to apply the patch and it failed on Android.mk. LOL, wrong patch. Sorry! I’ll work on landing this. Thanks, Darin. (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. Committed r78632: <http://trac.webkit.org/changeset/78632> |