Bug 164857 - WKBundleNodeHandleSetHTMLInputElementSpellcheckEnabled should keep text replacement enabled
Summary: WKBundleNodeHandleSetHTMLInputElementSpellcheckEnabled should keep text repla...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-11-16 23:02 PST by Ryosuke Niwa
Modified: 2016-11-17 20:57 PST (History)
6 users (show)

See Also:


Attachments
Fixes the bug (11.13 KB, patch)
2016-11-16 23:16 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Leave smart quotes/dashes disabled (12.68 KB, patch)
2016-11-16 23:34 PST, Ryosuke Niwa
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (965.40 KB, application/zip)
2016-11-17 00:46 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (1.63 MB, application/zip)
2016-11-17 00:54 PST, Build Bot
no flags Details
Fixed the tests (12.30 KB, patch)
2016-11-17 16:40 PST, Ryosuke Niwa
wenson_hsieh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-11-16 23:02:24 PST
Change the SPI added in https://trac.webkit.org/changeset/201942 so that text replacement would be enabled.
Comment 1 Ryosuke Niwa 2016-11-16 23:03:32 PST
<rdar://problem/27721742>
Comment 2 Ryosuke Niwa 2016-11-16 23:16:48 PST
Created attachment 295032 [details]
Fixes the bug
Comment 3 Ryosuke Niwa 2016-11-16 23:34:08 PST
Created attachment 295034 [details]
Leave smart quotes/dashes disabled
Comment 4 Build Bot 2016-11-17 00:46:35 PST
Comment on attachment 295034 [details]
Leave smart quotes/dashes disabled

Attachment 295034 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2530138

New failing tests:
editing/spelling/spellcheck-attribute.html
editing/spelling/spellcheck-paste.html
editing/spelling/spelling-attribute-change.html
editing/spelling/spelling-hasspellingmarker.html
editing/spelling/spellcheck-queue.html
Comment 5 Build Bot 2016-11-17 00:46:38 PST
Created attachment 295040 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 6 Build Bot 2016-11-17 00:54:21 PST
Comment on attachment 295034 [details]
Leave smart quotes/dashes disabled

Attachment 295034 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2530143

New failing tests:
editing/spelling/spelling-attribute-change.html
editing/spelling/spellcheck-queue.html
editing/spelling/spellcheck-attribute.html
editing/spelling/spelling-hasspellingmarker.html
editing/spelling/spellcheck-paste.html
Comment 7 Build Bot 2016-11-17 00:54:24 PST
Created attachment 295041 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 8 Wenson Hsieh 2016-11-17 08:31:24 PST
Comment on attachment 295034 [details]
Leave smart quotes/dashes disabled

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

> Source/WebCore/ChangeLog:15
> +        No new tests since we don't have a good facility to test text replacement.

By text replacement, do you mean selecting a candidate in the popover that appears over a word to replace it? I believe internals.handleAcceptedCandidate exercises this codepath.
Comment 9 Darin Adler 2016-11-17 09:00:01 PST
Comment on attachment 295034 [details]
Leave smart quotes/dashes disabled

Waiting to review until the patch passes tests on EWS and until we verify that internals.handleAcceptedCandidate can’t be used to add another test.
Comment 10 Ryosuke Niwa 2016-11-17 14:55:10 PST
(In reply to comment #8)
> Comment on attachment 295034 [details]
> Leave smart quotes/dashes disabled
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295034&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        No new tests since we don't have a good facility to test text replacement.
> 
> By text replacement, do you mean selecting a candidate in the popover that
> appears over a word to replace it? I believe
> internals.handleAcceptedCandidate exercises this codepath.

No, this is a feature enabled by System Preference > Keyboard > Text.

What this patch does is surpassing spellchecking requests but allowing text replacement feature so I don’t think we can use that internals method.

We likely need some sort of a new WebKitTestRunner feature to do this if anything.
Comment 11 Ryosuke Niwa 2016-11-17 16:39:40 PST
Comment on attachment 295034 [details]
Leave smart quotes/dashes disabled

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

> Source/WebCore/html/HTMLInputElement.cpp:119
> -    , m_isSpellCheckingEnabled(true)
> +    , m_isSpellcheckDisabledExceptTextReplacement(true)

The tests are failing because this needs to be false.
Comment 12 Ryosuke Niwa 2016-11-17 16:40:37 PST
Created attachment 295109 [details]
Fixed the tests
Comment 13 Wenson Hsieh 2016-11-17 19:48:05 PST
Comment on attachment 295109 [details]
Fixed the tests

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

> Source/WebCore/editing/Editor.cpp:3487
> +TextCheckingTypeMask Editor::resolveTextCheckingTypeMask(Node& rootEditableElement, TextCheckingTypeMask textCheckingOptions)

Can this be a const Node& rootEditableElement?
Comment 14 Ryosuke Niwa 2016-11-17 20:57:30 PST
Thanks for the review!

(In reply to comment #13)
> Comment on attachment 295109 [details]
> Fixed the tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295109&action=review
> 
> > Source/WebCore/editing/Editor.cpp:3487
> > +TextCheckingTypeMask Editor::resolveTextCheckingTypeMask(Node& rootEditableElement, TextCheckingTypeMask textCheckingOptions)
> 
> Can this be a const Node& rootEditableElement?

Yup, fixed.
Comment 15 Ryosuke Niwa 2016-11-17 20:57:56 PST
Committed r208875: <http://trac.webkit.org/changeset/208875>