Bug 81323 - [Mac][Chromium] Should not spellcheck text pasted to an element having spellcheck disabled
Summary: [Mac][Chromium] Should not spellcheck text pasted to an element having spellc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-16 02:39 PDT by Hironori Bono
Modified: 2012-04-03 19:19 PDT (History)
3 users (show)

See Also:


Attachments
A layout test (2.23 KB, text/html)
2012-03-16 02:39 PDT, Hironori Bono
no flags Details
A quick fix v1 (6.15 KB, patch)
2012-03-16 02:51 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff
A quick fix v2 (applied comments, used the Internals interface, and skipped a new test) (12.04 KB, patch)
2012-03-21 04:07 PDT, Hironori Bono
morrita: review+
morrita: commit-queue-
Details | Formatted Diff | Diff
A quick fix v3 (Removed window.layoutTestController) (11.95 KB, patch)
2012-04-02 14:41 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-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.