Fix bug where misspelling underline is not removed properly

Fix bug where misspelling underline is not removed properly

I made a change in https://codereview.chromium.org/2755013004 that significantly
changed how DocumentMarkers are updated in response to text editing operations.
In particular, I made a change that means replacing exactly the range of text
marked by a DocumentMarker will preserve the marker (resizing it if necessary)
on the new piece of text (previously the marker was removed in this case). This
introduced a bug when replacing words using spellcheck suggestions (e.g. from
the right-click context menu) because the current logic just replaces the word
without explictly removing the marker. The fix is to insert an extra step to
explicitly remove the marker.

I have added a test case to hopefully ensure this bug does not reoccur.

Note: this bug does not occur when the Blink feature IdleTimeSpellChecking is
enabled. I believe this is because with the flag enabled, the spellchecker
removes the marker when it rechecks the word in the background.

BUG=722721

[rlanday, 2017-05-19 23:31:59]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[rlanday, 2017-05-19 23:32:15]

rlanday@chromium.org changed reviewers:
+ xiaochengh@chromium.org, yosin@chromium.org

[commit-bot: I haz the power, 2017-05-19 23:33:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 23:42:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 23:42:09]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Xiaocheng, 2017-05-20 00:01:55]

lgtm

Thanks for the quick fix!

[yosin_UTC9, 2017-05-22 05:14:10]

https://codereview.chromium.org/2891403004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right):

https://codereview.chromium.org/2891403004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:841:
GetFrame().GetDocument()->Markers().RemoveMarkersInRange(
If we decide to remove marker explicitly, I against,
It should be done just before, or after(?), 
| GetFrame().GetEditor().ReplaceSelectionWithText()| call
to respect "beforeinput" event, at L853.

https://codereview.chromium.org/2891403004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:842:
marker_range, DocumentMarker::MisspellingMarkers());
I'm not sure why do we need to remove marker explicitly?

We should remove other types of markers too, e.g. text match marker.

Editor::ReplaceSelectionWithText(), which is called at end of
|SpellChecker::ReplaceMisspelledRange()|, should remove a markers.

[rlanday, 2017-05-22 05:21:50]

On 2017/05/22 at 05:14:10, yosin wrote:
>
https://codereview.chromium.org/2891403004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:842:
marker_range, DocumentMarker::MisspellingMarkers());
> I'm not sure why do we need to remove marker explicitly?
> 
> We should remove other types of markers too, e.g. text match marker.

This change was made to support Android SuggestionSpans. When you apply a
suggestion, it's possible to undo the replacement or pick a different suggestion
by tapping on the text again after doing the replacement. So we need to preserve
the suggestion marker when doing a text replace. The way this is implemented,
ReplaceSelectionWithText() does an atomic replacement that preserves the
DocumentMarker. To properly support suggestion markers, we either need to keep
this behavior, or alternatively, have two different replace methods (which I
think would be confusing).

You think text match markers are also broken by this change?

[yosin_UTC9, 2017-05-22 06:07:25]

On 2017/05/22 at 05:21:50, rlanday wrote:
> On 2017/05/22 at 05:14:10, yosin wrote:
> >
https://codereview.chromium.org/2891403004/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:842:
marker_range, DocumentMarker::MisspellingMarkers());
> > I'm not sure why do we need to remove marker explicitly?
> > 
> > We should remove other types of markers too, e.g. text match marker.
> 
> This change was made to support Android SuggestionSpans. When you apply a
suggestion, it's possible to undo the replacement or pick a different suggestion
by tapping on the text again after doing the replacement. So we need to preserve
the suggestion marker when doing a text replace. The way this is implemented,
ReplaceSelectionWithText() does an atomic replacement that preserves the
DocumentMarker. To properly support suggestion markers, we either need to keep
this behavior, or alternatively, have two different replace methods (which I
think would be confusing).
> 
> You think text match markers are also broken by this change?

It is make sense to keep markers for IME and suggestions, since these markers
are for range instead of content of range.
However, markers for spelling, grammar, and text match depend on content of
range. These markers should be removed
when underling content is changed.

This should be done in DMC, or ListImpl, for response of content change, e.g.
replace, delete, insert commands.

Explicit removing markers for content change, in this patch
Editor::ReplaceSelectionWithText(), leaks marker's
nature outside DMC. It is not good for module relations.

[rlanday, 2017-05-22 06:15:26]

On 2017/05/22 at 06:07:25, yosin wrote:
> Explicit removing markers for content change, in this patch
Editor::ReplaceSelectionWithText(), leaks marker's
> nature outside DMC. It is not good for module relations.

You're basically suggesting that we have different marker update logic for
different MarkerTypes. We can do that, but the update logic is fairly complex,
and I don't think we should maintain multiple versions of it without a
compelling reason.

I don't think it's possible to avoid "leaking marker's nature"; we had one
behavior before (replacing a marked piece of text removes the DocumentMarker)
and now we have a different behavior (replacing a marked piece of text preserves
the marker). Either way, when we write code that uses DocumentMarkers, we have
to understand what the behavior will be and write code to work with that
behavior.


If you really want to have different behavior for different marker types, I
think one way to do this is to add a boolean method on DocumentMarkerList called
something like PreservesMarkerOnTextReplace() and pass that value into
DocumentMarker::ComputeOffsetsAfterShift().

[rlanday, 2017-05-22 06:16:56]

On 2017/05/22 at 06:15:26, rlanday wrote:
> If you really want to have different behavior for different marker types, I
think one way to do this is to add a boolean method on DocumentMarkerList called
something like PreservesMarkerOnTextReplace() and pass that value into
DocumentMarker::ComputeOffsetsAfterShift().

Or, we could have the flag be something like ClearMarkerOnTextChange(), and
clear the marker on all edits that change the text, not just a full replacement.

[yosin_UTC9, 2017-05-22 07:03:19]

On 2017/05/22 at 06:15:26, rlanday wrote:
> On 2017/05/22 at 06:07:25, yosin wrote:
> > Explicit removing markers for content change, in this patch
Editor::ReplaceSelectionWithText(), leaks marker's
> > nature outside DMC. It is not good for module relations.
> 
> You're basically suggesting that we have different marker update logic for
different MarkerTypes. We can do that, but the update logic is fairly complex,
and I don't think we should maintain multiple versions of it without a
compelling reason.
> 
> I don't think it's possible to avoid "leaking marker's nature"; we had one
behavior before (replacing a marked piece of text removes the DocumentMarker)
and now we have a different behavior (replacing a marked piece of text preserves
the marker). Either way, when we write code that uses DocumentMarkers, we have
to understand what the behavior will be and write code to work with that
behavior.

Please don't "leaking marker's nature". If we really need to do so, it is bad
design.
Spell checker only knows spelling/grammar, IME only knows Composition, and so
on.

> 
> If you really want to have different behavior for different marker types, I
think one way to do this is to add a boolean method on DocumentMarkerList called
something like PreservesMarkerOnTextReplace() and pass that value into
DocumentMarker::ComputeOffsetsAfterShift().

Please don't have boolean functions to change behavior based on "marker's
nature."
We have marker type specific list implementation, it should be done in each
DMListImpl.
To share code, we can have two functions, content dependent and content
independent, DMListEditor.

[rlanday, 2017-05-22 17:41:12]

On 2017/05/22 at 07:03:19, yosin wrote:
> Please don't have boolean functions to change behavior based on "marker's
nature."
> We have marker type specific list implementation, it should be done in each
DMListImpl.
> To share code, we can have two functions, content dependent and content
independent, DMListEditor.

Ok, I will refactor the marker-shifting code so we have two versions.

Note that we need to get a fix for this merged into the version 59 release
branch. Do you think it's safe to merge another substantial change to the
marker-shifting logic, or should we just go ahead and merge the one-line fix for
the release?

[rlanday, 2017-05-22 22:41:56]

On 2017/05/22 at 17:41:12, rlanday wrote:
> On 2017/05/22 at 07:03:19, yosin wrote:
> > Please don't have boolean functions to change behavior based on "marker's
nature."
> > We have marker type specific list implementation, it should be done in each
DMListImpl.
> > To share code, we can have two functions, content dependent and content
independent, DMListEditor.
> 
> Ok, I will refactor the marker-shifting code so we have two versions.
> 
> Note that we need to get a fix for this merged into the version 59 release
branch. Do you think it's safe to merge another substantial change to the
marker-shifting logic, or should we just go ahead and merge the one-line fix for
the release?

Here's the CL to change the marker-shifting logic:
https://codereview.chromium.org/2895353003