Bug 195795 – Add a platform-driven spell-checking mechanism

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Add an platform-driven spell-checking mechanism

[Tim Horton]

Created attachment 364782 [details]
wip

[EWS Watchlist]

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.

[Tim Horton]

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

[Tim Horton]

Created attachment 364852 [details]
Patch

[EWS Watchlist]

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.

[Tim Horton]

Created attachment 364889 [details]
Patch

[EWS Watchlist]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Tim Horton]

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

[Tim Horton]

Created attachment 365129 [details]
Patch

[Tim Horton]

Created attachment 365202 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ryosuke Niwa]

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.

[Tim Horton]

Created attachment 365322 [details]
Patch

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 365322 [details]
Patch

Clearing flags on attachment: 365322

Committed r243195: <https://trac.webkit.org/changeset/243195>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49053492>