Bug 83214 – [Chromium] moving a cursor on a misspelled word should not remove a misspelled underline

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hironori Bono]

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

[Hironori Bono]

Created attachment 135691 [details]
A patch v0

[Ryosuke Niwa]

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?

[Hironori Bono]

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.

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

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.

[WebKit Review Bot]

Comment on attachment 135691 [details]
A patch v0

Clearing flags on attachment: 135691

Committed r113405: <http://trac.webkit.org/changeset/113405>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hironori Bono]

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.

[Ryosuke Niwa]

(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!