Bug 83214 - [Chromium] moving a cursor on a misspelled word should not remove a misspelled underline
Summary: [Chromium] moving a cursor on a misspelled word should not remove a misspelle...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-04 14:19 PDT by Hironori Bono
Modified: 2012-05-16 09:42 PDT (History)
8 users (show)

See Also:


Attachments
A patch v0 (9.29 KB, patch)
2012-04-04 15:03 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hironori Bono 2012-04-04 14:19:46 PDT
Greetings,

When Chrome uses asynchronous spellchecking, it attaches Spelling markers to misspelled words in the background and attaches their suggestions to their description members. Unfortunately, WebKit removes these markers when we move a cursor onto the misspelled words on all platforms except Lion. (WebKit change r70071 <http://trac.webkit.org/changeset/70071> prevents removing Spelling markers on Lion.) It would be great not to remove Spelling markers also on Chromium. (Leaving Spelling markers also prevents spellchecking words every time when we right-click a word.)

Regards,

Hironori Bono
Comment 1 Hironori Bono 2012-04-04 15:03:29 PDT
Created attachment 135691 [details]
A patch v0
Comment 2 Ryosuke Niwa 2012-04-05 12:37:28 PDT
Comment on attachment 135691 [details]
A patch v0

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

> Source/WebCore/editing/Editor.cpp:2931
> +#if PLATFORM(CHROMIUM)
> +        if (!m_frame->settings() || !m_frame->settings()->asynchronousSpellCheckingEnabled()) {
> +            if (RefPtr<Range> wordRange = newAdjacentWords.toNormalizedRange())
> +                m_frame->document()->markers()->removeMarkers(wordRange.get(), DocumentMarker::Spelling);
> +        }
> +#else

Does this need to be guarded inside Chromium at all? Doesn't other port that implements async spell checking want this fix as well?
Comment 3 Hironori Bono 2012-04-05 13:34:51 PDT
Comment on attachment 135691 [details]
A patch v0

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

>> Source/WebCore/editing/Editor.cpp:2931
>> +#else
> 
> Does this need to be guarded inside Chromium at all? Doesn't other port that implements async spell checking want this fix as well?

Unfortunately, it loses compatibility with AppKit on Snow Leopard. Even though AppKit of Snow Leopard can spellcheck text in the background when we paste text, it deletes misspelling markers when we move a cursor.
Comment 4 Ryosuke Niwa 2012-04-05 13:46:51 PDT
Comment on attachment 135691 [details]
A patch v0

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

>>> Source/WebCore/editing/Editor.cpp:2931
>>> +#else
>> 
>> Does this need to be guarded inside Chromium at all? Doesn't other port that implements async spell checking want this fix as well?
> 
> Unfortunately, it loses compatibility with AppKit on Snow Leopard. Even though AppKit of Snow Leopard can spellcheck text in the background when we paste text, it deletes misspelling markers when we move a cursor.

Interesting. It seems acceptable to land this patch as is but we should figure out a better way of abstracting these port-specific behaviors.
Maybe we want some client callback here?
Comment 5 Ryosuke Niwa 2012-04-05 13:47:54 PDT
Jpu & morrita: Can you think of a good way to push these port-specific code to WebKit layer?

It's really annoying to have all these if-defs in WebCore code.
Comment 6 WebKit Review Bot 2012-04-05 19:48:56 PDT
Comment on attachment 135691 [details]
A patch v0

Clearing flags on attachment: 135691

Committed r113405: <http://trac.webkit.org/changeset/113405>
Comment 7 WebKit Review Bot 2012-04-05 19:49:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Hironori Bono 2012-05-16 01:06:08 PDT
Greetings Niwa-san,

Sorry for my slow response. I have filed Bug 86591 <http://webkit.org/b/86591> to refactor it.

Regards,

Hironori Bono

(In reply to comment #5)
> Jpu & morrita: Can you think of a good way to push these port-specific code to WebKit layer?
> 
> It's really annoying to have all these if-defs in WebCore code.
Comment 9 Ryosuke Niwa 2012-05-16 09:42:22 PDT
(In reply to comment #8)
> Sorry for my slow response. I have filed Bug 86591 <http://webkit.org/b/86591> to refactor it.

r=me on that patch. Thanks for the follow up!