New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 727172 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug

Blocked on:
issue 715365



Sign in to add a comment

Regression: Spellcheck markers not shown

Project Member Reported by noel@chromium.org, May 29 2017

Issue description

Load http://csreis.github.io/tests/cross-site-iframe-simple.html in chrome stable 58, and enter "szad asd asd asd asd asd" in the "What is your name?" field.

Spell check markers (red underlines) are shown for each word of the text. 
 - Chrome 58 Stable.png attached

Load the site in Chrome Canary 61, spell check markers are not shown for any words.
 - Chrome 61.0.3114.0 Canary.png attached

Spellcheck markers should be shown for misspelt words: this bug as a regression.

 
Chrome 58 Stable.png
154 KB View Download
Chrome 61.0.3114.0 Canary.png
151 KB View Download

Comment 1 by noel@chromium.org, May 29 2017

Components: Blink>Editing>Spellcheck UI>Browser>Language>Spellcheck

Comment 2 by noel@chromium.org, May 29 2017

Description: Show this description

Comment 3 by yosin@chromium.org, May 29 2017

Components: -UI>Browser>Language>Spellcheck
Labels: OS-Mac OS-Windows
Owner: xiaoche...@chromium.org
Status: Assigned (was: Untriaged)
I could not re-produce on
Wi10 61.0.3114.0 (Official Build) canary (64-bit) (cohort: 64-Bit)
for both chrome://flags/#enable-idle-time-spell-checking Enabled/Disabled

Screenshot in #c1 seems to be MacOS.

Comment 4 by noel@chromium.org, May 29 2017

Labels: -OS-Windows
Yes, confirm not seeing on Windows canary.  MacOS only for now.

Comment 5 by noel@chromium.org, May 29 2017

Blockedon: 715365
Bisecting on MacOSX, suggests patch https://codereview.chromium.org/2848943002 on  bug 715365  caused this issue.

Comment 6 by noel@chromium.org, May 29 2017

I reverted that change locally on MacOSX form my ToT Chromium build, and the spellcheck markers appear per n normal.

Comment 7 by yosin@chromium.org, May 29 2017

Owner: rlanday@chromium.org
Per #c5, rlanday@, could you take look?

Comment 8 by yosin@chromium.org, May 29 2017

FYI: "Ask Google for suggestions", AGfS works as expected on Windows.
See reproeduce steps in the  issue 633641 .

AGfS also returns the result async as MacOS spell checker.
Project Member

Comment 9 by bugdroid1@chromium.org, May 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02

commit e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02
Author: noel <noel@chromium.org>
Date: Mon May 29 05:54:15 2017

Revert of Allow storing multiple replacements on SpellCheckResult (patchset #11 id:200001 of https://codereview.chromium.org/2848943002/ )

Reason for revert:
Caused spell markers to no longer appear on OSX, per  crbug.com/727172 

Original issue's description:
> Allow storing multiple replacements on SpellCheckResult
>
> I'm working on adding support for the Android spellcheck suggestion list that
> appears in a native text box when you tap on a misspelled word. It appears that
> I need to modify this piece of infrastructure to support passing multiple
> suggestions through to the Spelling marker that gets added.
>
> For reference, we construct the SpellCheckResult objects from the Android
> spellchecker results in SpellCheckerSessionBridge::ProcessSpellCheckResults().
>
> Alternatively, ContextMenuClientImpl has logic for calling the spellchecker
> again to get the list of suggestions that we might be able to use:
> https://chromium.googlesource.com/chromium/src/+/adc8910776c64a876f0422454da618a37d01167b/third_party/WebKit/Source/web/ContextMenuClientImpl.cpp#346
>
> However, I talked to aelias and he said it would be better to store the
> suggestions the first time we call the spellchecker, at the time we add the
> marker, to avoid going back and forth unnecessarily.
>
> BUG= 715365 
>
> Review-Url: https://codereview.chromium.org/2848943002
> Cr-Commit-Position: refs/heads/master@{#475058}
> Committed: https://chromium.googlesource.com/chromium/src/+/c45f50742410a644a054325478d885e61d286713

TBR=jochen@chromium.org,aelias@chromium.org,groby@chromium.org,mkwst@chromium.org,xiaochengh@chromium.org,rlanday@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 715365 ,  727172 

Review-Url: https://codereview.chromium.org/2906243002
Cr-Commit-Position: refs/heads/master@{#475293}

[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/chrome/browser/renderer_context_menu/spelling_menu_observer.cc
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/chrome/browser/spellchecker/spellcheck_message_filter_platform_mac_unittest.cc
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/chrome/browser/spellchecker/spellcheck_message_filter_unittest.cc
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/chrome/browser/spellchecker/spelling_service_client_unittest.cc
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/components/spellcheck/common/BUILD.gn
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/components/spellcheck/common/spellcheck_messages.h
[delete] https://crrev.com/64b85971b8b58c052b7e8b53754d5558b8e1dc53/components/spellcheck/common/spellcheck_result.cc
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/components/spellcheck/common/spellcheck_result.h
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/components/spellcheck/renderer/spellcheck.cc
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/components/spellcheck/renderer/spellcheck_provider_test.cc
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/components/spellcheck/renderer/spellcheck_provider_unittest.cc
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/components/spellcheck/renderer/spellcheck_unittest.cc
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/content/shell/test_runner/mock_spell_check.cc
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/content/shell/test_runner/spell_check_client.cc
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/third_party/WebKit/Source/core/editing/spellcheck/SpellCheckRequester.h
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/third_party/WebKit/Source/core/editing/spellcheck/SpellCheckerTest.cpp
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/third_party/WebKit/Source/platform/text/TextChecking.h
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/third_party/WebKit/Source/web/WebTextCheckingResult.cpp
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/e6a7d7e3cd5665b6bc9b0e8cac4c813c52f90b02/third_party/WebKit/public/web/WebTextCheckingResult.h

Comment 10 by noel@chromium.org, May 29 2017

After #9, built ToT Chromium Mac OSX and the bug is gone (I see spell check markers again in web forms, web contentEditable, etc).

Follow-up bug to add a browser test for macOS spellcheck:
crbug.com/728659
Project Member

Comment 12 by bugdroid1@chromium.org, Jun 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/455b156778ed1f2df63f8a14dff99399c88e475c

commit 455b156778ed1f2df63f8a14dff99399c88e475c
Author: rlanday <rlanday@chromium.org>
Date: Fri Jun 02 14:48:11 2017

[Reland] Allow storing multiple replacements on SpellCheckResult

This CL was reverted because it did not properly handle the case where a
misspelling result is created with no suggested replacements. We want to
preserve misspelling results that don't include any suggested replacements, but
remove those that have some suggested replacements but they're all just changing
between different types of apostrophes.

Original CL:
https://codereview.chromium.org/2848943002

Revert:
https://codereview.chromium.org/2906243002

Original description:

Allow storing multiple replacements on SpellCheckResult

I'm working on adding support for the Android spellcheck suggestion list that
appears in a native text box when you tap on a misspelled word. It appears that
I need to modify this piece of infrastructure to support passing multiple
suggestions through to the Spelling marker that gets added.

For reference, we construct the SpellCheckResult objects from the Android
spellchecker results in SpellCheckerSessionBridge::ProcessSpellCheckResults().

Alternatively, ContextMenuClientImpl has logic for calling the spellchecker
again to get the list of suggestions that we might be able to use:
https://chromium.googlesource.com/chromium/src/+/adc8910776c64a876f0422454da618a37d01167b/third_party/WebKit/Source/web/ContextMenuClientImpl.cpp#346

However, I talked to aelias and he said it would be better to store the
suggestions the first time we call the spellchecker, at the time we add the
marker, to avoid going back and forth unnecessarily.

BUG= 715365 , 727172 

Review-Url: https://codereview.chromium.org/2911253003
Cr-Commit-Position: refs/heads/master@{#476644}

[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/chrome/browser/renderer_context_menu/spelling_menu_observer.cc
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/chrome/browser/spellchecker/spell_check_host_impl_unittest.cc
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/chrome/browser/spellchecker/spellcheck_message_filter_platform_mac_unittest.cc
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/chrome/browser/spellchecker/spelling_service_client_unittest.cc
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/components/spellcheck/common/BUILD.gn
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/components/spellcheck/common/spellcheck_messages.h
[add] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/components/spellcheck/common/spellcheck_result.cc
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/components/spellcheck/common/spellcheck_result.h
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/components/spellcheck/renderer/spellcheck.cc
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/components/spellcheck/renderer/spellcheck_provider_test.cc
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/components/spellcheck/renderer/spellcheck_provider_unittest.cc
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/components/spellcheck/renderer/spellcheck_unittest.cc
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/content/shell/test_runner/mock_spell_check.cc
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/content/shell/test_runner/spell_check_client.cc
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/third_party/WebKit/Source/core/editing/spellcheck/SpellCheckRequester.h
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/third_party/WebKit/Source/core/editing/spellcheck/SpellCheckerTest.cpp
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/third_party/WebKit/Source/core/exported/WebTextCheckingResult.cpp
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/third_party/WebKit/Source/platform/text/TextChecking.h
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/455b156778ed1f2df63f8a14dff99399c88e475c/third_party/WebKit/public/web/WebTextCheckingResult.h

Status: Fixed (was: Assigned)

Comment 14 by noel@chromium.org, Jun 6 2017

Status: Verified (was: Fixed)
Verified fixed, Mac looks good to me.

Sign in to add a comment