Bug 81323 – [Mac][Chromium] Should not spellcheck text pasted to an element having spellcheck disabled

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Hironori Bono]

Created attachment 132235 [details]
A layout test

Greetings,

I notice WebKit spellchecks text pasted to an element whose spellcheck attribute is false. In fact, the attached layout test fails on Mac. It would be great to prevent spellchecking text pasted to such an element.

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 132237 [details]
A quick fix v1

Greetings,

I have written a quick fix that fixes this issue. Would it be possible to review it?

Regards,

Hironori Bono

[Ryosuke Niwa]

Comment on attachment 132237 [details]
A quick fix v1

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

> Source/WebCore/editing/Editor.cpp:425
>      m_spellChecker->requestCheckingFor(SpellCheckRequest::create(resolveTextCheckingTypeMask(TextCheckingTypeSpelling | TextCheckingTypeGrammar), TextCheckingProcessBatch, rangeToCheck, rangeToCheck));

It's probably better to check the above conditions inside requestCheckingFor. Or maybe add a variant like requestCheckingForIfEnabled.

[Hajime Morrita]

Comment on attachment 132237 [details]
A quick fix v1

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

>> Source/WebCore/editing/Editor.cpp:425
>>      m_spellChecker->requestCheckingFor(SpellCheckRequest::create(resolveTextCheckingTypeMask(TextCheckingTypeSpelling | TextCheckingTypeGrammar), TextCheckingProcessBatch, rangeToCheck, rangeToCheck));
> 
> It's probably better to check the above conditions inside requestCheckingFor. Or maybe add a variant like requestCheckingForIfEnabled.

IMHO, such kind of defensiveness isn't good idea. 
My suggestion is to 
- add SpellChecker::isCheckable(Range*) and check it before invoking requestCheckingFor()
- assert it inside requestCheckingFor().

[Hironori Bono]

Created attachment 133006 [details]
A quick fix v2 (applied comments, used the Internals interface, and skipped a new test)

Greetings Morita-san and Niwa-san,

Thanks for your review and comments.
I have moved a isSpellCheckingEnabled() check to SpellChecker::isCheckable(). (On the other hand, a isInPasswordField() check is in Editor.cpp because this check is not so trivial.) Also have I updated this change to use the Internals interface and added the new test to Skipped lists. Would it be possible to take another look?

Regards,

Hironori Bono

[Hajime Morrita]

Comment on attachment 133006 [details]
A quick fix v2 (applied comments, used the Internals interface, and skipped a new test)

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

Looks good. Thanks for the fix!

> LayoutTests/editing/spelling/spellcheck-paste-disabled.html:34
> +if (window.layoutTestController) {

You don't need this change since you the preparation phase already uses window.internals anyway, which is only available in DRT.

[Hironori Bono]

Created attachment 135193 [details]
A quick fix v3 (Removed window.layoutTestController)

[WebKit Review Bot]

Comment on attachment 135193 [details]
A quick fix v3 (Removed window.layoutTestController)

Rejecting attachment 135193 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 135193 [details]
A quick fix v3 (Removed window.layoutTestController)

Rejecting attachment 135193 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12313726

[WebKit Review Bot]

Comment on attachment 135193 [details]
A quick fix v3 (Removed window.layoutTestController)

Clearing flags on attachment: 135193

Committed r113127: <http://trac.webkit.org/changeset/113127>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.