Bug 47629 - Moving cursor back to autocorrected word in the same line shouldn't remove the underline
Summary: Moving cursor back to autocorrected word in the same line shouldn't remove th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 47630
  Show dependency treegraph
 
Reported: 2010-10-13 15:45 PDT by Jia Pu
Modified: 2010-10-19 19:47 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch (v1) (309.17 KB, patch)
2010-10-18 15:03 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Additional Patch (v1) (1.61 KB, patch)
2010-10-19 16:47 PDT, Jia Pu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jia Pu 2010-10-13 15:45:44 PDT
<rdar://problem/8546758>
Comment 1 Jia Pu 2010-10-18 15:03:23 PDT
Created attachment 71088 [details]
Proposed patch (v1)
Comment 2 Adele Peterson 2010-10-18 15:22:58 PDT
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?
Comment 3 Jia Pu 2010-10-18 15:35:08 PDT
(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.
Comment 4 Adele Peterson 2010-10-18 15:47:17 PDT
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?
Comment 5 Jia Pu 2010-10-18 17:37:54 PDT
(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".
Comment 6 WebKit Commit Bot 2010-10-19 11:28:56 PDT
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.
Comment 7 WebKit Commit Bot 2010-10-19 11:30:13 PDT
Comment on attachment 71088 [details]
Proposed patch (v1)

Clearing flags on attachment: 71088

Committed r70071: <http://trac.webkit.org/changeset/70071>
Comment 8 WebKit Commit Bot 2010-10-19 11:30:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 James Robinson 2010-10-19 14:25:32 PDT
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.
Comment 10 James Robinson 2010-10-19 14:52:38 PDT
It appears this only affects chromium mac, not Safari or chromium on other platforms.
Comment 11 Jia Pu 2010-10-19 14:56:57 PDT
(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?
Comment 12 James Robinson 2010-10-19 15:01:32 PDT
I believe that would work.  I'll try confirming locally.
Comment 13 Jia Pu 2010-10-19 15:02:27 PDT
(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.
Comment 14 Ryosuke Niwa 2010-10-19 15:03:56 PDT
(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?
Comment 15 Ryosuke Niwa 2010-10-19 15:07:49 PDT
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?
Comment 16 Jia Pu 2010-10-19 15:19:35 PDT
(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)"
Comment 17 James Robinson 2010-10-19 15:24:01 PDT
(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).
Comment 18 Ryosuke Niwa 2010-10-19 15:25:59 PDT
(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.
Comment 19 Jia Pu 2010-10-19 15:26:42 PDT
(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.
Comment 20 Jia Pu 2010-10-19 15:37:17 PDT
Reopen the bug. The first patch broke some Chromium Mac behavior.
Comment 21 Jia Pu 2010-10-19 16:47:51 PDT
Created attachment 71225 [details]
Additional Patch (v1)

New patch to make sure there's no change of behavior on non-Mac platforms.
Comment 22 James Robinson 2010-10-19 16:52:06 PDT
Comment on attachment 71225 [details]
Additional Patch (v1)

R=me

Thanks for following up on this!
Comment 23 WebKit Commit Bot 2010-10-19 17:20:45 PDT
Comment on attachment 71225 [details]
Additional Patch (v1)

Clearing flags on attachment: 71225

Committed r70113: <http://trac.webkit.org/changeset/70113>
Comment 24 WebKit Commit Bot 2010-10-19 17:20:52 PDT
All reviewed patches have been landed.  Closing bug.