Bug 47629 – Moving cursor back to autocorrected word in the same line shouldn't remove the underline

• Component: New Bugs
• Status: RESOLVED FIXED

[Jia Pu]

<rdar://problem/8546758>

[Jia Pu]

Created attachment 71088 [details]
Proposed patch (v1)

[Adele Peterson]

Comment on attachment 71088 [details]
Proposed patch (v1)

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

> WebCore/editing/Editor.cpp:-3616
> -            m_frame->document()->markers()->removeMarkers(wordRange.get(), DocumentMarker::CorrectionIndicator);

Why remove this line?

[Jia Pu]

(In reply to comment #2)
> (From update of attachment 71088 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71088&action=review
> 
> > WebCore/editing/Editor.cpp:-3616
> > -            m_frame->document()->markers()->removeMarkers(wordRange.get(), DocumentMarker::CorrectionIndicator);
> 
> Why remove this line?

This marker value doesn't get added on SnowLeopard or earlier. So, since now this couple lines of code is only for SL or earlier OS, we don't need to remove this marker here anymore.

[Adele Peterson]

Comment on attachment 71088 [details]
Proposed patch (v1)

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

>>> WebCore/editing/Editor.cpp:-3616
>>> -            m_frame->document()->markers()->removeMarkers(wordRange.get(), DocumentMarker::CorrectionIndicator);
>> 
>> Why remove this line?
> 
> This marker value doesn't get added on SnowLeopard or earlier. So, since now this couple lines of code is only for SL or earlier OS, we don't need to remove this marker here anymore.

Ah, ok.  It looks like before recent changes, there was a call to removeMarkers for DocumentMarker::Replacement.  Does that need to be re-added?

[Jia Pu]

(In reply to comment #4)
> (From update of attachment 71088 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71088&action=review
> 
> >>> WebCore/editing/Editor.cpp:-3616
> >>> -            m_frame->document()->markers()->removeMarkers(wordRange.get(), DocumentMarker::CorrectionIndicator);
> >> 
> >> Why remove this line?
> > 
> > This marker value doesn't get added on SnowLeopard or earlier. So, since now this couple lines of code is only for SL or earlier OS, we don't need to remove this marker here anymore.
> 
> Ah, ok.  It looks like before recent changes, there was a call to removeMarkers for DocumentMarker::Replacement.  Does that need to be re-added?

There's no need to add that. The statement for removing Replacement is also part of the patch for adding autocorrection support. But later on, I replaced "Replacement" with "CorrectionIndicator".

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 71088 [details]:



Please file bugs against the tests.  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 71088 [details]
Proposed patch (v1)

Clearing flags on attachment: 71088

Committed r70071: <http://trac.webkit.org/changeset/70071>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This changed the rendering for a number of editing tests in chromium mac (on 10.5, not on 10.7 obviously):


editing/deleting/delete-3800834-fix.html
editing/deleting/merge-unrendered-space.html
editing/deleting/pruning-after-merge-2.html
editing/deleting/smart-delete-002.html
editing/execCommand/4924441.html
editing/execCommand/5080333-1.html
editing/execCommand/5080333-2.html
editing/execCommand/insert-list-and-stitch.html
editing/execCommand/remove-list-item-1.html
editing/inserting/insert-div-026.html
editing/inserting/insert-paragraph-04.html
editing/inserting/insert-text-with-newlines.html
editing/inserting/return-key-with-selection-001.html
editing/inserting/return-key-with-selection-002.html
editing/pasteboard/5483567.html
editing/pasteboard/paste-text-019.html
editing/pasteboard/smart-paste-008.html
editing/pasteboard/subframe-dragndrop-1.html
editing/selection/4983858.html
editing/selection/5057506-2.html
editing/selection/move-by-character-001.html
editing/style/5046875-1.html
editing/style/create-block-for-style-001.html
editing/style/create-block-for-style-003.html
editing/style/create-block-for-style-004.html
editing/style/create-block-for-style-007.html
editing/style/create-block-for-style-009.html
editing/style/create-block-for-style-010.html
editing/style/create-block-for-style-013.html


I haven't confirmed if the rendering results are different yet in safari but will check that next.  The new rendering has red squigglies under many words that previously did not.

[James Robinson]

It appears this only affects chromium mac, not Safari or chromium on other platforms.

[Jia Pu]

(In reply to comment #10)
> It appears this only affects chromium mac, not Safari or chromium on other platforms.
I think, instead of:

#if PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD))

I should add:

#if !PLATFORM(MAC) || (PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD)))

This way, it only changes the behavior in Safari on 10.7. Shall I create a separate bug for this, or adding another path in this bug?

James, what do I need to run tests for chromium mac?

[James Robinson]

I believe that would work.  I'll try confirming locally.

[Jia Pu]

(In reply to comment #12)
> I believe that would work.  I'll try confirming locally.

Also, can we find out when were these tests created? <http://trac.webkit.org/changeset/66643> introduced a regression in the underline behavior, which has been corrected by this patch.

So, if these tests were generated after <http://trac.webkit.org/changeset/66643>, we need to regenerate the screen shots after the fix.

[Ryosuke Niwa]

(In reply to comment #11)
> (In reply to comment #10)
> > It appears this only affects chromium mac, not Safari or chromium on other platforms.
> I think, instead of:
> 
> #if PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD))
> 
> I should add:
> 
> #if !PLATFORM(MAC) || (PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD)))
> 
> This way, it only changes the behavior in Safari on 10.7. Shall I create a separate bug for this, or adding another path in this bug?

SGTM.

But did you get comment about there's a code that adds correction indicator that's outside of ifdef's?
http://trac.webkit.org/browser/trunk/WebCore/editing/Editor.cpp?rev=70071#L2788

Could you verify that this is intended?

[Ryosuke Niwa]

Also, I have to say that the new behavior seems better than the old behavior.  In http://trac.webkit.org/browser/trunk/LayoutTests/editing/deleting/delete-3800834-fix.html for example, we have:

<span id="test">Foo<BR><BLOCKQUOTE>Bar</BLOCKQUOTE></span>

And we delete "\nBar" backwards.  We have correction indicator (or whatever red dotted line is called) until we delete "Bar" but the indicator disappears when we delete "\n" between "Foo" and "Bar" (deleted by that moment).  Is this intended?

[Jia Pu]

(In reply to comment #14)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > It appears this only affects chromium mac, not Safari or chromium on other platforms.
> > I think, instead of:
> > 
> > #if PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD))
> > 
> > I should add:
> > 
> > #if !PLATFORM(MAC) || (PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD)))
> > 
> > This way, it only changes the behavior in Safari on 10.7. Shall I create a separate bug for this, or adding another path in this bug?
> 
> SGTM.
> 
> But did you get comment about there's a code that adds correction indicator that's outside of ifdef's?
> http://trac.webkit.org/browser/trunk/WebCore/editing/Editor.cpp?rev=70071#L2788
> 
> Could you verify that this is intended?

The CorrectionIndicator marker isn't used except in Safari on 10.7. But, for the sake of clarity, I suppose I should put that statement inside "#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)"

[James Robinson]

(In reply to comment #11)
> (In reply to comment #10)
> > It appears this only affects chromium mac, not Safari or chromium on other platforms.
> I think, instead of:
> 
> #if PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD))
> 
> I should add:
> 
> #if !PLATFORM(MAC) || (PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD)))
> 
> This way, it only changes the behavior in Safari on 10.7. Shall I create a separate bug for this, or adding another path in this bug?
> 
> James, what do I need to run tests for chromium mac?

Confirmed that this change restores the previous Chromium Mac behavior (which matches the current Safari Mac behavior).

[Ryosuke Niwa]

(In reply to comment #17)
> Confirmed that this change restores the previous Chromium Mac behavior (which matches the current Safari Mac behavior).

Great!  Jia, I think you can commit this change as build fix since it broke chromium test bots.

[Jia Pu]

(In reply to comment #15)
> Also, I have to say that the new behavior seems better than the old behavior.  In http://trac.webkit.org/browser/trunk/LayoutTests/editing/deleting/delete-3800834-fix.html for example, we have:
> 
> <span id="test">Foo<BR><BLOCKQUOTE>Bar</BLOCKQUOTE></span>
> 
> And we delete "\nBar" backwards.  We have correction indicator (or whatever red dotted line is called) until we delete "Bar" but the indicator disappears when we delete "\n" between "Foo" and "Bar" (deleted by that moment).  Is this intended?

I'm not quite sure about the rationale behind the old behavior. So for this patch, my intention is to only change introduce the new behavior into Safari on 10.7. 

If the new behavior is desirable across the board, I could certainly make this change to all platforms.

[Jia Pu]

Reopen the bug. The first patch broke some Chromium Mac behavior.

[Jia Pu]

Created attachment 71225 [details]
Additional Patch (v1)

New patch to make sure there's no change of behavior on non-Mac platforms.

[James Robinson]

Comment on attachment 71225 [details]
Additional Patch (v1)

R=me

Thanks for following up on this!

[WebKit Commit Bot]

Comment on attachment 71225 [details]
Additional Patch (v1)

Clearing flags on attachment: 71225

Committed r70113: <http://trac.webkit.org/changeset/70113>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.