Summary: | [Chromium] Replace correct misspelled range | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rouslan Solomakhin <rouslan+webkit> | ||||||||||||||
Component: | HTML Editing | Assignee: | 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
Rouslan Solomakhin
2013-01-31 12:29:39 PST
Created attachment 186074 [details]
Patch
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? (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. (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/ Does smart replace have more useful functionality that I did not notice? Comment on attachment 186074 [details]
Patch
Which test does this fix?
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. (In reply to comment #6) > (From update of attachment 186074 [details]) > Which test does this fix? I am not sure how to test this... Created attachment 186083 [details]
Patch
Comment on attachment 186083 [details]
Patch
Explained the reason for disabling smart replace in ChangeLog.
(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 on attachment 186083 [details]
Patch
Going to write the test.
(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 ? (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. (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? (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. (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 :-) Created attachment 186692 [details]
Patch
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 Created attachment 187378 [details]
Patch
Comment on attachment 187378 [details]
Patch
Fixed the test.
Created attachment 187380 [details]
Patch
Comment on attachment 187380 [details]
Patch
This patch avoids a leak in WebFrameTest by keeping the spellcheck client on stack.
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? (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 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 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? (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 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 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; Created attachment 187633 [details]
Patch for landing
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 on attachment 187633 [details] Patch for landing Clearing flags on attachment: 187633 Committed r142494: <http://trac.webkit.org/changeset/142494> All reviewed patches have been landed. Closing bug. |