Bug 45709 – Need to remove autocorrection underlines in current line when newline is entered.

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

[Jia Pu]

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.

[Jia Pu]

<rdar://problem/8335576>

[Jia Pu]

Created attachment 67475 [details]
Proposed patch (v1)

[Darin Adler]

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?

[Enrica Casucci]

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.

[Jia Pu]

Created attachment 67822 [details]
Proposed patch (v2)

Revised patch based on feedback in comment #4.

[Ryosuke Niwa]

There is no way to test this?

[Ryosuke Niwa]

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

[Darin Adler]

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.

[Jia Pu]

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

[Ryosuke Niwa]

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

[Jia Pu]

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

[Darin Adler]

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

[Jia Pu]

Created attachment 68618 [details]
Proposed patch (v3)

Much simplified patch. 

This change affects only Mac OSX.

[WebKit Review Bot]

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.

[Jia Pu]

Created attachment 68621 [details]
Proposed patch (v4)

Removed tabs from previous patch.

[Enrica Casucci]

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.

[Jia Pu]

Created attachment 68629 [details]
Proposed patch (v5)

Revised per comment #16.

[Enrica Casucci]

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
> [phone]  Jia Pu  <[e-mail]>

You used a different email address in earlier patches. Are you sure you want to switch?

[Jia Pu]

Created attachment 68641 [details]
proposed patch (v6)

Using correct patch author email address.

[Shinichiro Hamaji]

Comment on attachment 68641 [details]
proposed patch (v6)

r+ based on mitz review. As Jia mentioned, the last patch only changes email address.

[WebKit Commit Bot]

Comment on attachment 68641 [details]
proposed patch (v6)

Clearing flags on attachment: 68641

Committed r68243: <http://trac.webkit.org/changeset/68243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.