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
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 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 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().
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 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.
Created attachment 135193 [details] A quick fix v3 (Removed window.layoutTestController)
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 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 on attachment 135193 [details] A quick fix v3 (Removed window.layoutTestController) Clearing flags on attachment: 135193 Committed r113127: <http://trac.webkit.org/changeset/113127>
All reviewed patches have been landed. Closing bug.