1140402 - develop an interface for grammar checkers

Status: UNCONFIRMED

(Core :: Spelling checker, enhancement)

Description

[Daniel Naber]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150224183107

Steps to reproduce:

It would be great to have an interface in Firefox that allows add-on developers to plug
in a grammar checker. Currently, adding grammar checking to Firefox as an add-on
is very difficult: it requires using Javascript and DOM manipulations but the
days of simple textareas are long gone, so any integration is very fragile. The
user comments on existing grammar checker add-ons show that
(https://addons.mozilla.org/en-US/firefox/addon/after-the-deadline-spell-and-g/ and
https://addons.mozilla.org/en-US/firefox/addon/grammarly/). The more robust alternative
is an integration that doesn't really underline errors in the textarea but opens a
pop-up somewhere else, but that's not as user friendly.

From the interface point of view, grammar checking is very similar to spell checking.
These are the major differences:

* A spell checker checks word by word -- a grammar checker needs the complete text
* A spell checker returns true/false whether the word is correct and, optionally, a list of corrections -- a grammar checker returns a list of positions with errors and for each error: start position, end position, error message, optional: list of corrections 

Thus the interface takes the complete text and returns objects with errors it found.
It probably also takes the input language of the text as an argument, as taken from
the HTML 'lang' attributes. This is optional and the grammar checker will need to
deal with this value being absent or even having a wrong value.
As checking might be slow, it should be asynchronous and the grammar checker should return
each error via some kind of call-back as soon as it finds them.

Note that the grammar checker should get the complete text at once, as it might want
to check for errors that are not on the sentence or paragraph level.

The grammar checker might include spell checking or not, so it should be able to
indicate that and traditional spell checking should be turned off if the grammar
checker includes it.

From the user interface, the results of grammar checking look a lot spell checking:
incorrect text gets underlined, but maybe in a different color than the red used for
spell checking. Also, not only single words get underlined but longer phrases.

With a spell checker you can add words to your dictionary and make the error message
disappear. This is a bit more complicated for grammar checking, as the error is
not about single words being wrong, but about words in their context. Some kind of "ignore"
option will be needed to turn off false alarms. I'll leave this out of this issue for now.

Comment 1

[Jos Collin]

Plan for adding interfaces for Grammar Checker Extensions in Firefox.
---------------------------------------------------------------------
We've tried to come up with a plan about how this feature could be implemented. We would like to have feedback on the plan to see if it makes sense and whether there's a good chance of getting a patch accepted into Firefox. If there's positive feedback, we're going to start working on a patch.



Overview
--------
The Javascript-based extension will implement the grammar checking task. The checking logic can work either locally or remotely. The extension gets the text by listening to key events from the web page's text fields (<textarea>s). Alternatively, it can wait until the user clicks a “check text” button provided by the Extension. After the text has been checked, the extension calls the interface with the list of errors it has found. The implementation of the interface then marks the errors in the textarea, by default using a blue underline (similar to the spell checker).



Details
-------
Firefox shall have the new interfaces and the related classes for doing the following tasks:

1. Get the list of errors from the extension and underline the errors with a blue line.
2. Get the list optional corrections for each error from the extension and add it to the context menu when the user right clicks on a particular error.



The following are the list of new classes and interfaces required:

1. GrammarCheckError
This class defines the error for the Grammar Checker in the Firefox. An object of this class is considered as an error returned by the Grammar Checker Extension:

* string errortext: The error text message returned by the Extension. This needs to be short enough to be shown in the context menu.

* Textarea: a reference to the textarea. From the Extension point of view this is the source of the event, i.e. 'event.target'.

* int startposition, int endposition: The start and end position (in characters) of the error returned by the Extension.

* vector<string> corrections: the optional list of corrections returned by the Extension.

* void markError(): finds the appropriate text range using the startposition and endposition and  underlines the text in blue color.



2. GrammarCheckErrorList
This class contains the list of objects of type GrammarCheckError and the functions that operate on the list of objects.

* vector<GrammarCheckError> errorlist: The vector of error objects. This contains the entire set of errors returned by the Extension after a Grammar Check is performed.

* vector<string> findCorrections(int characterposition): Iterates through the errorlist and returns the optional corrections based on the current character position, when the user right clicks on an underlined error. For example, if the text contains the error “a hour” and the start and end position underline this as an error, this would return a list with the element “an hour”.

* void markErrors(): Iterates through the vector of errorlist and calls markError() on each GrammarCheckError object.



3. nsIEditorGrammarCheck
This is the interface for the Grammar Checker Extensions in the Firefox. This interface class declares and exposes a function that the Grammar Checker Extensions can call:

* void markErrors(GrammarCheckErrorListObj) 
This interface function takes in the detected errors as a parameter. Does it make sense to get the errors a JSON string?



4. nsEditorGrammarCheck
This class implements the interface functions for the Grammar Checker Extension.

* NS_IMETHOD markErrors(GrammarCheckErrorList* errorlistobj): Gets the errorlist object and calls the markErrors() function in the class GrammarCheckErrorList. This function is called by the Extension via the interface.

* void addCorrectionsToContextMenu(vector<string> vCorrections): Adds the optional corrections to the context menu. vCorrections  is the vector that is returned by  findCorrections() function. This function is called when the user right clicks on an underlined error.

Comment 2

[Olivier R.]

Sometimes it’s difficult to explain what is wrong or why it is wrong in a small contextual menu entry. So it would be useful to have a property in GrammarCheckError called “FullCommentURL”. If this property is not empty, the contextual menu offers another entry called “More informations…” which would open a new tab towards the website where there is a full explanatory. We have this option in LibreOffice/OpenOffice.
(Another option: add the property FullExplanation in GrammarCheckError which would trigger a popover window where we would have more space than in a single line.)

There is different kinds of errors. In GrammarCheckerError, it would nice to have a property called “UnderlineColor”, where we could set another color than the default one.
Example: https://en-support.files.wordpress.com/2009/08/spelling11.png

In LibreOffice/OpenOffice, the contextual menu also offers the entries “Ignore” to remove the error and “Ignore All” to remove all errors of the same kind. The last option can be done only because each grammar rule have an identifier. And this identifier is set in each error in the property “RuleIdentifier”.

In LibreOffice/OpenOffice, texts are checked paragraph by paragraph. When the user enters a new paragraph, the previous one is checked. It seems that the grammar checker checks all paragraphs where the cursor is not, and if everything else is checked, it checks the paragraph where the cursor is when the user don’t type.

I may have more comments later on your API.

Comment 3

[Daniel Naber]

Ehsan, as you commented already on the related issue 395327, could you maybe comment here and/or set this issue to confirmed?

Comment 4

[(no longer active)]

I'm a bit busy these days, but I'll try to get to this as soon as I can.  Please stay tuned!

Comment 5

[Jos Collin]

Hello,

I have the following questions with the Plan that I have submitted above:

1. As per the Current Plan, markErrors(GrammarCheckErrorListObj) interface function in nsIEditorGrammarCheck is supposed to take in the list of errors as a parameter. We cannot pass a C++ object (containing the list of errors) or an STL vector from the Extension to the markErrors() interface. So Does it make sense to get the errors a JSON string?  Thus the GrammarCheckErrorList object or the Vector should be created with in the C++ code by parsing the JSON string data. So the data will arrive from the Extension side and the C++ will add functions to operate on that data, which forms the GrammarCheckErrorList object. Please suggest.

2. If the errors can be passed in as a JSON string, then Is there any way to convert the JSON string to say C++ vectors ? Is there any such function already exist in the Firefox code ? 

3. If the errors can be passed in as a JSON string, then Is there a JSON String Parser already exist in the Firefox code that we could use ?


4. Regarding the Reference to the TextArea (event.target), how the Textarea object or reference can be passed to the C++ code ? What would be the type of that object ? Is it possible to do as in nsIEditorSpellCheck::InitSpellChecker ? In InitSpellChecker, the Textarea is passed in as a parameter: 'in nsIEditor editor'. Please suggest.

5. In nsEditorSpellCheck.cpp, I couldn't find an underlinered() function, which underline the errors found by the Spell Checker. I wonder how this is implemented for the spell checker. Is it a feature provided by the libeditor (editor/libeditor/) ?


Thanks,
Jos Collin

Comment 6

[commercials24]

this is needed badly, https://www.languagetool.org/de/ does such a  great job at spell checking for my language but the addon for thunderbird for example isnt working properly, besides being installed by 2000 people.

support for this would also be great in firefox.

Comment 7

[lcutugno]

Attachment: patch.diff

Comment 8

[Sebastian Zartner [:sebo]]

Comment on attachment 8666428 [details] [diff] [review]
patch.diff

I guess you wanted to request feedback. To do so you need to set the flags to '?' and add someone specific for review.
I did that now requesting review by Olli Pettay. Olli, if you're not the right person to ask, can you please set the right person?

Sebastian

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Ok, I'll try to go through the patch soon, within couple of days.

In the mean time, feel free to update it to follow
Mozilla coding style https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Comment 10

[(no longer active)]

Thanks everyone, and sorry I couldn't get back to you guys sooner.  I'm happy that you're making progress and can't wait to see this in Firefox!  :-)

Comment 11

[lcutugno]

Just thought I'd add some details about the patch and what does it do:
We added a simple API for grammar checking add-ons:

* registerAddon(callback): this is used to register the callback function from the add-on that does the grammar checking logic, it's called every time the text is changed or focus is changed

* errorsFound(array errorsStart, array errorsEnd, int count): this needs to be called when the add-on finds grammar errors in the text, it needs to supply 2 arrays, the first one with the starting positions of the error ranges (i.e. 0 and 4), and the second with the end positions (i.e. 2 and 9), and finally the number of errors found (i.e. 2)

* addSuggestionForError(int error, string suggestion, string description, [optional] bool messageOnly): adds a suggestion text that is shown in the context menu and when selected replaces the underlined text, the description parameter is used for the tooltip.

A sample add-on using this API is available at https://github.com/lenny93/ns-grammarcheck-plugin

At the moment this functionality works on <input> and <textarea> elements only.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

[e-mail] any chance you could update the patch to follow Mozilla's coding style.
That would let the reviewer to focus on actual functionality and not the style ;)


(no tabs, only spaces, aFoo naming for arguments, 2 spaces for indentation, always {} with 'if',  { to same line as 'if' etc.)
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Comment 13

[lcutugno]

Attachment: patch.diff

Updated to use Mozilla's coding style.

Comment 14

[lcutugno]

Attachment: patch.diff

Updated to use Mozilla's coding standards (2)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I'll try to get to this tomorrow. sorry about the delay.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8675224 [details] [diff] [review]
patch.diff

Sorry about the delay!

This is a massive patch and will require couple of iterations.

>@@ -732,6 +800,7 @@ nsContextMenu.prototype = {
>           else {
>             InlineSpellCheckerUI.init(this.target.QueryInterface(Ci.nsIDOMNSEditableElement).editor);
>             InlineSpellCheckerUI.initFromEvent(aRangeParent, aRangeOffset);
>+     this.initGrammarItems(this.target.QueryInterface(Ci.nsIDOMNSEditableElement).editor, aRangeParent, aRangeOffset);
fix indentation

>@@ -864,6 +933,7 @@ nsContextMenu.prototype = {
>                                         .getInterface(Ci.nsIEditingSession);
>           InlineSpellCheckerUI.init(editingSession.getEditorForWindow(targetWin));
>           InlineSpellCheckerUI.initFromEvent(aRangeParent, aRangeOffset);
>+     this.initGrammarItems(editingSession.getEditorForWindow(targetWin), aRangeParent, aRangeOffset);
fix indentation


>@@ -7308,6 +7309,11 @@ HTMLInputElement::OnValueChanged(bool aNotify)
>   if (HasDirAuto()) {
>     SetDirectionIfAuto(true, aNotify);
>   }
>+  
>+  if (nsEditorGrammarCheck::GetGrammarCheckService()->mEditor == this->GetEditor()) {
>+   nsEditorGrammarCheck::GetGrammarCheckService()->DoGrammarCheck();
>+  }
...
>@@ -1536,6 +1537,11 @@ HTMLTextAreaElement::OnValueChanged(bool aNotify)
>   if (validBefore != IsValid()) {
>     UpdateState(aNotify);
>   }
>+  
>+  if (nsEditorGrammarCheck::GetGrammarCheckService()->mEditor == this->GetEditor()) {
>+   nsEditorGrammarCheck::GetGrammarCheckService()->DoGrammarCheck();
>+  }
Why do we need this code? Couldn't we follow the same pattern as what spellchecker does?


>+++ b/editor/composer/nsEditorGrammarCheck.cpp
>@@ -0,0 +1,542 @@
>+#include <iostream>                 // for getenv
>+#include <string.h>                     // for nullptr, strcmp
>+#include "mozilla/Attributes.h"         // for final
>+#include "mozilla/Preferences.h"        // for Preferences
>+#include "mozilla/Services.h"           // for GetXULChromeRegistryService
>+#include "mozilla/dom/Element.h"        // for Element
>+#include "mozilla/dom/Selection.h"
>+#include "mozilla/mozalloc.h"           // for operator delete, etc
>+#include "mozilla/dom/Selection.h"
>+#include "mozilla/ModuleUtils.h"
>+#include "nsIEditor.h"                  // for nsIEditor
>+#include "nsMemory.h"
>+#include "nsIClassInfoImpl.h"
>+#include "nsEditorGrammarCheck.h"
>+#include "nsString.h"
>+#include "nsITransferable.h"
Why you need nsITransferable.h?

>+#include "nsXPCOMCIDInternal.h"
>+#include "nsIServiceManager.h"
>+#include "nsIDocumentEncoder.h"
>+#include "nsIHTMLDocument.h"
>+#include "nsContentCID.h"
>+#include "nsGkAtoms.h"
>+#include "../../modules/libpref/Preferences.h"
Why you need libpref/Preferences.h when you always include mozilla/Preferences.h ?



>+nsEditorGrammarCheck::nsEditorGrammarCheck() : gCallback(nullptr), mEditor(), mCheckEnabled(true)
>+{
>+ NS_ASSERTION(!gGrammarCheckService,
>+        "Attempting to create two instances of the service!");
>+ gGrammarCheckService = this;
2 spaces for indentation



>+nsEditorGrammarCheck::~nsEditorGrammarCheck() 
>+{
>+ NS_ASSERTION(gGrammarCheckService == this,
>+        "Deleting a non-singleton instance of the service");
indentation

>+ if (gGrammarCheckService == this)
>+   gGrammarCheckService = nullptr;
>+
>+}


>+NS_IMETHODIMP
>+nsEditorGrammarCheck::Init()
>+{
>+ mCheckEnabled = Preferences::GetBool("grammarCheckEnabled", true);
You should default to false.

>+ Preferences::SetBool("grammarCheckEnabled", mCheckEnabled);
I don't understand why you first get the pref, and then immediately set it to the same value.

Also, I would use Preferences::AddBoolVarCache to get the pref value.
That way no need to call GetBool all the time, but just check some static variable.
Something like http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCaret.cpp?rev=41dea9df27ed#156



>+   if (aEndOffset)
>+     range->SetEnd(aEndNode, aEndOffset);
>+   else
>+     range->SetEndAfter(aEndNode);
if (foo) {
  ...
} else {
  ...
}



>+NS_IMETHODIMP
>+nsEditorGrammarCheck::IsSuggestionMessageOnly(uint32_t aOffset, uint32_t suggestion, bool* _retval)
aSuggestion, aRetVal


>+NS_IMETHODIMP
>+nsEditorGrammarCheck::GetSuggestionsForOffset(nsIEditor* editor, uint32_t aOffset, nsIArray** _retval)
aEditor, aRetVal


>+NS_IMETHODIMP
>+nsEditorGrammarCheck::GetDescriptionsForOffset(nsIEditor* editor, uint32_t aOffset, nsIArray** _retval)
aEditor, aRetVal

+
>+
>+NS_IMETHODIMP
>+nsEditorGrammarCheck::ToggleEnabled()
>+{
>+ mCheckEnabled = !mCheckEnabled;
>+ Preferences::SetBool("grammarCheckEnabled", mCheckEnabled);
>+
>+ if (!mEditor)
>+   return NS_OK;
always {} with if

>+
>+   nsCOMPtr<nsISelectionController> selcon;
>+   rv = mEditor->GetSelectionController(getter_AddRefs(selcon));
declare rv where it is first time used.
So 
nsresult rv = mEditor->GetSelectionController(getter_AddRefs(selcon));



>+NS_IMETHODIMP
>+nsEditorGrammarCheck::IsGrammarCheckEnabled(bool* _retval)
>+{
>+ *_retval = mCheckEnabled;
s/_retval/aRetVal/



>+NS_IMETHODIMP
>+nsEditorGrammarCheck::DoGrammarCheck()
>+{
>+ if (mEditor == nullptr || !mCheckEnabled)
>+   return NS_OK;
always {} with if


>+static const mozilla::Module kSampleModule = {
>+    mozilla::Module::kVersion,
>+    kSampleCIDs,
>+    kSampleContracts,
>+    kSampleCategories
>+};
2 spaces for indentation. (there is still some old code which isn't following the coding style, but new code should).
Same issue also elsewhere.


>+// The following line implements the one-and-only "NSGetModule" symbol
>+// for compatibility with mozilla 1.9.2. You should only use this
>+// if you need a binary which is backwards-compatible and if you use
>+// interfaces carefully across multiple versions.
>+//NS_IMPL_MOZILLA192_NSGETMODULE(&kSampleModule)
Copy-pasted from somewhere? Gecko 1.9.2 is ancient and should not be supported just just drop this




>+++ b/editor/composer/nsEditorGrammarCheck.h
>@@ -0,0 +1,116 @@
>+#ifndef __GRAMMARCHECK_IMPL_H__
>+#define __GRAMMARCHECK_IMPL_H__

Should be just GrammarCheck_h



>+#define GC_FACTORY_SINGLETON_IMPLEMENTATION(_className, _sInstance)        \
>+_className * _className::_sInstance = nullptr;                                \
>+                                      \
>+already_AddRefed<_className>                                                 \
>+_className::GetSingleton()                                                   \
>+{                                                                            \
>+if (_sInstance) {                                                          \
>+  nsRefPtr<_className> ret = _sInstance;                                   \
>+  return ret.forget();                                                     \
>+}                                                                          \
>+_sInstance = new _className();                                             \
>+nsRefPtr<_className> ret = _sInstance;                                     \
>+if (NS_FAILED(_sInstance->Init())) {                                       \
>+  /* Null out ret before _sInstance so the destructor doesn't assert */    \
>+  ret = nullptr;                                                           \
>+  _sInstance = nullptr;                                                    \
>+  return nullptr;                                                          \
>+}                                                                          \
>+return ret.forget();                                                       \
>+}
Why do we need this macro which is used only once.


>+ struct GRAMMARERROR
>+ {
>+   int errorStart;
>+   int errorEnd;
>+   std::vector<nsString> suggestions;
>+   std::vector<nsString> descriptions;
>+   std::vector<bool> messageOnly;
>+ };
GRAMMARERROR is odd struct name.
should be GrammarError
And please use nsTArray and not std::vector, and 
either uint32_t or int32_t for errorStart and errorEnd.
And member variables should have m-prefix.
So, mErrorStart etc.



>+ static nsEditorGrammarCheck* GetGrammarCheckService()
>+ {
>+   if (!gGrammarCheckService) {
>+     nsCOMPtr<nsIEditorGrammarCheck> serv = do_GetService(NS_GRAMMARCHECK_CONTRACTID);
>+     NS_ENSURE_TRUE(serv, nullptr);
>+     NS_ASSERTION(gGrammarCheckService, "Should have static instance pointer now");
>+   }
>+   return gGrammarCheckService;
I don't think we should initiate the service if grammarCheckEnabled is false


>+ std::vector<GRAMMARERROR> mErrors;
Please use nsTArray. 



>+[scriptable, uuid(8ac26150-586f-4b70-90df-07610b80b45f)]
make this builtinclass too.




>@@ -269,6 +269,7 @@ GetIndexFromSelectionType(SelectionType aType)
>     case nsISelectionController::SELECTION_ACCESSIBILITY: return 6; break;
>     case nsISelectionController::SELECTION_FIND: return 7; break;
>     case nsISelectionController::SELECTION_URLSECONDARY: return 8; break;
>+ case nsISelectionController::SELECTION_GRAMMARCHECK: return 9; break;
fix indentation


>@@ -290,6 +291,7 @@ GetSelectionTypeFromIndex(int8_t aIndex)
>     case 6: return nsISelectionController::SELECTION_ACCESSIBILITY; break;
>     case 7: return nsISelectionController::SELECTION_FIND; break;
>     case 8: return nsISelectionController::SELECTION_URLSECONDARY; break;
>+ case 9: return nsISelectionController::SELECTION_GRAMMARCHECK; break;
fix indentation


>@@ -4739,7 +4741,7 @@ Selection::RemoveRange(nsRange& aRange, ErrorResult& aRv)
>     // into view. The spell-check selection, however, is created and destroyed
>     // in the background. We don't want to scroll in this case or the view
>     // might appear to be moving randomly (bug 337871).
>-    if (mType != nsISelectionController::SELECTION_SPELLCHECK && cnt > 0)
>+    if ((mType != nsISelectionController::SELECTION_SPELLCHECK && cnt > 0) || (mType != nsISelectionController::SELECTION_GRAMMARCHECK && cnt > 0))
overlong line. Please break at ||


>     eIndexSelRawText,
>     eIndexConvText,
>     eIndexSelConvText,
>-    eIndexSpellChecker
>+    eIndexSpellChecker,
>+ eIndexGrammarChecker
fix indentation


>@@ -349,6 +350,8 @@ public:
>         return eIndexSelConvText;
>       case nsISelectionController::SELECTION_SPELLCHECK:
>         return eIndexSpellChecker;
>+   case nsISelectionController::SELECTION_GRAMMARCHECK:
>+     return eIndexGrammarChecker;
fix indentation

@@ -3807,13 +3810,18 @@ static StyleIDs SelectionStyleIDs[] = {
>     LookAndFeel::eColorID_LAST_COLOR,
>     LookAndFeel::eColorID_SpellCheckerUnderline,
>     LookAndFeel::eIntID_SpellCheckerUnderlineStyle,
>-    LookAndFeel::eFloatID_SpellCheckerUnderlineRelativeSize }
>+    LookAndFeel::eFloatID_SpellCheckerUnderlineRelativeSize },
>+ { LookAndFeel::eColorID_LAST_COLOR,
>+ LookAndFeel::eColorID_LAST_COLOR,
>+ LookAndFeel::eColorID_GrammarCheckerUnderline,
>+ LookAndFeel::eIntID_SpellCheckerUnderlineStyle,
>+ LookAndFeel::eFloatID_GrammarCheckerUnderlineRelativeSize }
fix indentation.




>@@ -5186,6 +5195,7 @@ ComputeSelectionUnderlineHeight(nsPresContext* aPresContext,
>     case nsISelectionController::SELECTION_IME_CONVERTEDTEXT:
>     case nsISelectionController::SELECTION_IME_SELECTEDCONVERTEDTEXT:
>       return aFontMetrics.underlineSize;
>+ case nsISelectionController::SELECTION_GRAMMARCHECK:
fix indentation

>@@ -5327,6 +5337,7 @@ static void DrawSelectionDecorations(gfxContext* aContext,
>       }
>       break;
>     }
>+ case nsISelectionController::SELECTION_GRAMMARCHECK:
fix indentation


>@@ -6597,7 +6608,8 @@ nsTextFrame::CombineSelectionUnderlineRect(nsPresContext* aPresContext,
>     float relativeSize;
>     int32_t index =
>       nsTextPaintStyle::GetUnderlineStyleIndexForSelectionType(sd->mType);
>-    if (sd->mType == nsISelectionController::SELECTION_SPELLCHECK) {
>+    if (sd->mType == nsISelectionController::SELECTION_SPELLCHECK ||
>+   sd->mType == nsISelectionController::SELECTION_GRAMMARCHECK) {
align sd->mType to be under the sd->mType in the previous line



>+++ b/widget/LookAndFeel.h
>@@ -59,6 +59,7 @@ public:
>     eColorID_IMESelectedConvertedTextUnderline,
> 
>     eColorID_SpellCheckerUnderline,
>+ eColorID_GrammarCheckerUnderline,
> 
>     // New CSS 2 color definitions
>     eColorID_activeborder,
>@@ -464,7 +465,8 @@ public:
> 
>     // The width/height ratio of the cursor. If used, the CaretWidth int metric
>     // should be added to the calculated caret width.
>-    eFloatID_CaretAspectRatio
>+ eFloatID_CaretAspectRatio,
>+ eFloatID_GrammarCheckerUnderlineRelativeSize
Please align the new values based on the old code


>@@ -177,6 +177,9 @@ nsLookAndFeel::NativeGetColor(ColorID aID, nscolor& aColor)
>     case eColorID_SpellCheckerUnderline:
>       aColor = NS_RGB(0xff, 0, 0);
>       break;
>+    case eColorID_GrammarCheckerUnderline:
>+   aColor = NS_RGB(0, 0, 0xff);
odd indentation here.


>@@ -688,7 +691,10 @@ nsLookAndFeel::GetFloatImpl(FloatID aID, float &aResult)
>         break;
>     case eFloatID_SpellCheckerUnderlineRelativeSize:
>         aResult = 1.0f;
>-        break;
>+   break;
>+ case eFloatID_GrammarCheckerUnderlineRelativeSize:
>+   aResult = 1.0f;
>+   break;
odd indentation here. See the code around


>     case eFloatID_CaretAspectRatio:
>         aResult = sCaretRatio;
>         break;
>diff --git a/widget/windows/nsLookAndFeel.cpp b/widget/windows/nsLookAndFeel.cpp
>index 50e3a8b..a3b3359 100644
>--- a/widget/windows/nsLookAndFeel.cpp
>+++ b/widget/windows/nsLookAndFeel.cpp
>@@ -153,6 +153,9 @@ nsLookAndFeel::NativeGetColor(ColorID aID, nscolor &aColor)
>     case eColorID_SpellCheckerUnderline:
>         aColor = NS_RGB(0xff, 0, 0);
>         return NS_OK;
>+    case eColorID_GrammarCheckerUnderline:
>+   aColor = NS_RGB(0, 0, 0xff);

odd indentation here.

Comment 17

[lcutugno]

Attachment: new.diff

Comment 18

[lcutugno]

>Why do we need this code? Couldn't we follow the same pattern as what spellchecker does?
What I'm doing is grammar checking on focused editors only. I thought this could be better for performance reasons, but I guess it could be done the other way too.
Other than this I have updated the patch with your proposed changes.

Comment 19

[Daniel Naber]

[e-mail] any change you could review the updated patch?

Comment 20

[Daniel Naber]

Any update to get this reviewed?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

bear with me. I'm so sorry about the delay here. (tons of other stuff to do, bad excuse).
I'll try to prioritize this.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8687129 [details] [diff] [review]
new.diff

>diff --git a/browser/base/content/browser-context.inc b/browser/base/content/browser-context.inc
>index f9fd961..40c5c87 100644
>--- a/browser/base/content/browser-context.inc
>+++ b/browser/base/content/browser-context.inc
>@@ -38,6 +38,7 @@
>       </menugroup>
>       <menuseparator id="context-sep-navigation"/>
>       <menuseparator id="page-menu-separator"/>
>+      <menuseparator id="grammar-suggestion-separator"/>
>       <menuitem id="spell-no-suggestions"
>                 disabled="true"
>                 label="&spellNoSuggestions.label;"/>
>@@ -393,6 +394,11 @@
>                 type="checkbox"
>                 accesskey="&spellCheckToggle.accesskey;"
>                 oncommand="InlineSpellCheckerUI.toggleEnabled(window);"/>
>+      <menuitem id="grammar-check-enabled"
>+                label="&grammarCheckToggle.label;"
>+                type="checkbox"
>+                accesskey="&grammarCheckToggle.accesskey;"
>+                oncommand="gContextMenu.grammarCheckToggle();"/>
>       <menuitem id="spell-add-dictionaries-main"
>                 label="&spellAddDictionaries.label;"
>                 accesskey="&spellAddDictionaries.accesskey;"
Why do we need this stuff if FF will ship by default without any grammar checking. Addons should be able to add new menu items if needed.

>+++ b/browser/base/content/nsContextMenu.js
>@@ -14,6 +14,10 @@ XPCOMUtils.defineLazyModuleGetter(this, "Pocket",
>   "resource:///modules/Pocket.jsm");
> 
> var gContextMenuContentData = null;
>+var gcSvc = Components.classes["@mozilla.org/grammarcheck;1"].
>+  getService(Components.interfaces.nsIEditorGrammarCheck);
>+var rangeOffset;
>+var mGrammarSuggestions = [];
> 
> function nsContextMenu(aXulMenu, aIsShift) {
>   this.shouldDisplay = true;
>@@ -430,9 +434,12 @@ nsContextMenu.prototype = {
>     var onMisspelling = InlineSpellCheckerUI.overMisspelling;
>     var showUndo = canSpell && InlineSpellCheckerUI.canUndo();
>     this.showItem("spell-check-enabled", canSpell);
>+    this.showItem("grammar-check-enabled", true);
>     this.showItem("spell-separator", canSpell || this.onEditableArea);
>     document.getElementById("spell-check-enabled")
>             .setAttribute("checked", canSpell && InlineSpellCheckerUI.enabled);
>+    document.getElementById("grammar-check-enabled")
>+            .setAttribute("checked", gcSvc.isGrammarCheckEnabled());
> 
>     this.showItem("spell-add-to-dictionary", onMisspelling);
>     this.showItem("spell-undo-add-to-dictionary", showUndo);
>@@ -468,6 +475,58 @@ nsContextMenu.prototype = {
>     else
>       this.showItem("spell-add-dictionaries-main", false);
>   },
>+  
>+  grammarCheckToggle: function() {
>+    gcSvc.toggleEnabled();
>+  },
>+  
>+  doGrammarCorrection: function(index) {
>+    gcSvc.doGrammarCorrection(this.mWordOffset, index);
>+  },
>+  
>+  initGrammarItems: function(aEditor, rangeParent, rangeOffset) {
>+    var gramSuggestions = gcSvc.getSuggestionsForOffset(aEditor, rangeOffset);
>+    var gramDescriptions = gcSvc.getDescriptionsForOffset(aEditor, rangeOffset);
>+    this.mWordOffset = rangeOffset;
>+    var separator = document.getElementById("grammar-suggestion-separator");
>+    var itemNoSuggestion = document.getElementById("grammar-no-suggestions");
>+    var menu = separator.parentNode;
>+    
>+    var index;
>+    for(index = 0; index < mGrammarSuggestions.length; index++) {
>+      menu.removeChild(mGrammarSuggestions[index]);
>+    }
>+    
>+    mGrammarSuggestions = [];
>+    
>+    for(index = 0; index < gramSuggestions.length; index++) {
>+      var gs = gramSuggestions.queryElementAt(index, Components.interfaces.nsISupportsString);
>+      var gd = gramDescriptions.queryElementAt(index, Components.interfaces.nsISupportsString);
>+      var messageOnly = gcSvc.isSuggestionMessageOnly(rangeOffset, index);
>+      
>+      var item = menu.ownerDocument.createElement("menuitem");
>+      item.setAttribute("label", gs.data);
>+      item.setAttribute("value", gs.data);
>+      item.setAttribute("tooltip", gd.data);
>+      item.setAttribute("tooltiptext", gd.data);
>+      item.setAttribute("class", "spell-suggestion");
>+      
>+      if(messageOnly) {
>+        item.setAttribute("disabled", true);
>+      } else {
>+        var callback = function(me, val) { return function(evt) { me.doGrammarCorrection(val); } };
>+        item.addEventListener("command", callback(this, index), true);
>+      }
>+      menu.insertBefore(item, separator);
>+      mGrammarSuggestions.push(item);
>+    }
>+    
>+    if(gramSuggestions.length > 0) {
>+      this.showItem("grammar-suggestion-separator", true);
>+    } else {
>+      this.showItem("grammar-suggestion-separator", false); 
>+    }
>+  },
Same here. This all belongs to some addon, at least until someone decides that FF should include grammar checker built-in.


> NS_IMPL_NS_NEW_HTML_ELEMENT_CHECK_PARSER(Input)
> 
>@@ -7308,6 +7309,10 @@ HTMLInputElement::OnValueChanged(bool aNotify)
>   if (HasDirAuto()) {
>     SetDirectionIfAuto(true, aNotify);
>   }
>+  
>+  if (nsEditorGrammarCheck::GetGrammarCheckService()->mEditor == this->GetEditor()) {
>+    nsEditorGrammarCheck::GetGrammarCheckService()->DoGrammarCheck();
>+  }
We should not try to access grammar service if there isn't any grammar checker installed.
So nsEditorGrammarCheck::GetGrammarCheckService() should be something which returns non-null only if we have a grammar checker.
and then this code should do a null check. And mEditor should be hidden behind some getter method.

>+  if (nsEditorGrammarCheck::GetGrammarCheckService()->mEditor == this->GetEditor()) {
>+    nsEditorGrammarCheck::GetGrammarCheckService()->DoGrammarCheck();
>+  }
ditto

>+++ b/editor/composer/nsEditorGrammarCheck.cpp
>@@ -0,0 +1,526 @@
>+#include <iostream>                     // for getenv
>+#include <string.h>                     // for nullptr, strcmp
>+#include "mozilla/Attributes.h"         // for final
>+#include "mozilla/Preferences.h"        // for Preferences
>+#include "mozilla/Services.h"           // for GetXULChromeRegistryService
>+#include "mozilla/dom/Element.h"        // for Element
>+#include "mozilla/dom/Selection.h"
>+#include "mozilla/mozalloc.h"           // for operator delete, etc
>+#include "mozilla/dom/Selection.h"
>+#include "mozilla/ModuleUtils.h"
>+#include "nsIEditor.h"                  // for nsIEditor
>+#include "nsMemory.h"
>+#include "nsIClassInfoImpl.h"
>+#include "nsEditorGrammarCheck.h"
>+#include "nsString.h"
>+#include "nsXPCOMCIDInternal.h"
>+#include "nsIServiceManager.h"
>+#include "nsIDocumentEncoder.h"
>+#include "nsIHTMLDocument.h"
>+#include "nsContentCID.h"
>+#include "nsGkAtoms.h"
I'm pretty sure you have extra #includes here.



>+NS_IMPL_CLASSINFO(nsEditorGrammarCheck, nullptr, 0, NS_GRAMMARCHECK_CID)
>+NS_IMPL_ISUPPORTS_CI(nsEditorGrammarCheck, nsIEditorGrammarCheck)
Why do we need classinfo for nsEditorGrammarCheck?

>+nsEditorGrammarCheck * nsEditorGrammarCheck::gGrammarCheckService = nullptr;
nit, extra space before *

>+already_AddRefed<nsEditorGrammarCheck>
>+nsEditorGrammarCheck::GetSingleton()
I don't understand how this is different to GetGrammarCheckService(). One is returning already_AddRefed and one raw pointer sure, 
but how they are functionally different?
Only one of them, and it should return a service only if there is some real grammar checker installed.


>+NS_IMETHODIMP nsEditorGrammarCheck::Init()
>+{
>+  mCheckEnabled = Preferences::GetBool("grammarCheckEnabled", false);
>+  Preferences::SetBool("grammarCheckEnabled", mCheckEnabled);
Like, you probably want to move this to GetSingleton() or GetGrammarCheckService() (which ever there will be)
and use Preferences::AddBoolVarCache to there.
See for example http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#790 how it is used.

>+NS_IMETHODIMP nsEditorGrammarCheck::RegisterAddon(nsIEditorGrammarCheckCallback* callback)
aCallback
Same also elsewhere. Arguments should be in form aArgumentName

>+  mCheckEnabled = Preferences::GetBool("grammarCheckEnabled", true);
so this should be needed elsewhere, if Preferences::AddBoolVarCache was used elsewhere and the value was cached in some static variable

>+  nsWeakPtr nwp = do_GetWeakReference(aEditor);
>+
>+  mEditor = do_QueryReferent(nwp);
What is this code trying to do? Why to go from nsIEditor to nsWeakPtr and back to nsIEditor? 

>+++ b/editor/composer/nsEditorGrammarCheck.h
>@@ -0,0 +1,94 @@
>+#ifndef GrammarCheck_h
>+#define GrammarCheck_h
>+
>+#include "nsCOMPtr.h"                   // for nsCOMPtr
>+#include "nsCycleCollectionParticipant.h"
>+#include "nsIEditorGrammarCheck.h"
>+#include "nsISupportsImpl.h"
>+#include "nsString.h"                   // for nsString
>+#include "nsTArray.h"                   // for nsTArray
>+#include "nscore.h"    
>+#include "../../extensions/spellcheck/src/mozInlineSpellWordUtil.h"
Looks like you want to modify moz.build to include some directory to be considered in includes.

>+
>+  nsCOMPtr<nsIEditorGrammarCheckCallback> gCallback;
This is not a global variable, but a normal member variable so it should have m -prefix

>+[scriptable, builtinclass, uuid(8ac26150-586f-4b70-90df-07610b80b45f)]
>+interface nsIEditorGrammarCheck : nsISupports
>+{
>+  void registerAddon(in nsIEditorGrammarCheckCallback callback);
Should this be called registerGrammarChecker? Or even just setGrammarChecker since the code supports only one. checker.

>+  
>+  nsEditorGrammarCheck::GetGrammarCheckService()->SetCurrentEditor(this);
Again, this should have a null check since we shouldn't initiate GrammarCheckService all the time, only if we have
something to actually do the check and the pref is enabled.

>+++ b/toolkit/locales/en-US/chrome/global/textcontext.dtd
>@@ -20,7 +20,10 @@
> <!ENTITY spellUndoAddToDictionary.label "Undo Add To Dictionary">
> <!ENTITY spellUndoAddToDictionary.accesskey "n">
> <!ENTITY spellCheckToggle.label "Check Spelling">
>-<!ENTITY spellCheckToggle.accesskey "g">
>+<!ENTITY spellCheckToggle.accesskey "s">
>+<!ENTITY grammarCheckToggle.label "Check Grammar">
>+<!ENTITY grammarCheckToggle.accesskey "g">
>+<!ENTITY grammarNoSuggestion.label "(No Grammar Suggestions)">
> <!ENTITY spellNoSuggestions.label "(No Spelling Suggestions)">
> <!ENTITY spellDictionaries.label "Languages">
> <!ENTITY spellDictionaries.accesskey "l">
>diff --git a/tools/mach_commands.py b/tools/mach_commands.py
This stuff looks like something an addon should add.

>+++ b/tools/mach_commands.py
>@@ -75,7 +75,7 @@ class Interface(object):
>     from, what its uuid is, and where in the source file the uuid is.
>     '''
>     def __init__(self, filename, production):
>-        import xpidl
>+        from xpidl import xpidl
>         assert isinstance(production, xpidl.Interface)
>         self.name = production.name
>         self.base = production.base
>@@ -172,7 +172,7 @@ class UUIDProvider(object):
>                           'Their descendants are updated as well.')
>     def update_uuids(self, path, interfaces):
>         import os
>-        import xpidl
>+        from xpidl import xpidl
>         from mozpack.files import FileFinder
>         import mozpack.path as mozpath
>         from tempfile import mkdtemp
totally unrelated changes. If this is fixing some bug, please file a new bug and upload this small part of the patch there.

Comment 23

[Olivier R.]

The bounty for this feature is now $815.
https://www.bountysource.com/issues/9358930-develop-an-interface-for-grammar-checkers

Comment 24

[Chase Vasic]

I will preface what I'm thinking with, I am not familiar with the Firefox codebase. It doesn't seem like there is too much interest in this. However, I don't think it's a bad idea, in some regard, anyway. I personally use Grammarly and it functions well, but I think that for security reasons it would be better if there was an interface for extensions to deal directly with text editing, rather than accessing the entire page.

What I am thinking, what I will propose, and possibly look at working to help implement, is an interface for extensions that specifically deal with text input. What I am thinking of is a generic interface that would provide what is necessary for a wide range of plugins including spell-check, grammar-checking, translation, and alternative input like speech-to-text, or vim/emacs editing (I use an extension called GhostText to edit textareas with vim, and I would really like if it could more directly manage text input.)

If I am the person to implement such an interface, I will need to become acclimated with this codebase much more than I am currently. I would appreciate input from anyone who is either interested in a generic textarea input/checking/modification interface, or anyone who can explain why this might be unnecessary or even counterproductive. I am thinking that if a generic interface was created it benefit Firefox in several ways:
1) Extensions that intend to deal with form data, or more specifically, textarea data, can have greatly restricted access permission. A grammar extension should *only* be able to read and write to textareas, or text inputs, not the entire page.
2) The Firefox codebase could remain slim by providing a single interface which provides for a range of extensions which (as previously mentioned) include spellchecking, grammar checking, translation, speech-to-text, and other types of text input/manipulation.
3) User experience can be improved by this interface because it would provide access to text fields for extensions which is more direct than current interfaces provide.

I will look into what is necessary for such an interface to be implemented. I am not a Firefox developer or even a Firefox extension developer, so I very well may have a naive view of this issue. Since this bug has been open for so long I am assuming that what I am thinking of is worth implementing though. So, I will look into what is necessary and return to either describe a potential feature implementation, or explain why this bug should be closed.

Comment 25

[fluks]

(In reply to Chase Vasic from comment #24)

What I am thinking, what I will propose, and possibly look at working to
help implement, is an interface for extensions that specifically deal with
text input. What I am thinking of is a generic interface that would provide
what is necessary for a wide range of plugins including spell-check,
grammar-checking, translation, and alternative input like speech-to-text, or
vim/emacs editing (I use an extension called GhostText to edit textareas
with vim, and I would really like if it could more directly manage text
input.)

A general webextension text input API would be a lot better indeed, rather than only for grammar checking. The spellchecking engines currently included in Firefox aren't well suited for Finnish and the developers seem to be unwilling to include a new engine (https://bugzilla.mozilla.org/show_bug.cgi?id=1403735). I tried to make a webextension because libvoikko can be compiled into webassembly, but highlighting text input elements is very difficult. I don't know how well Grammarly does it. Are there any open source add-ons that can do it exceptionally well? I couldn't find any.

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 10 votes.
:jjalkanen, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 27

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.