333038 - Support for inline spellchecking for designMode

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Robert Strong (they/them - no direct email)]

We should support inline spellchecking for designMode / midas for Firefox 2.0

Comment 1

[Brett Wilson]

I'm swapping the dependencies between this bug and the land spellchecking bug to reflect what will actually happen (I'm treating that other bug as migration of the current stuff from trunk). If this goes in to 2.0 it will happen long after the "regular" spellchecking is on branch.

Comment 2

[Mike Connor [:mconnor]]

Who's going to own this?  cc-ing Blake in case he knows what type of rabbit hole we'll be going down here.

Nice to have, but I'd ship without it.

Comment 3

[Robert Strong (they/them - no direct email)]

One way to get the editor in JavaScript after finding a document with designMode="on"

var docshell = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
                  .getInterface(Components.interfaces.nsIWebNavigation)
                  .QueryInterface(Components.interfaces.nsIDocShell);

if (docshell instanceof Components.interfaces.nsIEditorDocShell)
  var editor = docshell.editor;


Not sure what it will take to add the context menu items for inline spellchecking of a designMode document.

Comment 4

[Michael Wu [:mwu]]

Inline spellchecking seems rather unhelpful without the corresponding context menu items, and adding proper context menu items for designMode is covered by bug 312930.

Comment 5

[Mike Connor [:mconnor]]

-> Enn, unless mwu has something in the way of a patch

Comment 6

[Brett Wilson]

The context menu is easy to make work. You just need to make to context menu code in browser.js know when it's in a designmode document and set the flag accordingly. There may need to be some other tweaks to the code if it assumes anything about textfields.

Comment 7

[Neil Deakin]

Does this mean having spellchecking by default, or just an option on the context menu to enable it? The latter is already covered by bug 312930.

Comment 8

[Brett Wilson]

*** Bug 338470 has been marked as a duplicate of this bug. ***

Comment 9

[Martijn Wargers (dead)]

Fixed by bug 312930.

Comment 10

[Neil Deakin]

No, this isn't fixed. mconnor says that spellchecking for design mode should be on by default. I already have patch bing worked on.

Comment 11

[Brett Wilson]

Correct, it should follow the same preference as textareas.

Comment 12

[Neil Deakin]

Attachment: Enable spellchecking for design mode documents

Comment 13

[Boris Zbarsky [:bzbarsky]]

I probably won't get to this until next week...

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 226652 [details] [diff] [review]
Enable spellchecking for design mode documents

(clearing approval1.8.1?)

Can we get someone other than bz to review this patch, please? Once that's done, check it in on trunk, mark it fixed and then resubmit the approval request?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 226652 [details] [diff] [review]
Enable spellchecking for design mode documents

>Index: content/html/document/src/nsHTMLDocument.cpp

>+nsHTMLDocument::SetEnableRealTimeSpell(nsPIDOMWindow* window,

>+  nsCOMPtr<nsIDOMWindow> domWindow = do_QueryInterface(window);

No need; nsPIDOMWindow inherits from nsIDOMWindow, so you can just pass |window| through.

Oh, and call them aWindow and aEditSession.

>Index: content/html/document/src/nsHTMLDocument.h

>+  void SetEnableRealTimeSpell(nsPIDOMWindow* window,
>+                              nsIEditingSession* editSession);

Toss in some comments explaining what this does?

sr=bzbarsky

Comment 16

[Neil Deakin]

Checked in on trunk

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Neil, with a fresh profile on Windows, I'm seeing textareas with design mode not having spell check on by default. Have you seen this before?

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

So, using July 27th nightly of Minefield, I can confirm that both:

  http://www.gmail.com and 
  http://www.mozilla.org/editor/midasdemo/ 

have spell checking turned off by default in Windows using a clean profile. Oddly, the fields have spell checking turned on by default in OS X.

Adding qawanted.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 226652 [details] [diff] [review]
Enable spellchecking for design mode documents

Please renominate this for blocking once we're sure that there are no regressions or problems with the functionality.

Comment 20

[Neil Deakin]

Works fine for me on Windows with a new profile using a July 4 build.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 226652 [details] [diff] [review]
Enable spellchecking for design mode documents

Please go ahead and land this on the branch.

Comment 22

[Neil Deakin]

A different patch is needed for the branch here, the spellchecker interfaces called are different.

Comment 23

[Neil Deakin]

Attachment: Branch version of patch - does it need review?

Comment 24

[Mike Schroepfer]

Brettw and Bz are both out - so maybe PKasting can take a look?

Comment 25

[Peter Kasting]

Comment on attachment 228346 [details] [diff] [review]
Branch version of patch - does it need review?

I assumed the trunk patch was OK and just reviewed the differences to the branch patch.  Everything spellcheck-related looks good to me.  The only question I had was as follows:

In nsHTMLDocument::RealTimeSpellCallback(), the trunk patch looks like:
 +    nsPIDOMWindow *window = doc->GetWindow();
+    if (window) {
+      nsIDocShell *docshell = window->GetDocShell();
+      if (docshell) {
+        nsCOMPtr<nsIEditingSession> editSession = do_GetInterface(docshell);

While the branch patch looks like:
+    nsCOMPtr<nsIDOMWindow> window(do_QueryInterface(doc->GetScriptGlobalObject()));
+    if (window) {
+      nsCOMPtr<nsISupports> container = doc->GetContainer();
+      nsCOMPtr<nsIEditingSession> editSession = do_GetInterface(container);

I don't know enough about how the editing session is hooked up to know whether the branch change to get a container off the document is the right way to go; I'll assume so.  But it does make me wonder whether we still need to be checking if we can get a window here, since we don't get anything from it like we do on trunk.

Other than that, things look good to me.

Comment 26

[Mike Schroepfer]

Comment on attachment 228346 [details] [diff] [review]
Branch version of patch - does it need review?

Approving for drivers...

Comment 27

[Myk Melez [:myk] [@mykmelez]]

Landed on the 1.8 branch.

Comment 28

[ReleaseEngineer]

The solution to turn off spell checking when not needed for form elements has been introduced with the fix in Bug 339127. And how do we turn it off when we don't need it in designMode? 

perhaps something similar to execCommand("spellCheck",false,"false");??

Comment 29

[Peter Kasting]

(In reply to comment #28)
> The solution to turn off spell checking when not needed for form elements has
> been introduced with the fix in Bug 339127. And how do we turn it off when we
> don't need it in designMode? 

You should be able to set the spellcheck attribute to false on the BODY of the document that's in designmode (or going to be in designmode).  Designmode is treated like contenteditable is set on the BODY, so you need to adjust the value of spellcheck there as well.

I'm not sure if I have any testcases lying around which demonstrate this.  If you run into problems with this approach, please post a new bug and attach your testcase.

Comment 30

[Alfonso Martinez]

comment 29 works perfectly, but could it be possible to somehow improve the user experience in designMode fields?
I'm thinking about letting the document know if the current right clicked word is misspelled and the possible suggestions, so instead of showing the full context menu just show the suggestions, or if the page is using a custom context menu then also show the suggestions (or just the suggestions)

taking privacy considerations aside for a moment, does anybody think that this is worth filing as a RFE?

Comment 31

[Adam Guthrie]

Alfonso, that's definitely outside the scope of this bug. Please file a separate bug if you can't find any duplicates.