Bug 195795

Summary: Add a platform-driven spell-checking mechanism
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, darin, dbates, ews-watchlist, rniwa, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
wip
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Patch none

Description Tim Horton 2019-03-15 01:46:30 PDT
Add an platform-driven spell-checking mechanism
Comment 1 Tim Horton 2019-03-15 01:47:31 PDT
Created attachment 364782 [details]
wip
Comment 2 EWS Watchlist 2019-03-15 01:50:36 PDT
Attachment 364782 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DocumentMarker.h:112:  Missing spaces around <  [whitespace/operators] [3]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Tim Horton 2019-03-15 01:59:03 PDT
This is at best a prototype, and is missing a whole bunch of things. Here are my big concerns:

- probably need to use the right editing command, not replaceSelectionWithText
- performance cost of doing so much TextIterator
- what about all the things that aren't Spelling and Grammar (links? etc.)
- what if the selection changes? how can we validate the selection/offsets
- I don't think the selection quite ends back up in the right place all the time (there's much more complicated code in Editor for this, but I don't know enough about editing to know why)
- who knows if I've really disabled all of the normal spell checking things, or if there's a better bottleneck
Comment 4 Tim Horton 2019-03-15 15:09:06 PDT
Created attachment 364852 [details]
Patch
Comment 5 EWS Watchlist 2019-03-15 15:12:44 PDT
Attachment 364852 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DocumentMarker.h:112:  Missing spaces around <  [whitespace/operators] [3]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Tim Horton 2019-03-15 17:21:48 PDT
Created attachment 364889 [details]
Patch
Comment 7 EWS Watchlist 2019-03-15 17:24:24 PDT
Attachment 364889 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DocumentMarker.h:112:  Missing spaces around <  [whitespace/operators] [3]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Ryosuke Niwa 2019-03-15 22:03:57 PDT
Comment on attachment 364889 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.mm:110
> +        frameSelection.moveTo(range.get());
> +        frame.editor().replaceSelectionWithText([annotatedString.string string], WebCore::Editor::SelectReplacement::No, WebCore::Editor::SmartReplace::No);

We want to be using SpellingCorrectionCommand instead. Otherwise, we won't fire the right type of input events.
Comment 9 Ryosuke Niwa 2019-03-15 22:06:41 PDT
(In reply to Tim Horton from comment #3)
> This is at best a prototype, and is missing a whole bunch of things. Here
> are my big concerns:
> 
> - probably need to use the right editing command, not
> replaceSelectionWithText

Indeed, we want to use SpellingCorrectionCommand.

> - performance cost of doing so much TextIterator

This is probably okay.

> - I don't think the selection quite ends back up in the right place all the
> time (there's much more complicated code in Editor for this, but I don't
> know enough about editing to know why)

The tricky case is when there are multiple corrections coming on our way. e.g. auto-linkify then smart-quote at the same time. How are those things implemented with this API?

In general, the current approach of using selection-relative offsets makes me uneasy. There is no guarantee that the selection offsets don't change between the time we went to UIKit and by the time we receive the message back in the web content process. Usually, we use the offsets relative to the root editable element. Can we do that somehow?

> - who knows if I've really disabled all of the normal spell checking things,
> or if there's a better bottleneck

Well, I guess we'd have to start from somewhere.
Comment 10 Tim Horton 2019-03-18 20:39:11 PDT
(In reply to Ryosuke Niwa from comment #9)
> (In reply to Tim Horton from comment #3)
> > This is at best a prototype, and is missing a whole bunch of things. Here
> > are my big concerns:
> > 
> > - probably need to use the right editing command, not
> > replaceSelectionWithText
> 
> Indeed, we want to use SpellingCorrectionCommand.

Yeah, I've made that change.

> > - I don't think the selection quite ends back up in the right place all the
> > time (there's much more complicated code in Editor for this, but I don't
> > know enough about editing to know why)
> 
> The tricky case is when there are multiple corrections coming on our way.
> e.g. auto-linkify then smart-quote at the same time. How are those things
> implemented with this API?

Like I mentioned, at the moment it only does spelling and grammar. But in general, they can push as many updates (and changes of different kinds!) as they want in a single message.

> In general, the current approach of using selection-relative offsets makes
> me uneasy. There is no guarantee that the selection offsets don't change
> between the time we went to UIKit and by the time we receive the message
> back in the web content process. Usually, we use the offsets relative to the
> root editable element. Can we do that somehow?

Not right now, no. And this "range relative to selection" thing is very common in our interface with UIKit. I think it's generally OK because UIKit drives the selection in the general case, but obviously things will fall down if the page starts moving it around. But I think that's already true for many other interactions.

I'll bring it up with other folks and see what they think, but we'll start here.
Comment 11 Tim Horton 2019-03-19 00:05:02 PDT
Created attachment 365129 [details]
Patch
Comment 12 Tim Horton 2019-03-19 12:23:01 PDT
Created attachment 365202 [details]
Patch
Comment 13 EWS Watchlist 2019-03-19 12:28:34 PDT
Attachment 365202 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DocumentMarker.h:112:  Missing spaces around <  [whitespace/operators] [3]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 EWS Watchlist 2019-03-19 17:14:29 PDT
Comment on attachment 365202 [details]
Patch

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

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html
Comment 15 EWS Watchlist 2019-03-19 17:14:30 PDT
Created attachment 365266 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 16 Ryosuke Niwa 2019-03-19 19:49:20 PDT
Comment on attachment 365202 [details]
Patch

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

r=me. I guess we can polish the code more as the related radar gets resolved.

> Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.mm:73
> +RefPtr<Range> TextCheckingControllerProxy::rangeAndOffsetRelativeToSelection(int64_t offset, uint64_t length, size_t& outLocationInRoot)

Make this return optional<struct { Ref<Range> range; size_t locationInRoot; }> instead of taking an out argument?

> Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.mm:107
> +        bool restoreSelection = frameSelection.selection().selectionType() == VisibleSelection::CaretSelection;

Just check frameSelection.selection().isRange()

> Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.mm:109
> +        Editor::replaceRangeForSpellChecking(*range, [annotatedString.string string]);

Can we make this a regular instance member function? We already have frame here.
Comment 17 Tim Horton 2019-03-19 23:52:40 PDT
Created attachment 365322 [details]
Patch
Comment 18 EWS Watchlist 2019-03-19 23:54:03 PDT
Attachment 365322 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DocumentMarker.h:112:  Missing spaces around <  [whitespace/operators] [3]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 WebKit Commit Bot 2019-03-20 00:32:46 PDT
Comment on attachment 365322 [details]
Patch

Clearing flags on attachment: 365322

Committed r243195: <https://trac.webkit.org/changeset/243195>
Comment 20 WebKit Commit Bot 2019-03-20 00:32:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2019-03-20 00:33:22 PDT
<rdar://problem/49053492>