Bug 164857 – WKBundleNodeHandleSetHTMLInputElementSpellcheckEnabled should keep text replacement enabled

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Change the SPI added in https://trac.webkit.org/changeset/201942 so that text replacement would be enabled.

[Ryosuke Niwa]

<rdar://problem/27721742>

[Ryosuke Niwa]

Created attachment 295032 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 295034 [details]
Leave smart quotes/dashes disabled

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Wenson Hsieh]

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.

[Darin Adler]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 295109 [details]
Fixed the tests

[Wenson Hsieh]

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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r208875: <http://trac.webkit.org/changeset/208875>