Bug 188800 – [iOS][WK2] Misspelled words are not underlined

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

We should render misspelled words with the spelling correction dots in iOS WebKit (i.e. when using WKWebView). Currently we only support this in iOS WebKit Legacy (i.e. when using UIWebView).

The lack of spelling correction dots when using WKWebView was noticed by a person and mentioned in <https://stackoverflow.com/questions/46347640/how-to-make-the-spellcheck-work-on-uiwebview>.

[Daniel Bates]

<rdar://problem/34811332>

[Daniel Bates]

Created attachment 347665 [details]
Patch

[Daniel Bates]

Created attachment 347667 [details]
Patch

[Daniel Bates]

Created attachment 347668 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 347665 [details]
Patch

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

> Source/WebKit/UIProcess/ios/TextCheckerIOS.mm:56
> +    return mutableState();

Weird that it returns a const TextCheckerState&, and the function is non-const, but it uses mutableState(). So confused right now.

> Source/WebKit/UIProcess/ios/TextCheckerIOS.mm:153
> +static HashMap<int64_t, RetainPtr<UITextChecker>>& spellDocumentTagMap()

Please typedef the int64_t.

> Source/WebKit/UIProcess/ios/TextCheckerIOS.mm:208
> +    auto length = [stringToCheck length];

Doesn't seem necessary to pull this out into its own variable.

[EWS Watchlist]

Comment on attachment 347668 [details]
Patch

Attachment 347668 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8933586

New failing tests:
css3/filters/backdrop/add-remove-add-backdrop-filter.html

[EWS Watchlist]

Created attachment 347676 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Daniel Bates]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 347665 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347665&action=review
> 
> > Source/WebKit/UIProcess/ios/TextCheckerIOS.mm:56
> > +    return mutableState();
> 
> Weird that it returns a const TextCheckerState&, and the function is
> non-const, but it uses mutableState(). So confused right now.

I am open to suggestions.

Notice that TextChecker exposes a public function, static, member function state() that returns a const TextCheckerState&. 

> 
> > Source/WebKit/UIProcess/ios/TextCheckerIOS.mm:153
> > +static HashMap<int64_t, RetainPtr<UITextChecker>>& spellDocumentTagMap()
> 
> Please typedef the int64_t.

As I mentioned in-person, I will do this in a follow up to keep this patch focused on the iOS-specific changes.

> 
> > Source/WebKit/UIProcess/ios/TextCheckerIOS.mm:208
> > +    auto length = [stringToCheck length];
> 
> Doesn't seem necessary to pull this out into its own variable.

Will change.

[Daniel Bates]

(In reply to Build Bot from comment #6)
> Comment on attachment 347668 [details]
> Patch
> 
> Attachment 347668 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: https://webkit-queues.webkit.org/results/8933586
> 
> New failing tests:
> css3/filters/backdrop/add-remove-add-backdrop-filter.html

This failure is unrelated. Weird that the Mac WK2 EWS posted this comment even though it acknowledges that the tree is red.

[Daniel Bates]

Created attachment 347687 [details]
Patch

[Daniel Bates]

Created attachment 347697 [details]
Patch

[Wenson Hsieh]

Comment on attachment 347697 [details]
Patch

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

> Source/WebKit/UIProcess/ios/TextCheckerIOS.mm:64
>      notImplemented();

Can we remove this notImplemented() here?

> Source/WebKit/UIProcess/ios/TextCheckerIOS.mm:164
> +    static int64_t nextSpellDocumentTag;

Not about this patch, but we should consider something like:

using SpellDocumentTag = int64_t;

...at some point, instead of using int64_t directly to represent a document tag.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #12)
> Comment on attachment 347697 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347697&action=review
> 
> > Source/WebKit/UIProcess/ios/TextCheckerIOS.mm:64
> >      notImplemented();
> 
> Can we remove this notImplemented() here?
> 
> > Source/WebKit/UIProcess/ios/TextCheckerIOS.mm:164
> > +    static int64_t nextSpellDocumentTag;
> 
> Not about this patch, but we should consider something like:
> 
> using SpellDocumentTag = int64_t;
> 
> ...at some point, instead of using int64_t directly to represent a document
> tag.

Never mind, I see you filed <https://bugs.webkit.org/show_bug.cgi?id=188817>!

[Daniel Bates]

(In reply to Wenson Hsieh from comment #12)
> Comment on attachment 347697 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347697&action=review
> 
> > Source/WebKit/UIProcess/ios/TextCheckerIOS.mm:64
> >      notImplemented();
> 
> Can we remove this notImplemented() here?
> 

Will remove before landing.

[Daniel Bates]

Created attachment 347721 [details]
For landing

[Daniel Bates]

Comment on attachment 347721 [details]
For landing

Clearing flags on attachment: 347721

Committed r235149: <https://trac.webkit.org/changeset/235149>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Dawei Fenton (:realdawei)]

(In reply to Daniel Bates from comment #16)
> Comment on attachment 347721 [details]
> For landing
> 
> Clearing flags on attachment: 347721
> 
> Committed r235149: <https://trac.webkit.org/changeset/235149>

test editing/undo/replace-text-in-node-preserving-markers-crash.html is crashing consistently after this revision:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=editing%2Fundo%2Freplace-text-in-node-preserving-markers-crash.html

[Dawei Fenton (:realdawei)]

(In reply to David Fenton (:realdawei) from comment #18)
> (In reply to Daniel Bates from comment #16)
> > Comment on attachment 347721 [details]
> > For landing
> > 
> > Clearing flags on attachment: 347721
> > 
> > Committed r235149: <https://trac.webkit.org/changeset/235149>
> 
> test editing/undo/replace-text-in-node-preserving-markers-crash.html is
> crashing consistently after this revision:
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=editing%2Fundo%2Freplace-text-in-node-preserving-
> markers-crash.html

sample crash log:
https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r235225%20(6059)/editing/undo/replace-text-in-node-preserving-markers-crash-crash-log.txt

stderr:
1   0x1171509e9 WTFCrash
2   0x11b41f7f7 WebCore::DocumentMarker::DictationAlternativesData const& WTF::__throw_bad_variant_access<WebCore::DocumentMarker::DictationAlternativesData const&>(char const*)
3   0x11b41f781 WTF::__indexed_type<3l, bool, WTF::String, WebCore::DocumentMarker::DictationData, WebCore::DocumentMarker::DictationAlternativesData, WebCore::DocumentMarker::DraggedContentData>::__type const& WTF::get<3l, bool, WTF::String, WebCore::DocumentMarker::DictationData, WebCore::DocumentMarker::DictationAlternativesData, WebCore::DocumentMarker::DraggedContentData>(WTF::Variant<bool, WTF::String, WebCore::DocumentMarker::DictationData, WebCore::DocumentMarker::DictationAlternativesData, WebCore::DocumentMarker::DraggedContentData> const&)
4   0x11b41f745 WebCore::DocumentMarker::DictationAlternativesData const& WTF::get<WebCore::DocumentMarker::DictationAlternativesData, bool, WTF::String, WebCore::DocumentMarker::DictationData, WebCore::DocumentMarker::DictationAlternativesData, WebCore::DocumentMarker::DraggedContentData>(WTF::Variant<bool, WTF::String, WebCore::DocumentMarker::DictationData, WebCore::DocumentMarker::DictationAlternativesData, WebCore::DocumentMarker::DraggedContentData> const&)
5   0x11b41d499 WebCore::DocumentMarker::alternatives() const
6   0x11cdc2d89 WebCore::CompositeEditCommand::replaceTextInNodePreservingMarkers(WebCore::Text&, unsigned int, unsigned int, WTF::String const&)
7   0x11cddc979 WebCore::DeleteSelectionCommand::fixupWhitespace()
8   0x11cddedaf WebCore::DeleteSelectionCommand::doApply()
9   0x11cdc07bf WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::DumbPtrTraits<WebCore::EditCommand> >&&)
10  0x11cdc3411 WebCore::CompositeEditCommand::deleteSelection(WebCore::VisibleSelection const&, bool, bool, bool, bool, bool)
11  0x11ce66c88 WebCore::TypingCommand::deleteKeyPressed(WebCore::TextGranularity, bool)
12  0x11ce65ac8 WebCore::TypingCommand::deleteKeyPressed(WebCore::Document&, unsigned int, WebCore::TextGranularity)
13  0x11ce0aafa WebCore::executeDelete(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)
14  0x11cdf9233 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
15  0x11cba9a49 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)
16  0x11b49c418 WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::ExecState*, WebCore::JSDocument*, JSC::ThrowScope&)
17  0x11b47d7c6 long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::ExecState*, WebCore::JSDocument*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*)
18  0x11b47d4cc WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*)
19  0x5169cf62177
20  0x1174acc30 llint_entry
21  0x1174acc30 llint_entry
22  0x1174accaa llint_entry
23  0x1174accaa llint_entry
24  0x1174accaa llint_entry
25  0x1174accaa llint_entry
26  0x1174a4696 vmEntryToJavaScript
27  0x117d4c6da JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
28  0x117d4bc69 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
29  0x118025c3b JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
30  0x118025df0 JSC::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
31  0x11c7119bb WebCore::JSExecState::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
LEAK: 1 WebPageProxy

[Daniel Bates]

(In reply to David Fenton (:realdawei) from comment #19)
> (In reply to David Fenton (:realdawei) from comment #18)
> > (In reply to Daniel Bates from comment #16)
> > > Comment on attachment 347721 [details]
> > > For landing
> > > 
> > > Clearing flags on attachment: 347721
> > > 
> > > Committed r235149: <https://trac.webkit.org/changeset/235149>
> > 
> > test editing/undo/replace-text-in-node-preserving-markers-crash.html is
> > crashing consistently after this revision:
> > 
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=editing%2Fundo%2Freplace-text-in-node-preserving-
> > markers-crash.html
> 
> sample crash log:
> https://build.webkit.org/results/
> Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r235225%20(6059)/
> editing/undo/replace-text-in-node-preserving-markers-crash-crash-log.txt
> 
> stderr:
> 1   0x1171509e9 WTFCrash
> 2   0x11b41f7f7 WebCore::DocumentMarker::DictationAlternativesData const&
> WTF::__throw_bad_variant_access<WebCore::DocumentMarker::
> DictationAlternativesData const&>(char const*)
> 3   0x11b41f781 WTF::__indexed_type<3l, bool, WTF::String,
> WebCore::DocumentMarker::DictationData,
> WebCore::DocumentMarker::DictationAlternativesData,
> WebCore::DocumentMarker::DraggedContentData>::__type const& WTF::get<3l,
> bool, WTF::String, WebCore::DocumentMarker::DictationData,
> WebCore::DocumentMarker::DictationAlternativesData,
> WebCore::DocumentMarker::DraggedContentData>(WTF::Variant<bool, WTF::String,
> WebCore::DocumentMarker::DictationData,
> WebCore::DocumentMarker::DictationAlternativesData,
> WebCore::DocumentMarker::DraggedContentData> const&)
> 4   0x11b41f745 WebCore::DocumentMarker::DictationAlternativesData const&
> WTF::get<WebCore::DocumentMarker::DictationAlternativesData, bool,
> WTF::String, WebCore::DocumentMarker::DictationData,
> WebCore::DocumentMarker::DictationAlternativesData,
> WebCore::DocumentMarker::DraggedContentData>(WTF::Variant<bool, WTF::String,
> WebCore::DocumentMarker::DictationData,
> WebCore::DocumentMarker::DictationAlternativesData,
> WebCore::DocumentMarker::DraggedContentData> const&)
> 5   0x11b41d499 WebCore::DocumentMarker::alternatives() const
> 6   0x11cdc2d89
> WebCore::CompositeEditCommand::replaceTextInNodePreservingMarkers(WebCore::
> Text&, unsigned int, unsigned int, WTF::String const&)
> 7   0x11cddc979 WebCore::DeleteSelectionCommand::fixupWhitespace()
> 8   0x11cddedaf WebCore::DeleteSelectionCommand::doApply()
> 9   0x11cdc07bf
> WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::
> EditCommand, WTF::DumbPtrTraits<WebCore::EditCommand> >&&)
> 10  0x11cdc3411
> WebCore::CompositeEditCommand::deleteSelection(WebCore::VisibleSelection
> const&, bool, bool, bool, bool, bool)
> 11  0x11ce66c88
> WebCore::TypingCommand::deleteKeyPressed(WebCore::TextGranularity, bool)
> 12  0x11ce65ac8 WebCore::TypingCommand::deleteKeyPressed(WebCore::Document&,
> unsigned int, WebCore::TextGranularity)
> 13  0x11ce0aafa WebCore::executeDelete(WebCore::Frame&, WebCore::Event*,
> WebCore::EditorCommandSource, WTF::String const&)
> 14  0x11cdf9233 WebCore::Editor::Command::execute(WTF::String const&,
> WebCore::Event*) const
> 15  0x11cba9a49 WebCore::Document::execCommand(WTF::String const&, bool,
> WTF::String const&)
> 16  0x11b49c418
> WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::ExecState*,
> WebCore::JSDocument*, JSC::ThrowScope&)
> 17  0x11b47d7c6 long long
> WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::
> jsDocumentPrototypeFunctionExecCommandBody(JSC::ExecState*,
> WebCore::JSDocument*, JSC::ThrowScope&)),
> (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*)
> 18  0x11b47d4cc
> WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*)
> 19  0x5169cf62177
> 20  0x1174acc30 llint_entry
> 21  0x1174acc30 llint_entry
> 22  0x1174accaa llint_entry
> 23  0x1174accaa llint_entry
> 24  0x1174accaa llint_entry
> 25  0x1174accaa llint_entry
> 26  0x1174a4696 vmEntryToJavaScript
> 27  0x117d4c6da JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
> 28  0x117d4bc69 JSC::Interpreter::executeProgram(JSC::SourceCode const&,
> JSC::ExecState*, JSC::JSObject*)
> 29  0x118025c3b JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&,
> JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
> 30  0x118025df0 JSC::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason,
> JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
> 31  0x11c7119bb WebCore::JSExecState::profiledEvaluate(JSC::ExecState*,
> JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue,
> WTF::NakedPtr<JSC::Exception>&)
> LEAK: 1 WebPageProxy

This crash is not new and can be reproduced in DumpRenderTree using the following command line:

Tools/Scripts/run-webkit-tests -1 --debug --no-timeout --no-retry-failures --no-sample-on-timeout --child-processes 1 --iterations 10 --batch-size 10000 --ios-simulator editing/undo/replace-text-in-node-preserving-markers-crash.html

We did not notice this crash because we do not run WebKit1 tests (i.e. use DumpRenderTree) in iOS Simulator on build.webkit.org. The reason we are seeing this crash now, following <https://trac.webkit.org/changeset/235149> (the patch for bug), is because running editing/undo/replace-text-in-node-preserving-markers-crash.html causes WebCore to check for misspellings and now we actually check for misspellings (the patch for this bug hooked it up). Prior to <https://trac.webkit.org/changeset/235149> when WebCore asked the platform to spell checking on iOS in WebKit2 we did nothing.

[Daniel Bates]

(In reply to David Fenton (:realdawei) from comment #18)
> (In reply to Daniel Bates from comment #16)
> > Comment on attachment 347721 [details]
> > For landing
> > 
> > Clearing flags on attachment: 347721
> > 
> > Committed r235149: <https://trac.webkit.org/changeset/235149>
> 
> test editing/undo/replace-text-in-node-preserving-markers-crash.html is
> crashing consistently after this revision:
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=editing%2Fundo%2Freplace-text-in-node-preserving-
> markers-crash.html

Filed bug #188898 to fix this.

[Dawei Fenton (:realdawei)]

(In reply to Daniel Bates from comment #21)
> (In reply to David Fenton (:realdawei) from comment #18)
> > (In reply to Daniel Bates from comment #16)
> > > Comment on attachment 347721 [details]
> > > For landing
> > > 
> > > Clearing flags on attachment: 347721
> > > 
> > > Committed r235149: <https://trac.webkit.org/changeset/235149>
> > 
> > test editing/undo/replace-text-in-node-preserving-markers-crash.html is
> > crashing consistently after this revision:
> > 
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=editing%2Fundo%2Freplace-text-in-node-preserving-
> > markers-crash.html
> 
> Filed bug #188898 to fix this.

thanks!