In Bug 44958, we added blue underline on autocorrected words. To be consistent with the behavior in NSTextView, we need to remove autocorrection underline in current line when a newline character is inserted.
<rdar://problem/8335576>
Created attachment 67475 [details] Proposed patch (v1)
Comment on attachment 67475 [details] Proposed patch (v1) This is the wrong level for this code. I am almost certain we don't want to removal of markers tied specifically to the code path that handles hitting the newline key. Enrica, maybe you can help Jia find the right place for this?
I think the right place to do this is TypingCommand. Look at the doApply method, that is where we handle special characters such as delete, enter, backspace, etc.
Created attachment 67822 [details] Proposed patch (v2) Revised patch based on feedback in comment #4.
There is no way to test this?
(In reply to comment #6) > There is no way to test this? I think you can test this given the tests in: http://trac.webkit.org/browser/trunk/LayoutTests/editing/spelling Take a look at http://trac.webkit.org/browser/trunk/LayoutTests/editing/editing.js for how to insert a line break from the script.
Comment on attachment 67822 [details] Proposed patch (v2) > + * editing/TypingCommand.cpp: > + (WebCore::TypingCommand::insertLineBreak): Remove "CorrectionUIIndication" markers when a line > + break is inserted. > + (WebCore::TypingCommand::insertParagraphSeparatorInQuotedContent): Remove "CorrectionUIIndication" > + markers when a paragraph separator is inserted. > + (WebCore::TypingCommand::insertParagraphSeparator): Ditto. Won’t the call in respondToChangedSelection already take care of this? It doesn’t make sense to me that these calls need to explicitly be done by the typing command machinery.
(In reply to comment #8) > (From update of attachment 67822 [details]) > > + * editing/TypingCommand.cpp: > > + (WebCore::TypingCommand::insertLineBreak): Remove "CorrectionUIIndication" markers when a line > > + break is inserted. > > + (WebCore::TypingCommand::insertParagraphSeparatorInQuotedContent): Remove "CorrectionUIIndication" > > + markers when a paragraph separator is inserted. > > + (WebCore::TypingCommand::insertParagraphSeparator): Ditto. > > Won’t the call in respondToChangedSelection already take care of this? It doesn’t make sense to me that these calls need to explicitly be done by the typing command machinery. If I understand the code correctly, respondToChangedSelection() is called on every text insertion. However the behavior we want (which is consistent with AppKit) is clearing the markers only when a line break is inserted. That said, please let me know if this can be done in respondToChangedSelection().
(In reply to comment #9) > (In reply to comment #8) > > Won’t the call in respondToChangedSelection already take care of this? It doesn’t make sense to me that these calls need to explicitly be done by the typing command machinery. > > If I understand the code correctly, respondToChangedSelection() is called on every text insertion. However the behavior we want (which is consistent with AppKit) is clearing the markers only when a line break is inserted. Why do we want this behavior on all platforms? If you're trying to match WebKit's behavior with AppKit, shouldn't we just doing that for Mac platforms? Also, I'm not even sure clearing markers only when a line break is inserted is an obvious improvement on Mac.
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > Won’t the call in respondToChangedSelection already take care of this? It doesn’t make sense to me that these calls need to explicitly be done by the typing command machinery. > > > > If I understand the code correctly, respondToChangedSelection() is called on every text insertion. However the behavior we want (which is consistent with AppKit) is clearing the markers only when a line break is inserted. > > Why do we want this behavior on all platforms? If you're trying to match WebKit's behavior with AppKit, shouldn't we just doing that for Mac platforms? Also, I'm not even sure clearing markers only when a line break is inserted is an obvious improvement on Mac. This is in effect Mac only, since on non-Mac platform, the said marker doesn't get added in the first place. However, I can certainly bracket the removal code with ifdef's if it's desirable. The goal of this patch is to bring WebKit into parity with other UI on Mac. To further improve this design is probably outside of the scope of this patch.
(In reply to comment #9) > If I understand the code correctly, respondToChangedSelection() is called on every text insertion. I was referring to the code you added to respondToChangedSelection. But it seems that code removes the marker from the first word in the selection and is not related to the removal when inserting a line break. I still don’t understand why the code is tied to editing operations. Typically hitting return and pasting in a return character would do the same thing. Is it correct that pasting in a return character would not remove the marker even though hitting the return key would?
Created attachment 68618 [details] Proposed patch (v3) Much simplified patch. This change affects only Mac OSX.
Attachment 68618 [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] WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Total errors found: 3 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 68621 [details] Proposed patch (v4) Removed tabs from previous patch.
Comment on attachment 68621 [details] Proposed patch (v4) View in context: https://bugs.webkit.org/attachment.cgi?id=68621&action=review The rest looks good. > WebCore/editing/Editor.cpp:3131 > + VisiblePosition newSelectionEnd(newSelection.end(), newSelection.affinity()); There is no need to use oldSelection, oldSelectionStart and newSelectionEnd. You can simply do: if (!inSameParagraph(m_frame->selection()->selection().visibleStart(), newSelection.visibleEnd())) and remove the three lines above.
Created attachment 68629 [details] Proposed patch (v5) Revised per comment #16.
Comment on attachment 68629 [details] Proposed patch (v5) Looks good to me.
Comment on attachment 68629 [details] Proposed patch (v5) View in context: https://bugs.webkit.org/attachment.cgi?id=68629&action=review r=me. Set the cq flag to ? if you want this committed without changing the email address. > WebCore/ChangeLog:1 > +2010-09-23 Jia Pu <jiapu.mail@gmail.com> You used a different email address in earlier patches. Are you sure you want to switch?
Created attachment 68641 [details] proposed patch (v6) Using correct patch author email address.
Comment on attachment 68641 [details] proposed patch (v6) r+ based on mitz review. As Jia mentioned, the last patch only changes email address.
Comment on attachment 68641 [details] proposed patch (v6) Clearing flags on attachment: 68641 Committed r68243: <http://trac.webkit.org/changeset/68243>
All reviewed patches have been landed. Closing bug.