Bug 40092 – spellcheck does not check pasted text

• Component: HTML Editing
• Status: RESOLVED FIXED

[Tony Chang]

For example, copy "foo baz" into your clipboard and paste it into a bugzilla comment.  "foo" doesn't get a red underline because it isn't checked.

This is similar to bug 37050.

[Alexey Proskuryakov]

Interestingly, TextEdit on Mac OS X 10.5 has similar behavior - but that's not the case on 10.6 for me.

[Hironori Bono]

Created attachment 60861 [details]
A draft implementation (only for discussion)

Greetings,

I have quickly implemented an asynchronous spellchecker which uses [NSSpellChecker requestCheckingOfString:] so we can check the spellings (and grammar) of pasted text in the background on 10.6. Unfortunately, this change needs xcode 3.2 to compile it because it depends on GCD (Grand Central Dispatch). Even though this change does not include layout tests and is not ready for review, it may be good for starting a discussion to solve this issue. (I'm writing a change for <https://bugs.webkit.org/show_bug.cgi?id=41832> to write a layout test for this issue.)

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 61480 [details]
Another draft implementation

Greetings,

Even though nobody gave me any comments to my previous one, I have added a layout test to start a review process. Is it possible to review this change?

Regards,

Hironori Bono

[WebKit Review Bot]

Attachment 61480 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/editing/Editor.cpp:1196:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/mac/WebView/WebFrame.mm:1622:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 61480 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3528128

[Hironori Bono]

Created attachment 61485 [details]
Draft implementation #2

Sorry, my previous change causes a build break on GTK.

Regards,

Hironori Bono

[WebKit Review Bot]

Attachment 61485 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/editing/Editor.cpp:1198:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/mac/WebView/WebFrame.mm:1622:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hironori Bono]

Created attachment 61488 [details]
Draft implementation #3

Sorry for my annoying updates. I have fixed style issues in my previous change.

Regards,

Hironori Bono

[WebKit Review Bot]

Attachment 61480 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3533064

[WebKit Review Bot]

Attachment 61480 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3545048

[Eric Seidel (no email)]

IIRC Doug did the recent spell-checking work in WebKit and might want to see this.

[Douglas Davidson]

Yes, I'm definitely interested in looking at this.  Unfortunately I will not be available until 8/13, but I would like to see an asynchronous implementation.  Jia Pu is also working in this area.

[Eric Seidel (no email)]

Comment on attachment 61488 [details]
Draft implementation #3

WebCore/editing/Editor.cpp:1172
 +      String text;
I would have put this "get me the text I need to process" block in its own function to make this one simpler.

WebCore/editing/Editor.cpp:1187
 +  Node* Editor::getSpellingNode(Node* node)
We don't use get in names in WebKit.

WebCore/editing/Editor.cpp:1236
 +      m_spellingNode = 0;
This could be a set or reset function given how you always set these both at once.  Maybe these need to be a struct held by the object?

WebCore/editing/Editor.cpp:1207
 +      if (node) {
Early return is almost always cleaner.

WebCore/editing/Editor.cpp:1220
 +              Position end(start);
We have a PositionIterator for this sort of thing.

WebCore/editing/Editor.cpp:1230
 +              String source(m_spellingText.substring(results[i].location, results[i].length));
Personally I tend to use = over assignment constructors myself.

WebCore/editing/Editor.h:70
 +          : type(DocumentMarker::Spelling), location(0), length(0) { }
Each assignment gets its own line.

WebCore/editing/Editor.h:350
 +      Node* getSpellingNode(Node*);
Again, no "get" functions in WebKit.

WebKit/mac/ChangeLog:31
 +  2010-07-14  Hironori Bono  <[e-mail]>
Double ChangeLog entry!

WebKit/mac/WebCoreSupport/WebEditorClient.mm:827
 +  #if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_6 && NS_BLOCKS_AVAILABLE
NS_BLOCKS_AVAILABLE?

WebKit/mac/WebCoreSupport/WebEditorClient.mm:839
 +      [[NSSpellChecker sharedSpellChecker] requestCheckingOfString:text range:NSMakeRange(0, text.length()) types:NSTextCheckingAllSystemTypes options:0 inSpellDocumentWithTag:0 completionHandler:^(NSInteger sequenceNumber, NSArray *results, NSOrthography *orthography, NSInteger wordCount) {
Woh.  Please split this into multiple lines using local variables.

WebKit/mac/WebView/WebFrame.mm:1596
 +          switch ([[results objectAtIndex:i] resultType]) {
I would have put this switch into an inline function instead of inlining it here to make this loop easier to read.

WebKit/mac/WebView/WebFrame.mm:1614
 +  static Node* getSpellingNode(Frame* coreFrame)
No "get" in function names.

[Hironori Bono]

Created attachment 63034 [details]
Draft implementation #4

Greetings Eric,

Thank you for your review and comments.
I have added the AsyncSpellCheckerController class to encapsulate some spellchecker-specific code and apply your comments. (I may miss some of them, though.)

(In reply to comment #13)
> WebKit/mac/WebCoreSupport/WebEditorClient.mm:827
>  +  #if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_6 && NS_BLOCKS_AVAILABLE
> NS_BLOCKS_AVAILABLE?

This define means we can use GCD (Grand Central Dispatch), which is required by [NSSpellChecker requestCheckingOfString:]. (Even though GCD is enabled by default on 10.6, we can disable it. So it may be safer to add this define in my code.)

It is definitely helpful to review the updated one and give me your comments?

Regards,

Hironori Bono

[Hironori Bono]

ping?

[Hironori Bono]

Created attachment 65679 [details]
Draft implementation #5

Greetings,

Even though nobody reviewed my pervious change, I would like to update my change because my previous change does not compile any longer. Would it be possible to give me your comments?

Regards,

Hironori Bono

[Adam Barth]

Comment on attachment 65679 [details]
Draft implementation #5

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

> WebCore/editing/Editor.cpp:321
> +#if PLATFORM(MAC)
> +    requestCheckingOfFocusedNodeText();
> +#endif
I don't understand why this code has so many #if defs.  Should this code be in a Mac-specific file?

[Hironori Bono]

(In reply to comment #17)
> > WebCore/editing/Editor.cpp:321
> > +#if PLATFORM(MAC)
> > +    requestCheckingOfFocusedNodeText();
> > +#endif
> I don't understand why this code has so many #if defs.  Should this code be in a Mac-specific file?

Thank you for your comment.
In fact, most code under "WebCore" does not need #if defs. Nevertheless, I enclosed them with #if and #endif since this code needs special functions like [requestCheckingOfString:] or threads. Therefore, I'm wondering if other platforms except Mac (and Chromium) would like to use this code. (The Chromium implementation is a little more complicated: it consists of a WebKit change and a Chromium change. So, I would like to send them with another change.)

Regards,

[Hironori Bono]

(In reply to comment #17)
> I don't understand why this code has so many #if defs.  Should this code be in a Mac-specific file?

By the way, I'm a little wondering about the intention of your comment. Would you like to fix this issue only by changing Mac-specific files, to fix this issue without using Mac-specific functions, or something else?

Regards,

[Darin Adler]

Comment on attachment 65679 [details]
Draft implementation #5

Our usual approach would be to put in fewer ifdefs, and compile in more of the infrastructure for the feature even on platforms that don't have it yet.

Editor.h should not have all those PLATFORM(MAC) #if statements.

It’s also not good to add new classes to existing files. We probably want to add new source files.

[Hironori Bono]

Comment on attachment 65679 [details]
Draft implementation #5

Greetings,

I would like to cancel this review request since Morita-san kindly takes over this bug.

Regards,

[Hajime Morrita]

Created attachment 66859 [details]
Patch

[Hajime Morrita]

Hi Darin, thank you for reviewing!
As Bono-san mentioned, I'm taking this patch over him.

> 
> Editor.h should not have all those PLATFORM(MAC) #if statements.
> 
> It’s also not good to add new classes to existing files. We probably want to add new source files.

Updated patch introduce SpellChecker class, which defined in separated files.
The patch also attempt to minimize ifdefs.
Could you take a look at it?

[Hajime Morrita]

Created attachment 66879 [details]
sync up to the latest tree

[Eric Seidel (no email)]

Attachment 66879 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3966293

[Hajime Morrita]

Created attachment 66988 [details]
An attempt to fix build on Loopard

[Eric Seidel (no email)]

Attachment 66988 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3930286

[Hajime Morrita]

Created attachment 66994 [details]
Another attempt to fix build on Loopard

[Hajime Morrita]

Created attachment 67016 [details]
Saved the buildbot delay

[Hajime Morrita]

Long queue of windows bot prevents patches with new file from built successfully.
I'd like to leave it to avoid making the queue longer.

[Hajime Morrita]

Created attachment 70518 [details]
took care of textInput event

[Hajime Morrita]

Created attachment 70592 [details]
Updated to lastest

[Early Warning System Bot]

Attachment 70592 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4406004

[Hajime Morrita]

Created attachment 70599 [details]
Fixing Qt WebKit2 build

[Eric Seidel (no email)]

Comment on attachment 70599 [details]
Fixing Qt WebKit2 build

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

This generally appears sane.  It would be nice if Doug or one of the Apple folks could comment on the Obj-C.  r- for the nits.

> WebCore/dom/DocumentMarkerController.h:77
> +void showDocumentMarkers(const WebCore::DocumentMarkerController*);

Why is this outside of WebCore?

> WebCore/editing/Editor.cpp:3663
> +static Node* findFirstMarkable(Node* node)

We didn't have this logic before?

> WebCore/editing/SpellChecker.h:35
> +struct SpellCheckingResult {

Structs rarely stay structs for long...

> WebCore/editing/SpellChecker.h:48
> +class SpellChecker {

Seems this should inherit from NonCopyable.

> WebCore/editing/SpellChecker.h:53
> +    bool isAsynchronousEnabled() const;

is "Enabled" needed here?

> WebCore/editing/SpellChecker.h:54
> +    bool isAsynchronousAvailableFor(Node*) const;

canCheckAsynchronously(node)?  I'm not sure what this function does so my naming suggestions may make little sense.

> WebCore/editing/SpellChecker.h:57
> +    bool isCheckeable(Node* node) const;

isCheckable?  Also, "node" is not needed here.

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:954
> +    RetainPtr<NSArray> _results;

Seems a bit odd, but I guess this is Objective c++?  Are destructors called for obj-c++?  Isn't there a new fancy obj-c2 way to do autoretain?

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:978
> +- (WTF::Vector<WebCore::SpellCheckingResult>) _coreResults

Extra space "lt>) _coreR"

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:995
> +    for (size_t i = 0; i < [results count]; ++i) {
> +        NSTextCheckingType type = [[results objectAtIndex:i] resultType];
> +        if (type & NSTextCheckingTypeSpelling)
> +            coreResults[i].type = DocumentMarker::Spelling;
> +        else if (type & NSTextCheckingTypeGrammar)
> +            coreResults[i].type = DocumentMarker::Grammar;
> +        else
> +            coreResults[i].type = DocumentMarker::AllMarkers;
> +
> +        NSRange range = [[results objectAtIndex:i] range];
> +        coreResults[i].location = range.location;
> +        coreResults[i].length = range.length;
> +    }

This feels like a function passed to a map.  Sadly I don't think obj-c has list comprehensions.

[Douglas Davidson]

We'll need to look at this and see how it meshes with what we're doing.

[Darin Adler]

(In reply to comment #35)
> > WebKit/mac/WebCoreSupport/WebEditorClient.mm:954
> > +    RetainPtr<NSArray> _results;
> 
> Seems a bit odd, but I guess this is Objective c++?  Are destructors called for obj-c++?

Yes.

> Isn't there a new fancy obj-c2 way to do autoretain?

The fancy new Objective-C 2 way to do it would be to have property access handle the retain/release or to use garbage collection.

Using a RetainPtr is the right way to do it in WebKit.

[Hajime Morrita]

Created attachment 71426 [details]
Patch

[Hajime Morrita]

Created attachment 71427 [details]
Patch

[Hajime Morrita]

Eric and Everybody, thanks for giving a review!
I update the patch to address Eric's point.

> > WebCore/dom/DocumentMarkerController.h:77
> > +void showDocumentMarkers(const WebCore::DocumentMarkerController*);
> 
> Why is this outside of WebCore?
It follows RenderObject.cpp:showRenderTree() style. 
The function is designed to be called from gdb, so be in global namespace matters.

> 
> > WebCore/editing/Editor.cpp:3663
> > +static Node* findFirstMarkable(Node* node)
> 
> We didn't have this logic before?
Unfortunately, no. Other part of spellchecking is immediately done once
It finds a candidate to mark. So they need no extra traversing. 
But we need it because we mark them later.

> 
> > WebCore/editing/SpellChecker.h:35
> > +struct SpellCheckingResult {
> 
> Structs rarely stay structs for long...
Agreed. Made it to a class.

> 
> > WebCore/editing/SpellChecker.h:48
> > +class SpellChecker {
> 
> Seems this should inherit from NonCopyable.
Done.

> 
> > WebCore/editing/SpellChecker.h:53
> > +    bool isAsynchronousEnabled() const;
> 
> is "Enabled" needed here?
I think so. It is asking EditorClient to the setting.

> 
> > WebCore/editing/SpellChecker.h:54
> > +    bool isAsynchronousAvailableFor(Node*) const;
> 
> canCheckAsynchronously(node)?  I'm not sure what this function does so my naming suggestions may make little sense.
Done. I agree that brief name is preferable.

> 
> > WebCore/editing/SpellChecker.h:57
> > +    bool isCheckeable(Node* node) const;
> 
> isCheckable?  Also, "node" is not needed here.
Done.

> 
> > WebKit/mac/WebCoreSupport/WebEditorClient.mm:954
> > +    RetainPtr<NSArray> _results;
> 
> Seems a bit odd, but I guess this is Objective c++?  Are destructors called for obj-c++?  Isn't there a new fancy obj-c2 way to do autoretain?
It looks OK, as Darin mentioned.

> 
> > WebKit/mac/WebCoreSupport/WebEditorClient.mm:978
> > +- (WTF::Vector<WebCore::SpellCheckingResult>) _coreResults
> 
> Extra space "lt>) _coreR"
Done.

> 
> > WebKit/mac/WebCoreSupport/WebEditorClient.mm:995
> > +    for (size_t i = 0; i < [results count]; ++i) {
> > +        NSTextCheckingType type = [[results objectAtIndex:i] resultType];
> > +        if (type & NSTextCheckingTypeSpelling)
> > +            coreResults[i].type = DocumentMarker::Spelling;
> > +        else if (type & NSTextCheckingTypeGrammar)
> > +            coreResults[i].type = DocumentMarker::Grammar;
> > +        else
> > +            coreResults[i].type = DocumentMarker::AllMarkers;
> > +
> > +        NSRange range = [[results objectAtIndex:i] range];
> > +        coreResults[i].location = range.location;
> > +        coreResults[i].length = range.length;
> > +    }
> 
> This feels like a function passed to a map.  Sadly I don't think obj-c has list comprehensions.
Agreed but there is no map() from NSArray to WTF::Vector.
So I factored out the logic  to make it more map()-like feeling.

[Hajime Morrita]

(In reply to comment #36)
> We'll need to look at this and see how it meshes with what we're doing.
Hi Davidson!
Please take a look. Any feedback are appreciated.
I just this is not so large change. large chunks of the code is just a boilerplate.

[Jia Pu]

(In reply to comment #41)
> (In reply to comment #36)
> > We'll need to look at this and see how it meshes with what we're doing.
> Hi Davidson!
> Please take a look. Any feedback are appreciated.
> I just this is not so large change. large chunks of the code is just a boilerplate.

Since we don't do autocorrection for pasted text, this patch doesn't seem interfere with our recent work on autocorrection. But I'd like to apply to apply this patch on 10.7, see if everything work together. I will let you know.

My other concern is racing condition when multiple threads call  [NSSpellChecker requestCheckingOfString:]. Is it possible?

[Hajime Morrita]

Hi Jia, thank you for taking a look! 

> 
> Since we don't do autocorrection for pasted text, this patch doesn't seem interfere with our recent work on autocorrection. But I'd like to apply to apply this patch on 10.7, see if everything work together. I will let you know.

Hmm. on the other hand, we need this for chromium (with separate EditorClient  implementation)...
How about this? :
To land the patch, with the async check settings false.
And doing the experiments before landing it true (at 10.7 release?).
In this way, Chromium port can enable the feature 
with keeping other ports' behavior unchanged at this time.
(I hope every ports to enable the flag at a certain timing. 
But it might need more experiments as you mentioned.)

In my feeling, what really matters here is introducing the async spellcheck API.
Without it, we cannot run spellchecking against larger chunks of text 
like copy-paste, dynamic @spellcheck attribute change, etc.
And for Chromium (sorry for mentioning a specific port again...),
A grammar check could be done by a separate process so async API is unavoidable.

> 
> My other concern is racing condition when multiple threads call  [NSSpellChecker requestCheckingOfString:]. Is it possible?

Since WebKit is single-threaded, [NSSpellChecker requestCheckingOfString:] is 
only called from the main thread.
Although NSSpellChecker looks to run the actual check logic on another (worker) thread,
it is managed by NSSpellChecker. So we wouldn't care about that.

A tricky part is that the passed completionHandler is called at the worker thread.
To void a race, We go back to the main thread using [NSRunLoop performSelector:].
We touch WebCore objects only on the main thread by doing so.

[Jia Pu]

(In reply to comment #43)
> Hi Jia, thank you for taking a look! 
> 
> > 
> > Since we don't do autocorrection for pasted text, this patch doesn't seem interfere with our recent work on autocorrection. But I'd like to apply to apply this patch on 10.7, see if everything work together. I will let you know.
> 
> Hmm. on the other hand, we need this for chromium (with separate EditorClient  implementation)...
> How about this? :
> To land the patch, with the async check settings false.
I just tried the patch. It seems play well with autocorrection on Mac. This is likely a desirable behavior on Mac as well, but I think we should land it with async set to false for now, just to be safe.

> In my feeling, what really matters here is introducing the async spellcheck API.
> Without it, we cannot run spellchecking against larger chunks of text 
> like copy-paste, dynamic @spellcheck attribute change, etc.

Agree.

[Hajime Morrita]

Hi Jia,

> I just tried the patch. It seems play well with autocorrection on Mac. This is likely a desirable behavior on Mac as well, but I think we should land it with async set to false for now, just to be safe.

Thank you for trying this!
So I'll disable the default settings and add LayoutTestController method to make it enabled.

[Hajime Morrita]

Created attachment 72168 [details]
Patch

[Hajime Morrita]

Updated the patch. It
- introduced Settings::m_asynchronousSpellCheckingEnabled, false as default.
- added a LayoutTestController API to enable it.
- removed EditorClient::isAsynchronousSpellCheckingEnabled(), replaced by settings.

[Hajime Morrita]

Created attachment 72170 [details]
trying to make efl bot happy

[Hajime Morrita]

Created attachment 72177 [details]
resolving conflict

[Early Warning System Bot]

Attachment 72177 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4848049

[Eric Seidel (no email)]

Attachment 72177 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4836056

[WebKit Review Bot]

Attachment 72177 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4873014

[Hajime Morrita]

Created attachment 72283 [details]
trying to fix build

[Hajime Morrita]

Could anyone take a look ?
I think we got a consensus and the patch is ready to review.

Thanks in advance.

[Hajime Morrita]

Created attachment 73564 [details]
Updated to ToT

[Hajime Morrita]

Created attachment 74074 [details]
Patch

[Hajime Morrita]

Created attachment 76023 [details]
Updated to ToT

[Hajime Morrita]

Ojan, thank you reviewing!
Darin, Alexey, I'll land this if there is no objection.

[Hajime Morrita]

Committed r73886: <http://trac.webkit.org/changeset/73886>