Bug 188800 - [iOS][WK2] Misspelled words are not underlined
Summary: [iOS][WK2] Misspelled words are not underlined
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad iOS 11
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2018-08-21 11:23 PDT by Daniel Bates
Modified: 2018-10-14 22:04 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.45 KB, patch)
2018-08-21 11:46 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (9.73 KB, patch)
2018-08-21 11:54 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (9.76 KB, patch)
2018-08-21 11:57 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.22 MB, application/zip)
2018-08-21 13:16 PDT, EWS Watchlist
no flags Details
Patch (9.87 KB, patch)
2018-08-21 13:51 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (9.94 KB, patch)
2018-08-21 14:34 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
For landing (10.15 KB, patch)
2018-08-21 16:15 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-08-21 11:23:20 PDT
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>.
Comment 1 Daniel Bates 2018-08-21 11:23:42 PDT
<rdar://problem/34811332>
Comment 2 Daniel Bates 2018-08-21 11:46:28 PDT
Created attachment 347665 [details]
Patch
Comment 3 Daniel Bates 2018-08-21 11:54:59 PDT
Created attachment 347667 [details]
Patch
Comment 4 Daniel Bates 2018-08-21 11:57:12 PDT
Created attachment 347668 [details]
Patch
Comment 5 Simon Fraser (smfr) 2018-08-21 12:16:23 PDT
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.
Comment 6 EWS Watchlist 2018-08-21 13:16:39 PDT
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
Comment 7 EWS Watchlist 2018-08-21 13:16:41 PDT
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
Comment 8 Daniel Bates 2018-08-21 13:45:28 PDT
(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.
Comment 9 Daniel Bates 2018-08-21 13:47:55 PDT
(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.
Comment 10 Daniel Bates 2018-08-21 13:51:17 PDT
Created attachment 347687 [details]
Patch
Comment 11 Daniel Bates 2018-08-21 14:34:06 PDT
Created attachment 347697 [details]
Patch
Comment 12 Wenson Hsieh 2018-08-21 14:36:30 PDT
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.
Comment 13 Wenson Hsieh 2018-08-21 14:38:11 PDT
(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>!
Comment 14 Daniel Bates 2018-08-21 14:43:45 PDT
(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.
Comment 15 Daniel Bates 2018-08-21 16:15:05 PDT
Created attachment 347721 [details]
For landing
Comment 16 Daniel Bates 2018-08-21 17:46:05 PDT
Comment on attachment 347721 [details]
For landing

Clearing flags on attachment: 347721

Committed r235149: <https://trac.webkit.org/changeset/235149>
Comment 17 Daniel Bates 2018-08-21 17:46:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Dawei Fenton (:realdawei) 2018-08-23 10:07:23 PDT
(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
Comment 19 Dawei Fenton (:realdawei) 2018-08-23 10:55:51 PDT
(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
Comment 20 Daniel Bates 2018-08-23 12:34:17 PDT
(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.
Comment 21 Daniel Bates 2018-08-23 12:40:17 PDT
(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.
Comment 22 Dawei Fenton (:realdawei) 2018-08-23 13:53:02 PDT
(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!