Bug 59693 – [Feature Request] Need SpellCheck API

• Component: WebCore Misc.
• Status: REOPENED

[Hironori Bono]

Greetings,

From the point of an extension developer, It would be great for WebKit to have a JavaScript API that encapsulates some functions of the TextCheckerHelper class.
I assume these functions are very useful for web-application developers to write a web application that integrates spell-checking functionarities:

* bool checkSpellingOfString(DOMString text, DOMString language)
Encapsulates TextCheckerClient::checkSpellingOfString(). That is, this function checks the spellings of the specified text with the spell-checker. If a spell-checker supports multiple languages, it uses the dictionary specified for the language parameter. This function returns true when the specified text is correct. Otherwise, it returns false.

* DOMStringList getSuggestions(DOMString queryWord, DOMString language)
Encapsulates TextCheckerClient::getSuggestions(). That is, this function returns the list of suggestions for the specified misspelled word. If the specified word is a correct word or the word does not have any suggestions. (The checkSpellingOfString() function should be used for distinguishing these cases.)

* void learnWord(DOMString word, DOMString language)
Encapsulates TextCheckerClient::learnWord(). That is, this function adds the specified word to the user-dictionary.

Also, this JavaScript function is pretty useful for those who write spell-checker extensions:
* void addMarker(Node node, long start, long length, DOMString suggestion)
Encapsulates DocumentMarkerController::addMaker(). That is, this function adds a misspelled underline and the suggestion to the specified range. (This function does not use a Range object because it is not so easy to use a Range object to specify text in a <textarea> element and an <input> element.

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 91478 [details]
A concept design (not ready for review)

Greetings,

This is a change that implements my idea. It would be great to give me your feedback.

Regards,

Hironori Bono

[Dimitri Glazkov (Google)]

(In reply to comment #1)
> Created an attachment (id=91478) [details]
> A concept design (not ready for review)
> 
> Greetings,
> 
> This is a change that implements my idea. It would be great to give me your feedback.
> 
> Regards,
> 
> Hironori Bono

Bono-san, I don't think a bug tracker is a good place to discuss a public API being added to the Web. It's great to have a patch that shows how easily it can be done, but I would highly recommend raising this on public-webapps or public-html, presenting use cases and some sort of a document that captures how the API works and what are the implementation requirements.

[Hironori Bono]

Greetings Dimitri,

(In reply to comment #2)

> Bono-san, I don't think a bug tracker is a good place to discuss a public API being added to the Web. It's great to have a patch that shows how easily it can be done, but I would highly recommend raising this on public-webapps or public-html, presenting use cases and some sort of a document that captures how the API works and what are the implementation requirements.

Thank you so much for giving these MLs. (I have not noticed them and I have been wondering where I should send my proposal.) I will send my proposal to the [e-mail] soon.

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 94413 [details]
Patch v0 (not ready for reviews)

Greetings,

I have quickly implemented the API suggested in [e-mail]. Even though this change only works on Chromium, I would like to upload it for your information.

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 94756 [details]
Patch v1 (only for Chromium)

Greetings,

I have updated my change to simplify my previous change. Even though this change is only for Chromium, it would be great to give me your feedback.

Regards,

Hironori Bono

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebCore/html/HTMLTextAreaElement.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/html/HTMLInputElement.cpp:64:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/html/HTMLDivElement.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/dom/DocumentMarkerController.h:85:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 30 files


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

[Hironori Bono]

Created attachment 94769 [details]
Patch v2 (only for Chromium)

Greetings,

(In reply to comment #6)
> Source/WebCore/html/HTMLTextAreaElement.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
> Source/WebCore/html/HTMLInputElement.cpp:64:  Alphabetical sorting problem.  [build/include_order] [4]
> Source/WebCore/html/HTMLDivElement.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
> Source/WebCore/dom/DocumentMarkerController.h:85:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Total errors found: 4 in 30 files

Oops, I forgot running check-webkit-style. I have updated my change.

Regards,

Hironori Bono

[Hajime Morrita]

Comment on attachment 94769 [details]
Patch v2 (only for Chromium)

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

Bono-san, thank you for doing this!
I did some nit picking. The overall shape looks good for me.
But I'd like to hear from other interested folks about its overall design.

> Source/WebCore/dom/DocumentMarker.h:63
> +#if ENABLE(SPELLCHECK_API)

This adds an extra "," for the enums. I think You can just add UserSpelling without ENABLE() guard.
It's never used in disabled case and there is no runtime overhead.
Auto-correction related enums do same thing.

> Source/WebCore/dom/DocumentMarkerController.cpp:686
> +    return result.release();

You don't need release()

> Source/WebCore/dom/DocumentMarkerController.cpp:689
> +bool DocumentMarkerController::addUserSpellingMarker(Node* node, unsigned start, int length, RefPtr<DOMStringList> suggestions, int options)

Can length be unsigned?

> Source/WebCore/dom/DocumentMarkerController.cpp:721
> +Node* DocumentMarkerController::userSpellingNode(Node* node) const

We can define this as a static function.

> Source/WebCore/html/SpellcheckRange.h:40
> +    static PassRefPtr<SpellcheckRange> create() { return adoptRef(new SpellcheckRange()); }

Inline functions should be defined after the class definition and class definition should be function-body free.
This isn't mandatory. But it's a recent preference.

> Source/WebCore/html/SpellcheckRangeList.h:31
> +#include "SpellcheckRange.h"

can we replace this with a forward declaration?

> Source/WebCore/rendering/InlineTextBox.cpp:961
> +#if ENABLE(SPELLCHECK_API)

We don't need these guards

> Source/WebKit/chromium/src/WebRuntimeFeatures.cpp:341
> +#if ENABLE(SPELLCHECK_API)

Please add UNUSED_PARAM() for disabled path.

> LayoutTests/ChangeLog:1
> [phone]  Hironori Bono  <[e-mail]>

Please update skipped lists for unsupported platforms.

[Hironori Bono]

Created attachment 94961 [details]
Patch v2 (only for Chromium)

(In reply to comment #8)

Thank you for your review and comments. Even though I have not applied your comments totally, I have uploaded the updated change because I have uploaded an old version yesterday.

> > Source/WebCore/dom/DocumentMarker.h:63
> > +#if ENABLE(SPELLCHECK_API)
> 
> This adds an extra "," for the enums. I think You can just add UserSpelling without ENABLE() guard.
> It's never used in disabled case and there is no runtime overhead.
> Auto-correction related enums do same thing.

Thank you for your suggestion. I have removed the guard here and added a comma at the end of this line.
 
> > Source/WebCore/dom/DocumentMarkerController.cpp:689
> > +bool DocumentMarkerController::addUserSpellingMarker(Node* node, unsigned start, int length, RefPtr<DOMStringList> suggestions, int options)
> 
> Can length be unsigned?

Ah, yes. It is better to use unsigned since the callers use unsigned.
 
> > Source/WebCore/html/SpellcheckRangeList.h:31
> > +#include "SpellcheckRange.h"
> 
> can we replace this with a forward declaration?

Thank you for your catch. Yes, we can replace it. I have replaced this include with a forward declaration.
 
> > Source/WebCore/rendering/InlineTextBox.cpp:961
> > +#if ENABLE(SPELLCHECK_API)
> 
> We don't need these guards

Thank you for your comment. I have removed them.
 
> > Source/WebKit/chromium/src/WebRuntimeFeatures.cpp:341
> > +#if ENABLE(SPELLCHECK_API)
> 
> Please add UNUSED_PARAM() for disabled path.

Indeed. I forgot adding UNUSED_PARAM() here.

> > LayoutTests/ChangeLog:1
> > [phone]  Hironori Bono  <[e-mail]>
> 
> Please update skipped lists for unsupported platforms.

Ok. I have added my new test to Skipped files.

Regards,

Hironori Bono

[Hajime Morrita]

Comment on attachment 94961 [details]
Patch v2 (only for Chromium)

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

Hi Bono-san, thank you for update.
You can add Skipped entries where other editing or spelling test is listed.
It would reduce the chance of conflict.

> Source/WebCore/dom/DocumentMarkerController.cpp:677
> +        DocumentMarker marker = list->at(i);

Could you use const refrence if possible? It will save an extra copy.

> Source/WebCore/dom/DocumentMarkerController.cpp:683
> +        result->append(SpellcheckRange::create(marker.startOffset(), marker.endOffset() - marker.startOffset(), suggestions.release(), 0));

Could you add length() to DocumentMarker for here? It would be illustrative.

> Source/WebCore/html/HTMLDivElement.idl:27
> +        void addSpellcheckRange(in unsigned long start, in unsigned long length, in [Optional] DOMStringList suggestions, in [Optional] unsigned short options);

Don't we throw any exceptions? It might be a question about the spec though.

> LayoutTests/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=59693

Can we have a test pixel test for that to ensure that the user-marker is actually visible?

[Hajime Morrita]

Comment on attachment 94961 [details]
Patch v2 (only for Chromium)

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

> Source/WebCore/html/SpellcheckRange.cpp:40
> +{

Another spec-related question is coming up: Is  "sugestions" empty or null when there is no suggestion?

[Hironori Bono]

Created attachment 95156 [details]
Patch v3 (only for Chromium)

Greetings Morita-san,

Thank you for your review and comments.
I have applied your comments and updated the change.

(In reply to comment #10)

> Hi Bono-san, thank you for update.
> You can add Skipped entries where other editing or spelling test is listed.
> It would reduce the chance of conflict.

Indeed. It is definitely better to avoid unnecessary conflicts in Skipped files.
 
> > Source/WebCore/dom/DocumentMarkerController.cpp:677
> > +        DocumentMarker marker = list->at(i);
> 
> Could you use const refrence if possible? It will save an extra copy.

Done. Thank you for noticing it.
 
> > Source/WebCore/dom/DocumentMarkerController.cpp:683
> > +        result->append(SpellcheckRange::create(marker.startOffset(), marker.endOffset() - marker.startOffset(), suggestions.release(), 0));
> 
> Could you add length() to DocumentMarker for here? It would be illustrative.

OK. I have added the length() method.
 
> > Source/WebCore/html/HTMLDivElement.idl:27
> > +        void addSpellcheckRange(in unsigned long start, in unsigned long length, in [Optional] DOMStringList suggestions, in [Optional] unsigned short options);
> 
> Don't we throw any exceptions? It might be a question about the spec though.

Even though the current API does not send exceptions, we will hear opinions of users with actual implementation and clearify such ambiguous issues.
 
> > LayoutTests/ChangeLog:6
> > +        https://bugs.webkit.org/show_bug.cgi?id=59693
> 
> Can we have a test pixel test for that to ensure that the user-marker is actually visible?

OK. I have added another test. (I prefer script tests, though.)

Regards,

Hironori Bono

[WebKit Review Bot]

Comment on attachment 95156 [details]
Patch v3 (only for Chromium)

Attachment 95156 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8747030

[Dimitri Glazkov (Google)]

Adding some Apple folks.

[Alexey Proskuryakov]

As this is a new feature, please send an e-mail to webkit-dev for discussion.

You have previously discussed it on public-webapps, which was the best first step to make. For the next step, WebKit project now has a policy that new features need to be discussed on webkit-dev.

[Hironori Bono]

Created attachment 95413 [details]
Patch v4 (only for Chromium)

Greetings,

(In reply to comment #13)
> (From update of attachment 95156 [details])
> Attachment 95156 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8747030

Oops, r87483 changed the result type of DocumentMarkerController::markersInRange(). I have updated this change to catch up with it.

Regards,

Hironori Bono

[Hajime Morrita]

> Oops, r87483 changed the result type of DocumentMarkerController::markersInRange(). I have updated this change to catch up with it.
> 
Oops. I'm sorry for the conflict. Thank you for updating the patch for that.

[Brent Fulgham]

Comment on attachment 95413 [details]
Patch v4 (only for Chromium)

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

What a great, complete, patch!  This looks like it would be easy to implement under Windows (and Mac OS X).  Once this initial patch lands I can help get it working under Windows and/or Mac if you would like.

r+, but please correct the couple of minor comments before landing.

> Source/WebCore/dom/DocumentMarker.h:64
> +        // this marker is almost identical as the above Spelling marker, we need

... almost identical TO the above Spelling marker ...

> Source/WebCore/dom/DocumentMarker.h:105
> +    int length() const { return m_endOffset - m_startOffset; }

Is this ever allowed to be negative?  Should this also be unsigned?

> Source/WebCore/dom/DocumentMarkerController.cpp:665
> +        return result.release();

No need for release here.

> Source/WebCore/dom/DocumentMarkerController.cpp:679
> +    return result.release();

Ditto

> Source/WebCore/html/SpellcheckRange.h:31
> +#include "DOMStringList.h"

Can this be a forward declaration?  I can't remember if PassRefPtr needs the full implementation.

[Hironori Bono]

Created attachment 95891 [details]
Patch v5 (only for Chromium)

Greetings Brent,

Thank you for your review and comment.

(In reply to comment #18)
> (From update of attachment 95413 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95413&action=review
> 
> What a great, complete, patch!  This looks like it would be easy to implement under Windows (and Mac OS X).  Once this initial patch lands I can help get it working under Windows and/or Mac if you would like.
> 
> r+, but please correct the couple of minor comments before landing.
> 
> > Source/WebCore/dom/DocumentMarker.h:64
> > +        // this marker is almost identical as the above Spelling marker, we need
> 
> ... almost identical TO the above Spelling marker ...

Done. Thank you for correcting my description.
 
> > Source/WebCore/dom/DocumentMarker.h:105
> > +    int length() const { return m_endOffset - m_startOffset; }
> 
> Is this ever allowed to be negative?  Should this also be unsigned?

Thank you for noticing it. Probably we do not need this variable to be int. I have changed the result type to unsigned and added an ASSERT() here.
 
> > Source/WebCore/dom/DocumentMarkerController.cpp:665
> > +        return result.release();
> 
> No need for release here.

Thank you for correction. I forgot fixing this when Morita-san told it.

> > Source/WebCore/dom/DocumentMarkerController.cpp:679
> > +    return result.release();
> 
> Ditto
> 
> > Source/WebCore/html/SpellcheckRange.h:31
> > +#include "DOMStringList.h"
> 
> Can this be a forward declaration?  I can't remember if PassRefPtr needs the full implementation.

Thank you for noticing it. Unfortunately, I cannot make it a forward declaration because it causes compile errors while compiling an automatically-generated code for v8 as listed below.

distcc[70342] ERROR: compile /Users/hbono/Chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/DerivedSources/Release/webkit/bindings/V8DerivedSources19.cpp on localhost:3632/2 failed
distcc[70342] (dcc_build_somewhere) Warning: remote compilation of '/Users/hbono/Chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/DerivedSources/Release/webkit/bindings/V8DerivedSources19.cpp' failed, retrying locally
distcc[70342] Warning: failed to distribute /Users/hbono/Chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/DerivedSources/Release/webkit/bindings/V8DerivedSources19.cpp to localhost:3632/2, running locally instead
In file included from /Users/hbono/Chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/DerivedSources/Release/webkit/bindings/V8DerivedSources19.cpp:85:
../../JavaScriptCore/wtf/PassRefPtr.h: In function 'void WTF::derefIfNotNull(T*) [with T = WebCore::DOMStringList]':
../../JavaScriptCore/wtf/PassRefPtr.h:74:   instantiated from 'WTF::PassRefPtr<T>::~PassRefPtr() [with T = WebCore::DOMStringList]'
../html/SpellcheckRange.h:45:   instantiated from here
../../JavaScriptCore/wtf/PassRefPtr.h:59: error: invalid use of incomplete type 'struct WebCore::DOMStringList'
../bindings/v8/V8Binding.h:44: error: forward declaration of 'struct WebCore::DOMStringList'
../../JavaScriptCore/wtf/PassRefPtr.h: In function 'void WTF::refIfNotNull(T*) [with T = WebCore::DOMStringList]':
../../JavaScriptCore/wtf/RefPtr.h:44:   instantiated from 'WTF::RefPtr<T>::RefPtr(const WTF::RefPtr<T>&) [with T = WebCore::DOMStringList]'
/Users/hbono/Chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/DerivedSources/Release/webcore/bindings/V8HTMLDivElement.cpp:79:   instantiated from here
../../JavaScriptCore/wtf/PassRefPtr.h:53: error: invalid use of incomplete type 'struct WebCore::DOMStringList'
../bindings/v8/V8Binding.h:44: error: forward declaration of 'struct WebCore::DOMStringList'
distcc[70342] ERROR: compile /Users/hbono/Chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/DerivedSources/Release/webkit/bindings/V8DerivedSources19.cpp on localhost failed

Even though I have read the code generator that creates this binding code, I gave up fixing it because I could not find simple solution for this problem. Sorry.

Regards,

Hironori Bono

[Hajime Morrita]

With Brent's r+, I'll land this patch in a few days if there is no objection.

[Brent Fulgham]

Comment on attachment 95891 [details]
Patch v5 (only for Chromium)

Looks great!  Thanks for addressing my minor suggestions on the last patch.  Please set to cq+ if you would prefer to have the queue land this for you.

[Hajime Morrita]

Comment on attachment 95891 [details]
Patch v5 (only for Chromium)

So cq+ this now. I'll land this manually if the bot complains.

[WebKit Review Bot]

Comment on attachment 95891 [details]
Patch v5 (only for Chromium)

Rejecting attachment 95891 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
(WONTFIX) (8105):
      1 test timed out           ( 0.0%)
    109 text diff mismatch       ( 1.3%)
   7946 skipped                  (98.0%)


Unexpected flakiness: tests timed out (1)
  perf/class-list-remove.html = TEXT TIMEOUT PASS


Regressions: Unexpected text diff mismatch : (4)
  editing/spelling/spellcheck-api-pixel.html = TEXT
  fast/js/property-getters-and-setters.html = TEXT
  fast/loader/stateobjects/pushstate-object-types.html = TEXT
  media/video-display-aspect-ratio.html = TEXT



Full output: http://queues.webkit.org/results/8788407

[WebKit Review Bot]

Created attachment 96390 [details]
Archive of layout-test-results from ec2-cq-02

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hajime Morrita]

> Regressions: Unexpected text diff mismatch : (4)
>   editing/spelling/spellcheck-api-pixel.html = TEXT
>   fast/js/property-getters-and-setters.html = TEXT
>   fast/loader/stateobjects/pushstate-object-types.html = TEXT
>   media/video-display-aspect-ratio.html = TEXT
Okay, for spellcheck-api-pixel.html, I'll remove the expectation and rebaseline it later.
Other test are apparently unrelated. I'll land this and see what happens.

[Hajime Morrita]

Committed r88332: <http://trac.webkit.org/changeset/88332>

[Dimitri Glazkov (Google)]

(In reply to comment #26)
> Committed r88332: <http://trac.webkit.org/changeset/88332>

This change caused a regression, a crashing test. See bug 62460. I am not rolling this out for now, but please fix quickly.

[Simon Fraser (smfr)]

Comment on attachment 95891 [details]
Patch v5 (only for Chromium)

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

> Source/WebCore/html/HTMLDivElement.idl:28
> +        readonly attribute SpellcheckRangeList spellcheckRanges;
> +        void addSpellcheckRange(in unsigned long start, in unsigned long length, in [Optional] DOMStringList suggestions, in [Optional] unsigned short options);
> +        void removeSpellcheckRange(in SpellcheckRange range);

Why does it make any sense to only do this on div elements? What about all the other element types?

Also, if the spec is not finalized, any web-facing API has to be prefixed with webkit.

[Simon Fraser (smfr)]

This new API has some serious issues, noted above, and some questionable RefPtr usage.

[Adam Barth]

(In reply to comment #29)
> This new API has some serious issues, noted above, and some questionable RefPtr usage.

I've rolled out the patch in http://trac.webkit.org/changeset/89452

[Hajime Morrita]

> I've rolled out the patch in http://trac.webkit.org/changeset/89452
Adam, thanks much for your help.

[Hironori Bono]

Created attachment 98888 [details]
Patch v9

Greetings,

Sorry for my last change.
I have re-implemented this API to apply comments. Even though this API is enabled only on Chromium, I have updated xcode project file and a VisualStudio project file to avoid build breaks when enabling this API on these platforms. (FYI, when enabling this API on these platforms, we can also add/remove misspelled underlines on Safari. But we cannot add suggestions because JSC does not seem to have a conversion function from a string array to DOMStringList.) It would be definitely helpful to give me comments.
Thank you for your help in advance.

Regards,

Hironori Bono

[WebKit Review Bot]

Comment on attachment 98888 [details]
Patch v9

Attachment 98888 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8950663

New failing tests:
editing/spelling/spellcheck-api.html

[WebKit Review Bot]

Created attachment 98895 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hironori Bono]

Created attachment 98897 [details]
Patch v10

Greetings,

(In reply to comment #33)
> New failing tests:
> editing/spelling/spellcheck-api.html

Oops, I forgot updating this test to use layoutTestController.hasSpellingMarker() even though I have updated its test result. Sorry.

Regards,

Hironori Bono

[Simon Fraser (smfr)]

Where is the spec for this feature? I can't find it. Your Changelog entry should reference it.

[Adam Barth]

Comment on attachment 98897 [details]
Patch v10

Has this feature had a security review?

[Adam Barth]

Comment on attachment 98897 [details]
Patch v10

Marking this r- for now.  Please feel free to re-nominate once you've answer smfr's and my questions.

[Dimitri Glazkov (Google)]

Comment on attachment 98897 [details]
Patch v10

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

> Source/WebCore/html/SpellcheckRange.idl:30
> +    ] SpellcheckRange {

Shouldn't this be WebKitSpelllcheckRange?

> Source/WebCore/html/SpellcheckRangeList.idl:32
> +    ] SpellcheckRangeList {

WebKitSpellcheckRangeList?

[Hironori Bono]

Created attachment 102119 [details]
Patch v11

Greetings,

Thank you for your comments and sorry for my slow response.

(In reply to comment #36)
> Where is the spec for this feature? I can't find it. Your Changelog entry should reference it.

Thank you for noticing it. Even though we have talked about this API last month, it is not so easy to read though the thread. I have sent an updated spec to summarize the thread <http://lists.w3.org/Archives/Public/public-webapps/2011JulSep/0413.html> and added this URL to ChangeLog.

(In reply to comment #37)
> (From update of attachment 98897 [details])
> Has this feature had a security review?

Thanks to your help, we have finally had a change to talk with them about this API. If I understand the talk with our security people correctly, they do not block this API. (It would be great to correct me if I'm wrong.)

(In reply to comment #39)
> Shouldn't this be WebKitSpelllcheckRange?
> WebKitSpellcheckRangeList?

You are right. I totally forgot renaming them. I have updated this change to rename them.

Regards,

Hironori Bono

[Simon Fraser (smfr)]

> (In reply to comment #36)
> > Where is the spec for this feature? I can't find it. Your Changelog entry should reference it.
> 
> Thank you for noticing it. Even though we have talked about this API last month, it is not so easy to read though the thread. I have sent an updated spec to summarize the thread <http://lists.w3.org/Archives/Public/public-webapps/2011JulSep/0413.html> and added this URL to ChangeLog.

I don't think an email in a mailing list constitutes a sufficiently stable spec. There needs to be at least an W3C editor's draft at a stable URL.

[Dimitri Glazkov (Google)]

(In reply to comment #41)
> > (In reply to comment #36)
> > > Where is the spec for this feature? I can't find it. Your Changelog entry should reference it.
> > 
> > Thank you for noticing it. Even though we have talked about this API last month, it is not so easy to read though the thread. I have sent an updated spec to summarize the thread <http://lists.w3.org/Archives/Public/public-webapps/2011JulSep/0413.html> and added this URL to ChangeLog.
> 
> I don't think an email in a mailing list constitutes a sufficiently stable spec. There needs to be at least an W3C editor's draft at a stable URL.

That seems a little rash. There are plenty of features that went into WebKit without meeting this requirement.

However, I can totally understand a concern about this API being useful enough to ever make it into an HTML spec.

[Alexey Proskuryakov]

This seems like a feature that could particularly benefit from formal W3c process, including (but not limited to) I18 team review.

[Dimitri Glazkov (Google)]

(In reply to comment #43)
> This seems like a feature that could particularly benefit from formal W3c process, including (but not limited to) I18 team review.

Can you explain your reasoning here? What makes it so special?

[Alexey Proskuryakov]

Natural language handling is notoriously hard, and i18n team can have input that you and I may not envision.

> What makes it so special?

I did not claim that the feature is so special. It's not exceedingly trivial, and I agree with Sam that it's fair to ask for it to advance further in standards track.

[Dimitri Glazkov (Google)]

(In reply to comment #45)
> Natural language handling is notoriously hard, and i18n team can have input that you and I may not envision.

At this point, the API is pretty simplistic: it just lets Web Apps to add/remove squigglies to text using ranges. But it does make sense to consider i18 implications. Bono-san, can you provide some insight here? 

> 
> > What makes it so special?
> 
> I did not claim that the feature is so special. It's not exceedingly trivial, and I agree with Sam that it's fair to ask for it to advance further in standards track.

I don't understand why. Consider fullscreen API. It hasn't advanced anywhere in the standards track, has a few unresolved issues (like event handling when in full screen), and yet it's in a shipping browser, enabled by default. Furthermore, it was first announced on whatwg list _after_ it was landed in nightlies.

Why can't Bono-san do something similar? It seems he's done a bit more prior work, like proposed the API on public-webapps and adjusted it according to feedback received.

[Alexey Proskuryakov]

So, here are some questions from the top of my head.

1. As mentioned in the e-mail, the reason for this proposal is that it's difficult to provide custom spell checking in <input> or <textarea>.
    1a. Shouldn't we be providing better building blocks instead of custom APIы, so that the same techniques that are used for content editable would work?
    1b. A site advanced enough to have its own spellchecking will likely build its own inputs anyway, perhaps using a shadow DOM, so is there a place where the API will be used? 
2. Is this use case important enough to introduce an API? It seems to be an exceptionally rare desire to use a spellchecker other than one provided by the OS.
3. The proposal is quite incomplete. 
   3a. hat happens with attached underlines if the DOM is mutated?
   3b. The proposal talks about elements and offsets without much detail. We know that there is a lot of difficulty associated with canonicalizing range endpoints - are these supposed to be somehow canonical? This is important because renderer needs to know if it's a real end of an underline, or it will be connected to an underline below an adjacent element.
   3c. What happens if a user actually chooses a suggestion from a list? I don't see a way for UA to communicate this choice back.
4. Who performed security review? I definitely don't see anything sensitive here myself, but security professionals often find surprising attack vectors, and sometimes they do it during API review.
5. Providing a list of suggestions as a string list may not be forward looking enough. What if I want to pass brief explanations, too?
6. Should it be SpellcheckRange or SpellCheckRange? Or SpellingUnderlineRange?
7. How does grammar checking fit in the API?

As much as most software engineers hate process, there is a place for it. Accepting a feature shouldn't depend on who was reading a mailing list, or who noticed a patch in flight to ask questions like this (and there are certainly more questions that I didn't think of). It also shouldn't depend as much on that random individual's ability to steer a discussion as it does in WebKit Bugzilla.

[Hironori Bono]

Greetings Alexey, et al,

Thank you for your comments.
It seems I moved too fast without deep consideration about shadow DOMs and DOM mutation. (My proposal is always incomplete since I'm an incomplete man.)
To what it's worth, I just would like to have a method that applies a style to a certain range of text in <input> and <textarea> elements. So, I do not have any intention to stick to the incomplete approach of mine. (This API just stems from the fact that we cannot access to the Text nodes (in terms of WebKit) in an <input> element or a <textarea> element as we can do for contenteditable elements.) If a shadow DOM provides scripting access to the Text nodes in these elements, I'm sure it becomes a better solution. (If we can use DOM nodes, we can use DOM ranges to specify text and it solves difficulties about 'start' and 'length' variables.) Nevertheless, I'm not sure if it is a scope of shadow DOMs. (As far as I read through the discussions in the public-webapps ML, I cannot find it.) I would like to start with sending an e-mail to the public-webapps ML about it.

Regards,

Hironori Bono

(In reply to comment #47)
> So, here are some questions from the top of my head.
> 
> 1. As mentioned in the e-mail, the reason for this proposal is that it's difficult to provide custom spell checking in <input> or <textarea>.
>     1a. Shouldn't we be providing better building blocks instead of custom APIы, so that the same techniques that are used for content editable would work?
>     1b. A site advanced enough to have its own spellchecking will likely build its own inputs anyway, perhaps using a shadow DOM, so is there a place where the API will be used? 
> 2. Is this use case important enough to introduce an API? It seems to be an exceptionally rare desire to use a spellchecker other than one provided by the OS.
> 3. The proposal is quite incomplete. 
>    3a. hat happens with attached underlines if the DOM is mutated?
>    3b. The proposal talks about elements and offsets without much detail. We know that there is a lot of difficulty associated with canonicalizing range endpoints - are these supposed to be somehow canonical? This is important because renderer needs to know if it's a real end of an underline, or it will be connected to an underline below an adjacent element.
>    3c. What happens if a user actually chooses a suggestion from a list? I don't see a way for UA to communicate this choice back.
> 4. Who performed security review? I definitely don't see anything sensitive here myself, but security professionals often find surprising attack vectors, and sometimes they do it during API review.
> 5. Providing a list of suggestions as a string list may not be forward looking enough. What if I want to pass brief explanations, too?
> 6. Should it be SpellcheckRange or SpellCheckRange? Or SpellingUnderlineRange?
> 7. How does grammar checking fit in the API?
> 
> As much as most software engineers hate process, there is a place for it. Accepting a feature shouldn't depend on who was reading a mailing list, or who noticed a patch in flight to ask questions like this (and there are certainly more questions that I didn't think of). It also shouldn't depend as much on that random individual's ability to steer a discussion as it does in WebKit Bugzilla.

[Eric Seidel (no email)]

Comment on attachment 95413 [details]
Patch v4 (only for Chromium)

Cleared Brent Fulgham's review+ from obsolete attachment 95413 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 95891 [details]
Patch v5 (only for Chromium)

Cleared Brent Fulgham's review+ from obsolete attachment 95891 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

I think this is a very interesting feature.  My understanding is the motivation is to allow web-based document editing software to provide different spell-checking language support/behavior.  It seems like a reasonable request, given that Word provides its own spell-checker, etc on the Desktop.

It sounds like this work is on hold. Presumably may need more discussion from the standards bodies.  Feel free to place a block on my calendar if you'd like to discuss resuming this work over VC sometime.