Bug 108513 – [Chromium] Replace correct misspelled range

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

[Rouslan Solomakhin]

WebFrameImpl::replaceMisspelledRange() in Source/WebKit/chromium/src/WebFrameImpl.cpp sometimes replaces the wrong range. (Chromium will use this method instead of WebFrameImpl::replaceRange() when correcting misspellings.)

[Rouslan Solomakhin]

Created attachment 186074 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 186074 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1319
> -    frame()->editor()->replaceSelectionWithText(text, false, true);
> +    frame()->editor()->replaceSelectionWithText(text, false, false);

Why are you disabling smart replace?

[Rouslan Solomakhin]

(In reply to comment #2)
> (From update of attachment 186074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186074&action=review
> 
> > Source/WebKit/chromium/src/WebFrameImpl.cpp:1319
> > -    frame()->editor()->replaceSelectionWithText(text, false, true);
> > +    frame()->editor()->replaceSelectionWithText(text, false, false);
> 
> Why are you disabling smart replace?

If the user selects "wellcome_" together with underline, context clicks on the selection, and chooses the "welcome" suggestion, then smart replace will result in "welcome _". I think that might be OK behavior for replaceRange(), but not for replaceMisspelledRange(). This method should only replace the misspelling, not introduce extra spaces.

[Rouslan Solomakhin]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 186074 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=186074&action=review
> > 
> > > Source/WebKit/chromium/src/WebFrameImpl.cpp:1319
> > > -    frame()->editor()->replaceSelectionWithText(text, false, true);
> > > +    frame()->editor()->replaceSelectionWithText(text, false, false);
> > 
> > Why are you disabling smart replace?
> 
> If the user selects "wellcome_" together with underline, context clicks on the selection, and chooses the "welcome" suggestion, then smart replace will result in "welcome _". I think that might be OK behavior for replaceRange(), but not for replaceMisspelledRange(). This method should only replace the misspelling, not introduce extra spaces.

s/underline/underscore/

[Rouslan Solomakhin]

Does smart replace have more useful functionality that I did not notice?

[Tony Chang]

Comment on attachment 186074 [details]
Patch

Which test does this fix?

[Tony Chang]

Comment on attachment 186074 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:9
> +        (WebKit::WebFrameImpl::replaceMisspelledRange): Replace correct misspelled range.

You could explain why you disable smart replace here.

[Rouslan Solomakhin]

(In reply to comment #6)
> (From update of attachment 186074 [details])
> Which test does this fix?

I am not sure how to test this...

[Rouslan Solomakhin]

Created attachment 186083 [details]
Patch

[Rouslan Solomakhin]

Comment on attachment 186083 [details]
Patch

Explained the reason for disabling smart replace in ChangeLog.

[Tony Chang]

(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 186074 [details] [details])
> > Which test does this fix?
> 
> I am not sure how to test this...

I see.  Hmm, since this is all code in Source/WebKit/chromium, you might be able to write a webkit_unit_test for this.  See Source/WebKit/chromium/tests for other C++ unit tests.  What you'd want to do is set up a web frame with wellcome selected and call WebFrame::replaceMisspelledRange directly.  You could then verify that the content doesn't have the extra space between welcome and _.

[Rouslan Solomakhin]

Comment on attachment 186083 [details]
Patch

Going to write the test.

[Rouslan Solomakhin]

(In reply to comment #12)
> (From update of attachment 186083 [details])
> Going to write the test.

Would it be OK to add window.internals.replaceMisspellingRange() method for testing WebKit::WebFrameImpl::replaceMisspelledRange() from Source/WebKit/chromium/src/WebFrameImpl.h ?

[Ryosuke Niwa]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 186083 [details] [details])
> > Going to write the test.
> 
> Would it be OK to add window.internals.replaceMisspellingRange() method for testing WebKit::WebFrameImpl::replaceMisspelledRange() from Source/WebKit/chromium/src/WebFrameImpl.h ?

It's a WebKit API so it needs to be testRunner but I don't think we should be adding port specific testRunner method like that.

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 186074 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=186074&action=review
> > 
> > > Source/WebKit/chromium/src/WebFrameImpl.cpp:1319
> > > -    frame()->editor()->replaceSelectionWithText(text, false, true);
> > > +    frame()->editor()->replaceSelectionWithText(text, false, false);
> > 
> > Why are you disabling smart replace?
> 
> If the user selects "wellcome_" together with underline, context clicks on the selection, and chooses the "welcome" suggestion, then smart replace will result in "welcome _". I think that might be OK behavior for replaceRange(), but not for replaceMisspelledRange(). This method should only replace the misspelling, not introduce extra spaces.

You should be able to create a selection like that manually using getSelection(). There ought to be a way to trigger correction once the text is selected.

[Rouslan Solomakhin]

(In reply to comment #14)
> You should be able to create a selection like that manually using getSelection(). There ought to be a way to trigger correction once the text is selected.

Ryosuke: Tony suggests that I make WebFrameImpl::replaceMisspelledRange() call directly into WebCore. Then we can add JS function window.internals.replaceMisspelledRange() for layout tests. Do you agree with this plan?

[Ryosuke Niwa]

(In reply to comment #15)
> (In reply to comment #14)
> > You should be able to create a selection like that manually using getSelection(). There ought to be a way to trigger correction once the text is selected.
> 
> Ryosuke: Tony suggests that I make WebFrameImpl::replaceMisspelledRange() call directly into WebCore. Then we can add JS function window.internals.replaceMisspelledRange() for layout tests. Do you agree with this plan?

No. This function is specific to the Chromium port. Please don't add a new method to internals. If you're interested in unit testing this, add a Chromium API test instead.

[Rouslan Solomakhin]

(In reply to comment #16)
> > Ryosuke: Tony suggests that I make WebFrameImpl::replaceMisspelledRange() call directly into WebCore. Then we can add JS function window.internals.replaceMisspelledRange() for layout tests. Do you agree with this plan?
> 
> No. This function is specific to the Chromium port. Please don't add a new method to internals. If you're interested in unit testing this, add a Chromium API test instead.

Understood. I am working on it :-)

[Rouslan Solomakhin]

Created attachment 186692 [details]
Patch

[Rouslan Solomakhin]

I am not able to add misspelling markers from with the unit test. Am I missing something obvious or doing something completely wrong?

Here is the test output:

$ build-webkit --chromium && ./out/Release/webkit_unit_tests  --gtest_filter='WebFrameTest.*spell*'
ninja -C out/Release all 
ninja: Entering directory `out/Release'
[3/3] STAMP obj/Source/WebKit/chromium/all_webkit.actions_depends.stamp

====================================================================
 WebKit is now built (00m:06s). 
====================================================================
Note: Google Test filter = WebFrameTest.*spell*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from WebFrameTest
[ RUN      ] WebFrameTest.ReplaceMisspelledRange
../../Source/WebKit/chromium/tests/WebFrameTest.cpp:2149: Failure
Value of: document->markers()->markersInRange(range.get(), WebCore::DocumentMarker::Spelling).size()
  Actual: 0
Expected: 1U
Which is: 1
../../Source/WebKit/chromium/tests/WebFrameTest.cpp:2158: Failure
Value of: std::string(frame->contentAsText(std::numeric_limits<size_t>::max()).utf8().data())
  Actual: "_wellcome_."
Expected: "_welcome_."
[  FAILED  ] WebFrameTest.ReplaceMisspelledRange (33 ms)
[----------] 1 test from WebFrameTest (33 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (34 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] WebFrameTest.ReplaceMisspelledRange

 1 FAILED TEST
  YOU HAVE 9 DISABLED TESTS

[Rouslan Solomakhin]

Created attachment 187378 [details]
Patch

[Rouslan Solomakhin]

Comment on attachment 187378 [details]
Patch

Fixed the test.

[Rouslan Solomakhin]

Created attachment 187380 [details]
Patch

[Rouslan Solomakhin]

Comment on attachment 187380 [details]
Patch

This patch avoids a leak in WebFrameTest by keeping the spellcheck client on stack.

[Ryosuke Niwa]

Comment on attachment 187380 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1316
> +    RefPtr<Range> markerRange = Range::create(caretRange->ownerDocument(), caretRange->startContainer(), markers[0]->startOffset(), caretRange->endContainer(), markers[0]->endOffset());
> +    if (!markerRange)
> +        return;
> +    if (!frame()->selection()->shouldChangeSelection(markerRange.get()))

Doesn't this break spellchecking inside a contenteditable element?

[Rouslan Solomakhin]

(In reply to comment #24)
> (From update of attachment 187380 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187380&action=review
> 
> > Source/WebKit/chromium/src/WebFrameImpl.cpp:1316
> > +    RefPtr<Range> markerRange = Range::create(caretRange->ownerDocument(), caretRange->startContainer(), markers[0]->startOffset(), caretRange->endContainer(), markers[0]->endOffset());
> > +    if (!markerRange)
> > +        return;
> > +    if (!frame()->selection()->shouldChangeSelection(markerRange.get()))
> 
> Doesn't this break spellchecking inside a contenteditable element?

This patch adds a test that uses <div contentEditable></div> for spellcheck. I don't think it breaks anything.

[Tony Chang]

Comment on attachment 187380 [details]
Patch

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

> Source/WebKit/chromium/tests/WebFrameTest.cpp:2204
> +    virtual void requestCheckingOfText(const WebKit::WebString&, WebKit::WebTextCheckingCompletion* completion)

OVERRIDE

> Source/WebKit/chromium/tests/WebFrameTest.cpp:2206
> +        m_numberOfTimesChecked++;

Nit: ++m_numberOfTimesChecked

> Source/WebKit/chromium/tests/WebFrameTest.cpp:2208
> +        results.append(WebTextCheckingResult(WebTextCheckingTypeSpelling, 1, 8, WebString()));

Nit: Can we make local variables for 1 and 8 to make it more clear what the parameters are?

> Source/WebKit/chromium/tests/WebFrameTest.cpp:2226
> +    // Setup environment.

I would remove all of these comments.  They aren't "why?" comments.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:2236
> +    // Select the text "_wellcome_."
> +    frame->selectRange(WebRange::fromDocumentRange(frame, 0, 11));

For example, if you made local variables like:
  int wellcomeStartOffset = 0;
  int wellcomeEndOffset = 11;
I don't think you need the comment.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:2241
> +    // Wait for spellcheck.
> +    for (int i = 0; i < 10 && document->markers()->markersInRange(selectionRange.get(), DocumentMarker::Spelling).isEmpty(); ++i)
> +        webkit_support::RunAllPendingMessages();

Why 10 times?  Would it be sufficient to use "while (document->markers()->markersInRange(selectionRange.get(), DocumentMarker::Spelling).isEmpty())"?

[Ryosuke Niwa]

Comment on attachment 187380 [details]
Patch

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

>>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1316
>>> +    if (!frame()->selection()->shouldChangeSelection(markerRange.get()))
>> 
>> Doesn't this break spellchecking inside a contenteditable element?
> 
> This patch adds a test that uses <div contentEditable></div> for spellcheck. I don't think it breaks anything.

Does it work even if you had content (e.g. some text) before div?

[Rouslan Solomakhin]

(In reply to comment #27)
> (From update of attachment 187380 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187380&action=review
> 
> >>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1316
> >>> +    if (!frame()->selection()->shouldChangeSelection(markerRange.get()))
> >> 
> >> Doesn't this break spellchecking inside a contenteditable element?
> > 
> > This patch adds a test that uses <div contentEditable></div> for spellcheck. I don't think it breaks anything.
> 
> Does it work even if you had content (e.g. some text) before div?

Spellcheck works with that use-case both pre-patch and post-patch: spellcheck suggests a correctly-spelled word and lets you replace the word within the element. The replacement behavior changes because the patch turns off smart-replace, but I expect that.

I've tested pre-patch and post-patch spellcheck for the following URL:

data:text/html,<span contentEditable>well</span><span contentEditable>come</span> home

Most behavior does not change:

1) The text fragments "well" and "come" are treated as one misspelled word "wellcome".

2) Context-click on "well" selects only "well" and suggests "welcome" as correct spelling.

3) Context-click on "come" selects only "come" and suggest "welcome" as correct spelling.

The behavior that changes is due to turning off smart-replace in the patch:

BEFORE the patch: If you select "welcome" suggestion from context menu, you would get either "welcome come" or "well welcome", depending on whether you context clicked on "well" or "come" part of the word.

AFTER the patch: If you select "welcome" suggestion from context menu, you would get either "welcomecome" or "wellwelcome", depending on whether you context clicked on "well" or "come" part of the word.

[James Robinson]

Comment on attachment 187380 [details]
Patch

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

> Source/WebKit/chromium/tests/WebFrameTest.cpp:2218
> +    WebView* webView = FrameTestHelpers::createWebViewAndLoad("data:text/html,<div id=\"data\" contentEditable></div>");

Replace this with:

m_webView = FrameTestHelpers:....

so the test harness properly cleans this view up at the end of the test

> Source/WebKit/chromium/tests/WebFrameTest.cpp:2247
> +    webkit_support::RunAllPendingMessages();

call runPendingTasks() here (it's in FrameTestHelpers and pulled into the namespace of this file by a using statement)

[James Robinson]

Comment on attachment 187380 [details]
Patch

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

> Source/WebKit/chromium/tests/WebFrameTest.cpp:2220
> +    SpellCheckClient spellcheck;
> +    webView->setSpellCheckClient(&spellcheck);

actually, since you have this client on the stack you need to close (aka destroy) your WebView before this class falls out of scope or you may get mysterious crashes.  Add these two lines to the bottom of this function:

m_webView->close();
m_webView = 0;

[Rouslan Solomakhin]

Created attachment 187633 [details]
Patch for landing

[Rouslan Solomakhin]

Comment on attachment 187633 [details]
Patch for landing

I removed RunAllPendingMessages() because it turned out to not be necessary with the stub implementation of SpellCheckClient. It marks the misspelling immediately.

[WebKit Review Bot]

Comment on attachment 187633 [details]
Patch for landing

Clearing flags on attachment: 187633

Committed r142494: <http://trac.webkit.org/changeset/142494>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.