Bug 81323

Summary: [Mac][Chromium] Should not spellcheck text pasted to an element having spellcheck disabled
Product: WebKit Reporter: Hironori Bono <hbono>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: morrita, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: Unspecified   
Attachments:
Description Flags
A layout test
none
A quick fix v1
none
A quick fix v2 (applied comments, used the Internals interface, and skipped a new test)
morrita: review+, morrita: commit-queue-
A quick fix v3 (Removed window.layoutTestController) none

Description Hironori Bono 2012-03-16 02:39:04 PDT
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
Comment 1 Hironori Bono 2012-03-16 02:51:52 PDT
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
Comment 2 Ryosuke Niwa 2012-03-16 09:30:58 PDT
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.
Comment 3 Hajime Morrita 2012-03-18 20:49:29 PDT
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().
Comment 4 Hironori Bono 2012-03-21 04:07:25 PDT
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
Comment 5 Hajime Morrita 2012-03-21 23:12:02 PDT
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.
Comment 6 Hironori Bono 2012-04-02 14:41:00 PDT
Created attachment 135193 [details]
A quick fix v3 (Removed window.layoutTestController)
Comment 7 WebKit Review Bot 2012-04-02 14:42:00 PDT
Comment on attachment 135193 [details]
A quick fix v3 (Removed window.layoutTestController)

Rejecting attachment 135193 [details] from commit-queue.

hbono@chromium.org 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.
Comment 8 WebKit Review Bot 2012-04-02 15:32:05 PDT
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
Comment 9 WebKit Review Bot 2012-04-03 19:19:07 PDT
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>
Comment 10 WebKit Review Bot 2012-04-03 19:19:12 PDT
All reviewed patches have been landed.  Closing bug.