Bug 108513

Summary: [Chromium] Replace correct misspelled range
Product: WebKit Reporter: Rouslan Solomakhin <rouslan+webkit>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: groby, rniwa, rouslan+webkit, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 106815    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Rouslan Solomakhin 2013-01-31 12:29:39 PST
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.)
Comment 1 Rouslan Solomakhin 2013-02-01 10:23:03 PST
Created attachment 186074 [details]
Patch
Comment 2 Ryosuke Niwa 2013-02-01 10:48:18 PST
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?
Comment 3 Rouslan Solomakhin 2013-02-01 10:56:57 PST
(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.
Comment 4 Rouslan Solomakhin 2013-02-01 11:03:02 PST
(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/
Comment 5 Rouslan Solomakhin 2013-02-01 11:16:54 PST
Does smart replace have more useful functionality that I did not notice?
Comment 6 Tony Chang 2013-02-01 11:19:33 PST
Comment on attachment 186074 [details]
Patch

Which test does this fix?
Comment 7 Tony Chang 2013-02-01 11:20:08 PST
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.
Comment 8 Rouslan Solomakhin 2013-02-01 11:21:11 PST
(In reply to comment #6)
> (From update of attachment 186074 [details])
> Which test does this fix?

I am not sure how to test this...
Comment 9 Rouslan Solomakhin 2013-02-01 11:24:27 PST
Created attachment 186083 [details]
Patch
Comment 10 Rouslan Solomakhin 2013-02-01 11:25:06 PST
Comment on attachment 186083 [details]
Patch

Explained the reason for disabling smart replace in ChangeLog.
Comment 11 Tony Chang 2013-02-01 11:27:37 PST
(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 _.
Comment 12 Rouslan Solomakhin 2013-02-01 11:32:26 PST
Comment on attachment 186083 [details]
Patch

Going to write the test.
Comment 13 Rouslan Solomakhin 2013-02-04 10:13:57 PST
(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 ?
Comment 14 Ryosuke Niwa 2013-02-04 10:26:14 PST
(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.
Comment 15 Rouslan Solomakhin 2013-02-04 10:43:02 PST
(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?
Comment 16 Ryosuke Niwa 2013-02-04 10:50:19 PST
(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.
Comment 17 Rouslan Solomakhin 2013-02-04 10:52:12 PST
(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 :-)
Comment 18 Rouslan Solomakhin 2013-02-05 13:24:36 PST
Created attachment 186692 [details]
Patch
Comment 19 Rouslan Solomakhin 2013-02-05 13:27:09 PST
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
Comment 20 Rouslan Solomakhin 2013-02-08 15:54:47 PST
Created attachment 187378 [details]
Patch
Comment 21 Rouslan Solomakhin 2013-02-08 15:55:34 PST
Comment on attachment 187378 [details]
Patch

Fixed the test.
Comment 22 Rouslan Solomakhin 2013-02-08 15:59:43 PST
Created attachment 187380 [details]
Patch
Comment 23 Rouslan Solomakhin 2013-02-08 16:00:46 PST
Comment on attachment 187380 [details]
Patch

This patch avoids a leak in WebFrameTest by keeping the spellcheck client on stack.
Comment 24 Ryosuke Niwa 2013-02-08 22:39:22 PST
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?
Comment 25 Rouslan Solomakhin 2013-02-11 09:48:40 PST
(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.
Comment 26 Tony Chang 2013-02-11 09:58:53 PST
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())"?
Comment 27 Ryosuke Niwa 2013-02-11 10:27:30 PST
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?
Comment 28 Rouslan Solomakhin 2013-02-11 11:07:13 PST
(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.
Comment 29 James Robinson 2013-02-11 11:31:21 PST
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)
Comment 30 James Robinson 2013-02-11 11:33:23 PST
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;
Comment 31 Rouslan Solomakhin 2013-02-11 12:03:48 PST
Created attachment 187633 [details]
Patch for landing
Comment 32 Rouslan Solomakhin 2013-02-11 12:06:17 PST
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.
Comment 33 WebKit Review Bot 2013-02-11 12:25:26 PST
Comment on attachment 187633 [details]
Patch for landing

Clearing flags on attachment: 187633

Committed r142494: <http://trac.webkit.org/changeset/142494>
Comment 34 WebKit Review Bot 2013-02-11 12:25:31 PST
All reviewed patches have been landed.  Closing bug.