Please consider adding access to Mac OS X global spelling checker for all text blocks. I spend so much time filling out forms and such on the web (like this one for instance!) that it would really be helpfull to have access to a spelling checker. See OmniWeb for a decent implementation of this feature.

good idea

Related to/duplicate of bug 86886?

-->brade

Related to/Dependant on bug <a href="http://bugzilla.mozilla.org/show_bug.cgi?id=148098"> 148098</a>?

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

This would be a nice feature, but I wouldn't prioritise it over fixing bug 148098... we don't need form input to get any slower on old machines. :-)

If we implement a native text widget (bug 148098, we get spell-checking (as well as services and things listed in a dozen other bugs) for free. It is probably also possible to hack in the spell checker while still using gecko text, but this is still unlikely to create any major performance hit. It _is_ going to be important to decide at some point whether the difficulties implementing features to fix all the problems (both with features and with performance) in gecko text inputoutweigh the work it would take to use a native widget; this bug is definitely closely realated to 148098, even though it doesn't necessarily have to depend on it.

I think that the use native widgets would slow down scrolling and page rendering. Omniweb, for example, is switching from native widgets because of speed issues (or so I heard a couple months ago).

I've looked into this a little and the biggest problem seems to be a way to draw the red dots under the misspelt words. I don't know if there is any way to do this within a text field in Gecko.

How about just drawing the text in red, italics, perhaps? Most text entry widgets don't support styled text anyhow.

drawing the red dots isn't a problem (iirc); I ran into problems with the OSX spellchecker not showing its dialog.

This patch is where I left off (includes debugging etc); may be missing some nib/resource changes.

brade what does it do?

kin: see patch attached (it's VERY rough) Simon Woodside: the patch is a first attempt to invoke the OSX spell checker for textareas / input fields. More needs to be done before it can land.

Anny news, would be nice for 1.0 release?

This is probably about the same as the previous patch with some other things thrown in too. I am about to lose this computer so posting this patch in case someone else wants to pick it up and make it work.

If Useful: Apple recently posted some sample code to the reference library. Sample Code: SpellingChecker-CocoaCarbon http://developer.apple.com/samplecode/SpellingChecker-CocoaCarbon/SpellingChecker-CocoaCarbon.html

If Useful: Apple recently posted some sample code to the reference library. Sample Code: SpellingChecker-CocoaCarbon http://developer.apple.com/samplecode/SpellingChecker-CocoaCarbon/SpellingChecker-CocoaCarbon.html

Ludo, would you mind if I looked at this?

(In reply to comment #19) > Ludo, would you mind if I looked at this? Not at all

I'm taking it....for better or worse =)

Assuming we use the gecko inline editor work, depends on bug 58612. I think we'll also have to implement nsIEditorSpellCheck to wrap the cocoa spell checker.

Hey all, This is a beta patch. I just wanted to invite anyone watching to give their thoughts on it. Things NOT implemented are: 1) spellcheck as you type. It just beeps - I know. 2) underlining or coloring misspelled words. This kinda goes with #1 - doesn't matter too much by itself. Note on the patch: the way I currently implement the Ignore feature is to have all text fields on a page share an ignore list. Any fields in another window or tab have a separate ignore list. The ignore list is thrown away whenever the location in a tab changes.

Julian I was quickly looking at the nib you attached and I was wondering. In OS X text area contextmenu the menu looks as follows: Guesses Section ------ Ignore Spelling Learn Spelling ------ Everything beneath these options are in your nib file I was just wondering if you also had these opptions as they are pretty necesarry.

...

I found a way that Mozilla uses to mark things as misspelled. This shows what it looks like. It's a solid underline but I think it looks fine.

I'd love to pick this up again. But, 58612 hasn'gt been touched for awhile. Maybe I should apologize on bug 58612 for my rude comment, and then gently ask them to pick it up again :)

Can we get someone with some clout :-) to "lean on" bug 58612 so that it gets fixed in the Gecko 1.9 cycle so that we can actually get spell-checking by the time Camino 1.1 rolls around off the trunk again...?

For all that is sacred in this world, can someone...anyone, please work on this? The masses are awaiting spelcheking in Camino.

(In reply to comment #30) > For all that is sacred in this world, can someone...anyone, please work on this? > The masses are awaiting spelcheking in Camino. This bug is dependent on a core bug. Until that core bug is fixed, there won't be spell checking in Camino. If you want this to be fixed, go vote for bug 58612.

(In reply to comment #31) > (In reply to comment #30) > > For all that is sacred in this world, can someone...anyone, please work on this? > > The masses are awaiting spelcheking in Camino. > > This bug is dependent on a core bug. Until that core bug is fixed, there won't > be spell checking in Camino. If you want this to be fixed, go vote for bug 58612. I would expect Firefox spellchecker to depend on bug 58612, as it is crossplatform. But why should Camino's ? Bug 58612 sounds like a complete implementation. (With discussion over how to render the erroneous words and what event should trigger spell checking.) For Camino, we should aim at getting as close as possible to how Safari works, that is rely on Mac OS X spell checker. (Same dictionary, same dialog box for spell check settings, same rendering of erroneous words, etc.) (As was initially requested for that bug.) This is certainly possible to reuse in a web browser if Safari team got it right. Isn't it the whole idea behind Camino: leverage Mac OS X functionalities to better fit into the mac user experience ?

I wuold be working on this wholeheartedly, as I think it is possible to do it right without requiring the Core bug, but I am extremely distracted since at this moment I do not have a place to live starting Sep 23.

Because this needs changes to core which won't get fixed until Gecko 1.9 (see bug 58612), targeting for Camino 2.0.

here's what we need to do now that the spellcheck code has landed on the trunk (from brettw): 1. Enable the spellcheck extension and make sure it gets distributed. Also, be sure you have the dictionaries you want. They go in components/myspell. 2. Set layout.textarea.spellcheckDefault = true. This will make textareas get spellchecking by default. 3. Check that this works. There will be no UI, but you should see the red underlining if everything is set up properly. 4. Add the context menu stuff. See my patch for browser.js on the bug. You can call my shared JS file for most of the tasks. You may want to wait on this part for a few weeks: my shared JS UI file might get converted into a component or something else.

(In reply to comment #35) > here's what we need to do now that the spellcheck code has landed on the trunk > (from brettw): > > 1. Enable the spellcheck extension and make sure it gets distributed. > Also, be sure you have the dictionaries you want. They go in > components/myspell. Shouldn't we use the OS X dictionaries? Any idea how we could do that?

Yes, we should. Is there a dictionary API that we can implement?

(In reply to comment #37) > Yes, we should. Is there a dictionary API that we can implement? Assuming this needs to be done in Core(?), this looks like it might be the right API: http://developer.apple.com/documentation/Carbon/Reference/Dictionary_Manager/index.html

Comment on attachment 172534 [details] [diff] [review] beta patch rumor has it someone's working on this for real, again ;)

I tried to add some new files into the patch using /dev/null, I hope it works with 'patch' =) --------- Changes in order to build: Add to mozconfig: --enable-extensions=default,spellcheck To the pbxproj: Add src/embedding/CHEditorSpellCheck* In the Targets tab, Add spellcheck.xpt to Copy Files (64) (I think it's the first Copy Files) In the Targets tab, Add libspellchecker.dylib to the next Copy Files Add to HEADER_SEARCH_PATHS: ../dist/include/editor ../dist/include/txtsvc My changes introduced some build error which I had to work around, this is what it looks like: /mozilla-head/mozilla/camino/src/embedding/CHBrowserView.mm: In function `unsigned int -[CHBrowserView draggingEntered:](CHBrowserView*, objc_selector*, objc_object*)': /mozilla-head/mozilla/camino/src/embedding/CHBrowserView.mm:1528: warning: invalid conversion from `int' to `OpaqueDragRef*'

Comment on attachment 208498 [details] [diff] [review] changes to core Hi Boris, I noticed you reviewed the core patch for bug 302050. Would you mind taking a look at this one?

Comment on attachment 208498 [details] [diff] [review] changes to core For future reference, this would be a _lot_ easier to review if you use better diff options. Something like diff -up8 or so. -p puts in the function names, -8 gives more context. >Index: extensions/spellcheck/src/mozEnglishWordUtils.cpp >Index: extensions/spellcheck/src/mozInlineSpellChecker.h I can't review this part; please get someone familiar with spell-checker to do so. >Index: layout/forms/nsTextControlFrame.cpp > nsTextControlFrame::Destroy(nsPresContext* aPresContext) > { >+ nsContentUtils::UnregisterPrefCallback(PREF_DEFAULT_SPELLCHECK, >+ nsTextControlFrame::RealTimeSpellCallback, this); This is mis-indented. >@@ -1705,7 +1707,12 @@ >+ // we working on Camino decided to allow inline spellcheck for single line fields >+#ifdef MOZ_MACBROWSER That's fine but NOT with ifdefs. Add a pref to control it, and set it in your config or something. >nsTextControlFrame::RealTimeSpellCallback(const char* pref, void* context) >+{ This should assert that |pref| is what you expect. Also, aPref, aContext for the args. >+ nsTextControlFrame* frame = NS_REINTERPRET_CAST(nsTextControlFrame*, context); NS_STATIC_CAST, please. >+ if (frame) Under what conditions would |frame| be null? If it can't be, assert instead of checking. >+ return NS_OK; This function doesn't return an nsresult. It returns an int. I suggest returning 0. >+ >+ Why so much space? One blank line between functions should be about right. >Index: layout/forms/nsTextControlFrame.h >+ static NS_HIDDEN_(int) PR_CALLBACK RealTimeSpellCallback(const char* pref, void* closure); I'm not sure NS_HIDDEN is a good idea here. I'd take it out.

apologies

I guess I'll contact bryner about the spellcheck part

I suggest contacting a spellcheck owner/peer (like mscott) for the spellcheck part, actually.

Comment on attachment 209190 [details] [diff] [review] changes to core v2 >Index: layout/forms/nsTextControlFrame.cpp > #define PREF_DEFAULT_SPELLCHECK "layout.textarea.spellcheckDefault" >+#define PREF_SPELLCHECK_SINGLE_LINE "browser.spellcheck.singleLine" Wouldn't it make more sense to call the new pref "layout.textInput.spellcheckDefault"? Compare with the "layout.textarea.spellcheckDefault" pref? And perhaps change things such that it's possible to spell-check single-line inputs even if you're not spell-checking textareas? That is, depending on whether this is a single or multi-line control register for different prefs and so forth?

(In reply to comment #48) > (From update of attachment 209190 [details] [diff] [review] [edit]) > >Index: layout/forms/nsTextControlFrame.cpp > > #define PREF_DEFAULT_SPELLCHECK "layout.textarea.spellcheckDefault" > >+#define PREF_SPELLCHECK_SINGLE_LINE "browser.spellcheck.singleLine" > > Wouldn't it make more sense to call the new pref > "layout.textInput.spellcheckDefault"? Compare with the > "layout.textarea.spellcheckDefault" pref? And perhaps change things such that > it's possible to spell-check single-line inputs even if you're not > spell-checking textareas? That's exactly the point: the pref doesn't single handedly turn on or off spellchecking for text inputs; it turns it on or off *given that* spellchecking is on. It treats the already existing pref as a master pref. I saw no use in making it possible to enable realtime spell on JUST single-line inputs. > > That is, depending on whether this is a single or multi-line control register > for different prefs and so forth? > I'm sorry; what?

> That's exactly the point: the pref doesn't single handedly turn on or off > spellchecking for text inputs; it turns it on or off *given that* spellchecking > is on. Right. This is the part I don't understand. Why? That is, why apply a pref that controls spellchecking for textareas to text inputs and then have _another_ pref to decide whether it applies? Why not just apply one pref to textareas and another to text inputs? As things stand, to make your patch correct you would need to register an observer for both prefs. And then you still have the conceptual weirdness of a textarea pref affecting the behavior of text inputs...

For what its worth, I originally was planning on having one pref with values 0 (off) 1 (on for textareas) 2 (on for everything). But then we moved the pref to be called *.textarea.* which makes this make less sense. There's no reason I can think of why you would want to have spellchecking on by default for single-line, but off by default for multi-line. But having two prefs control three states as Julian did seems a little strange to me. Would it make people happier to rename it to "layout.spellcheckDefault" and have three possible values?

Comment on attachment 209190 [details] [diff] [review] changes to core v2 Hi, Would you want to lend a hand and review the changes to extensions/spellcheck that are in this patch?

includes small fixes

Yeah, making it a tri-state would also work for me. We didn't ship that pref in Firefox 1.5, did we?

Comment on attachment 209190 [details] [diff] [review] changes to core v2 Is this include really necessary? +#include "mozISpellI18NUtil.h" I don't see where you've added any new code to the spellchecker that would depend on that header file. I'm not comfortable reviewing the layout changes. I'll leave that to bz and others.

(In reply to comment #55) > (From update of attachment 209190 [details] [diff] [review] [edit]) > Is this include really necessary? > > +#include "mozISpellI18NUtil.h" > > I don't see where you've added any new code to the spellchecker that would > depend on that header file. The containing header file, mozInlineSpellChecker.h, has build errors when I build on my Mac, related to the use of mozISpellI18NUtil. Probably there is a difference in the include tree between platforms.

Hi Boris, I hope this is the final core patch

Some more info about building in Camino, I see I added some additional header search paths: ../editor/composer/src ../extensions/spellcheck/idl/_xpidlgen ../extensions/spellcheck/src I guess these should be added to dist like the other items. I'll leave that for the sr to consider that...

Comment on attachment 209805 [details] [diff] [review] changes to core v3 >Index: layout/forms/nsTextControlFrame.cpp >@@ -130,13 +130,13 @@ >-#define PREF_DEFAULT_SPELLCHECK "layout.textarea.spellcheckDefault" >+#define PREF_DEFAULT_SPELLCHECK "layout.spellcheckDefault" You need to change places that use the old pref value (eg firefox.js). I don't see changes to those in this patch. Note that you need to change not only the name but the value in that file. >@@ -1702,20 +1704,38 @@ nsTextControlFrame::SyncRealTimeSpell() >+ // check the pref to see what the default should be, default is 0: never spellcheck >+ PRInt32 spellcheckLevel = nsContentUtils::GetIntPref(PREF_DEFAULT_SPELLCHECK, 0); >+ if (!readOnly) { Why not get the pref once we have !readonly? Seems like it would be more efficient in that case, and identical in the other cases. >+ enable = true; PR_TRUE. r=bzbarsky with those three issues fixed. Thanks!

Hi Brian, wanna sr this? I think you're the man

be sure to use both v3 patches

--- src/browser/GoMenu.mm 29 Aug 2005 17:55:04 -0000 1.18 +++ src/browser/GoMenu.mm 30 Jan 2006 02:24:13 -0000 @@ -58,12 +58,13 @@ #import "HistoryItem.h" #import "HistoryDataSource.h" #import "GoMenu.h" +#include <Carbon/Carbon.h> is this necessary? +// Spellchecking +- (IBAction)chCheckSpelling:(id)sender; +- (IBAction)chShowGuessPanel:(id)sender; +- (IBAction)chToggleContinuousSpellChecking:(id)sender; why the |ch| prefix? +#define CH_ENSURE_SUCCESS(res) \ + if (!NS_SUCCEEDED(res)) return; isn't this already a gecko macro? why create our own? + else { + [self closeSpellDocument]; + mSpellDocumentTag = [NSSpellChecker uniqueSpellDocumentTag]; + } nit. fix indents. 2 space. + nsCOMPtr<nsIDOMWindow> domWindow = getter_AddRefs([self getContentWindow]); + nsCOMPtr<nsPIDOMWindow> privateWindow = do_QueryInterface(domWindow); + if (!privateWindow) + return NULL; again fix indents. several times in this routine. + nsCOMPtr<nsITextControlElement> textElem = do_QueryInterface(focusedItem); + nsITextControlElement *returnElem = textElem; + return returnElem; this is generally a bad pattern, we should avoid returning un-addref'd interfaces from functions. + // NOTE |GetCurrentDoc| doesn't AddRef?! + nsIDocument* doc = content->GetCurrentDoc(); + if (!doc) + return; + nsIPresShell* presShell = doc->GetShellAt(0); + if (!presShell) + return; are you sure about these not addRef'ing? + if (spellChecker == nsnull) { use |if (!spellChecker) {| instead + CHEditorSpellCheck* edSpellChecker = NS_REINTERPRET_CAST(CHEditorSpellCheck*, spellChecker); that seems really skanky. why can we make this assumption? Why not add another platform-specific interface? +// Imitate the spellcheck actions for Cocoa text fields. +// "chFoo" actions avoid collision with the real Cocoa non-ch actions, we add ch +// so that Cocoa text fields such as the location bar don't respond to the spellcheck items this comment should be in the header file. +// The only difference between "Check Spelling" and "SpellingÉ" is that use "..." not the ellipsis char, so it's readable across platforms. + nsCOMPtr<nsITextControlElement> activeElement = [self focusedTextControlElement]; why is this wrapped in a nsCOMPtr if you didn't addref before the return? be consistent. + if (selStart != mSavedSelStart || selEnd != mSavedSelEnd) { + NSBeep(); + NSLog(@"Selection changed"); + return; + } who's going to see this log message? Why bother printing it? + int newState = [rtSpell state] == NSOnState ? 2 : 0; aren't there constants for these states? +- (int)spellDocumentTag +{ + return mSpellDocumentTag; +} + does this really need to be part of the public api? should it be a private category? --- src/history/HistoryOutlineViewDelegate.mm 15 Jan 2006 21:03:32 -0000 1.10 +++ src/history/HistoryOutlineViewDelegate.mm 30 Jan 2006 02:24:15 -0000 @@ -47,12 +47,14 @@ #import "BrowserWindowController.h" #import "BookmarkViewController.h" #import "MainController.h" #import "HistoryOutlineViewDelegate.h" +#include <Carbon/Carbon.h> + again, not necessary +class CHEditorSpellCheck: public nsIEditorSpellCheck { open brace on a line by itself please + nsCOMPtr<nsITextServicesFilter> mTxtSrvFilter; // weak + nsCOMPtr<nsITextControlFrame> mCurTextControl; // weak + nsCOMPtr<nsIDOMNode> mDocRoot; // weak + nsCOMPtr<nsIEditor> mEditor; // weak how can these be weak, we own a strong ref to them with the nsCOMPtr! + static CHEditorSpellCheck* mSharedSpellCheck; // never dies after creation prefix statics with |s|, eg |sSharedSpellCheck| +CHEditorSpellCheck::CHEditorSpellCheck() +: mCurGuesses(nil), +mCurGuessIndex(-1), +mSpellDocTag(-1) +{ +} isn't there an init call taht needs to be made here to init the refcount? +NS_IMETHODIMP +CHEditorSpellCheck::CanSpellCheck(PRBool* canSpellCheck) +{ + *canSpellCheck = PR_TRUE; + return NS_OK; +} you should validate that |canSpellCheck| isn't null first. + PRInt32 begin,end; + PRBool isMisspelled = PR_FALSE; + PRBool docEOF = PR_FALSE; + PRBool wrapped = PR_FALSE; |begin| and |end| should be initialized here to something reasonable. Also, shouldn't |CHEditorSpellCheck| be a standalone component that firefox can use? We don't want to ship the dictionary for mac FF either.

Hi mike... (In reply to comment #62) > #import "GoMenu.h" > > +#include <Carbon/Carbon.h> > > is this necessary? Yes. All the added Carbon includes are necessary to allow us to remove it from PreferenceManager.h, which I need to include for CHBrowserView. Carbon.h pulls in a symbol collision for DragRef. I only need to include PrefMgr for the new define I added. Maybe that could go in its own file. > +#define CH_ENSURE_SUCCESS(res) \ > + if (!NS_SUCCEEDED(res)) return; > > isn't this already a gecko macro? why create our own? Which is it? This question open to everyone =) > + nsCOMPtr<nsITextControlElement> textElem = do_QueryInterface(focusedItem); > + nsITextControlElement *returnElem = textElem; > + return returnElem; > > this is generally a bad pattern, we should avoid returning un-addref'd > interfaces from functions. I learned it from some other BrowserView code, and the core code in the next comment. Nonetheless, I'll change it by the next patch unless you revoke the comment > + // NOTE |GetCurrentDoc| doesn't AddRef?! > + nsIDocument* doc = content->GetCurrentDoc(); > + if (!doc) > + return; > + nsIPresShell* presShell = doc->GetShellAt(0); > + if (!presShell) > + return; > > are you sure about these not addRef'ing? Yup, I followed all the core code that calls these. > + CHEditorSpellCheck* edSpellChecker = > NS_REINTERPRET_CAST(CHEditorSpellCheck*, spellChecker); > > that seems really skanky. why can we make this assumption? Why not add another > platform-specific interface? It's safe because we know that CHEditorSpellCheck is the implementor of nsIEditorSpellCheck according to AppComponents. I'm not sure about platform-specific interfaces - feel free to tell me more offline (well, online, but not here). > + nsCOMPtr<nsITextControlElement> activeElement = [self > focusedTextControlElement]; > > why is this wrapped in a nsCOMPtr if you didn't addref before the return? be > consistent. I need to hop the interfaces with QI. I suppose that's a good reason to make |focusedTextControlElement| return by ref? =) > > + int newState = [rtSpell state] == NSOnState ? 2 : 0; > > aren't there constants for these states? Yeah, in nsTextControlFrame.h (I created them). If we include that, we can use the constants. That file includes a *slew* of other files, though. > +- (int)spellDocumentTag > +{ > + return mSpellDocumentTag; > +} > + > > does this really need to be part of the public api? should it be a private > category? Except that, when doing realtime spellcheck, the CHEditorSpellCheck gets invoked without the BrowserView. But, the BrowserView owns the spellcheck tag. I couldn't come up with any other way of syncing the BrowserView to the CHEdSpellCheck. > + nsCOMPtr<nsITextServicesFilter> mTxtSrvFilter; // weak > + nsCOMPtr<nsITextControlFrame> mCurTextControl; // weak > + nsCOMPtr<nsIDOMNode> mDocRoot; // weak > + nsCOMPtr<nsIEditor> mEditor; // weak > > how can these be weak, we own a strong ref to them with the nsCOMPtr! I'm kinda asking if they should be weak. Should I hold on to them? > +CHEditorSpellCheck::CHEditorSpellCheck() > +: mCurGuesses(nil), > +mCurGuessIndex(-1), > +mSpellDocTag(-1) > +{ > +} > > isn't there an init call taht needs to be made here to init the refcount? In our constructor? Wouldn't that be in one of the base classes? Do you mean the NS_IMPL_ISUPPORTS stuff? I changed the file to use that instead of the separate macros. > + PRInt32 begin,end; > + PRBool isMisspelled = PR_FALSE; > + PRBool docEOF = PR_FALSE; > + PRBool wrapped = PR_FALSE; > > |begin| and |end| should be initialized here to something reasonable. Really? The core code doesn't. I'm all in favor of precedence... > Also, shouldn't |CHEditorSpellCheck| be a standalone component that firefox can > use? We don't want to ship the dictionary for mac FF either. Tell me more about standalone components.

What do we do to get someone to land the core patch?

(In reply to comment #64) > What do we do to get someone to land the core patch? > If it has r and sr then ask someone to land it for you.

Comment on attachment 210068 [details] [diff] [review] changes to core v3.1 (checked in) I checked this in on trunk. For future reference, it'd be good to put core patches in core bugs...

Comment on attachment 210068 [details] [diff] [review] changes to core v3.1 (checked in) We need this on the branch. bz: Are you the right person to ask for this stuff? Also, the changes to the spellchecker seemed to be for Camino. Does this affect Firefox on the Mac, or are we still planning on using myspell there too?

Comment on attachment 210068 [details] [diff] [review] changes to core v3.1 (checked in) Looks ok, assuming bug 302050 is going on branch

> Also, the changes to the spellchecker seemed to be for Camino. Does this affect > Firefox on the Mac, or are we still planning on using myspell there too? > Didn't the patch for bug 302050 include mozEnglishWordUtils? I don't know if that code is live or dead, but it's possible that Firefox is using that code for spellcheck. My change to that file would fix an issue that I assume would occur for any build that has realtime spell enabled, although I only know for sure that it was an issue in Camino. Also, if Firefox gets a UI for the realtime pref, which I don't think it has currently, then the changes to nsTextControlFrame will ensure that the pref is updated live.

whom should I flag for review?

patch notes: I needed to leave the change for the Carbon/Carbon.h includes because I needed to include PrefMgr.h for CHBrowserView after all. Everything else I said I'd do in comment 63 I did.

Comment on attachment 211692 [details] [diff] [review] changes to camino v3.1 > Index: src/application/MainController.mm > =================================================================== > + // sync realtime spellcheck setting > + // default is zero - off. 2 is on. > + int enableRealTimeSpellCheck = [prefsManager getIntPref:kRealTimeSpellPref withSuccess:&success]; > + if (!success || !enableRealTimeSpellCheck) { > + [mRealTimeSpellMenuItem setState:NSOffState]; > + } > + else { > + [mRealTimeSpellMenuItem setState:NSOnState]; > + } Does the menu this item is in have auto-updating turned off? Otherwise this won't stick. > Index: src/browser/GoMenu.mm > =================================================================== > RCS file: /cvsroot/mozilla/camino/src/browser/GoMenu.mm,v > retrieving revision 1.18 > diff -u -6 -p -r1.18 GoMenu.mm > --- src/browser/GoMenu.mm 29 Aug 2005 17:55:04 -0000 1.18 > +++ src/browser/GoMenu.mm 13 Feb 2006 06:44:52 -0000 > @@ -58,12 +58,13 @@ > > #import "HistoryItem.h" > #import "HistoryDataSource.h" > > #import "GoMenu.h" > > +#include <Carbon/Carbon.h> Put framework includes at the top, please. > Index: src/embedding/CHBrowserView.h > =================================================================== > +// Spellchecking > +// Imitate the spellcheck actions for Cocoa text fields. > +// "chFoo" actions avoid collision with the real Cocoa non-ch actions, we add ch > +// so that Cocoa text fields such as the location bar don't respond to the spellcheck items > +- (IBAction)chCheckSpelling:(id)sender; > +- (IBAction)chShowGuessPanel:(id)sender; > +- (IBAction)chToggleContinuousSpellChecking:(id)sender; > + > +- (void)checkSpelling:(id)sender; > +- (void)changeSpelling:(id)sender; > +- (void)ignoreSpelling:(id)sender; > +- (void)closeSpellDocument; > +- (int)spellDocumentTag; Spell checking adds a lot of code to CHBrowserView. Perhaps you could move it into a separate file which contains a category of CHBrowserView. It also adds some bad dependencies. Perhaps we should hide it all behind a protocol, and move the implementation up into non-embedding Camino code. > Index: src/embedding/CHBrowserView.mm > =================================================================== > +// Spellcheck integration > +#include "nsITextControlElement.h" > +#include "nsIFocusController.h" > +#include "nsIContent.h" > +#include "nsIFrame.h" > +#include "nsITextControlFrame.h" > +#include "nsIInlineSpellChecker.h" > +#include "nsIEditor.h" > +#include "nsIEditorSpellCheck.h" > +#include "nsISelection.h" > +#include "nsIPlaintextEditor.h" This strengthens my conviction. CHBrowserView is supposed to be a "model" gecko embedder, and most of these headers are not frozen and should not be used by embedders. It would be nice to move this all out. > // security > #include "nsISecureBrowserUI.h" > #include "nsISSLStatusProvider.h" > #include "nsISSLStatus.h" > #include "nsIX509Cert.h" > > #import "CHBrowserView.h" > +#import "PreferenceManager.h" You're adding a dependency here of lower-level code (in embedding) on higher-level code (in Camino). No can do. > +- (void)spellcheckCore; What does this mean? The method name is confusing > +#pragma mark - > +// Cocoa spellcheck integration > + > +// Get the current text control frame that has the focus, or NULL if none. > +- (already_AddRefed<nsITextControlElement>)getFocusedTextControlElement > +{ > + nsCOMPtr<nsIDOMWindow> domWindow = getter_AddRefs([self getContentWindow]); > + nsCOMPtr<nsPIDOMWindow> privateWindow = do_QueryInterface(domWindow); > + if (!privateWindow) > + return nsnull; Does this work in framesets? > +// Search for the misspelled word starting at the selection, wrapping if necessary > +- (void)spellcheckCore > +{ > + nsresult rv; Move this down to where you first use it. > + nsCOMPtr<nsITextControlElement> activeElement = [self getFocusedTextControlElement]; > + nsCOMPtr<nsIContent> content = do_QueryInterface(activeElement); > + if (!content) > + return; > + // NOTE |GetCurrentDoc| doesn't AddRef?! Right. Raw pointer return values are not addreffed. > + // manual spell check should use the inline spell checker's spell checker if possible. > + // otherwise we use our own editor spellcheck, which is shared between all > + // CHBrowserViews, and re-inited if the editor is different, i.e. > + // a different text field is active. So when do we fail to get the inline spell checker's spell checker? Your comment should say why/when this happens. > + CHEditorSpellCheck* edSpellChecker = NS_REINTERPRET_CAST(CHEditorSpellCheck*, spellChecker); > + if (edSpellChecker == NULL) { > + NSLog(@"Unable to get CHEditorSpellCheck"); > + return; > + } This is pretty evil. Normally if you need to do something like this, you create a private interface (nsPIFoo) and QI the object to it. How can you be sure that there isn't some other component in core which is creating spell checker objects? Note that going to about:plugins can get core objects instantiated that we normally override in AppComponents. See bug 285211. > + PRUnichar* prWord; > + rv = spellChecker->GetNextMisspelledWord(&prWord); > + NS_ENSURE_TRUE(rv,); prWord is leaked. (Sucks that the they don't use nsAString). > + // now that new range is highlighted, get the (slightly incorrect) flat range > + // to cache for our own purpose > + rv = textFrame->GetSelectionRange(&mSavedSelStart, &mSavedSelEnd); > + // not much ado if that failed Explain to the lay reader lwhen these offsets will be wrong. > +// Correct the spelling of a misspelled word already spellchecked > +- (void)changeSpelling:(id)sender > +{ > + nsresult rv; > + > + nsCOMPtr<nsITextControlElement> activeElement = [self getFocusedTextControlElement]; > + if (!activeElement) { > + NSLog(@"Focus lost"); > + return; > + } Remove NSLog in final patch. > + // Let's make sure that the user hasn't changed the selection since they checked the spelling > + // for the misspelled word, otherwise the spellchecker just inserts the text willy-nilly > + // this just compares the selected range at the time the spelling was checked, to > + // the time the ignore was issued; doesn't check that the identity of the focused > + // text frame is the same. > + int selStart, selEnd; > + textFrame->GetSelectionRange(&selStart, &selEnd); > + if (selStart != mSavedSelStart || selEnd != mSavedSelEnd) { > + NSBeep(); Beep? > + nsCOMPtr<nsIEditor> editor; > + rv = textFrame->GetEditor(getter_AddRefs(editor)); > + NS_ENSURE_TRUE(rv,); > + > + nsAutoString newWord; > + NSString* value = [[sender selectedCell] stringValue]; > + [value assignTo_nsAString:newWord]; > + nsCOMPtr<nsIDOMNode> node; > + PRInt32 offset; > + // get a node and offset for normal selection (which is the word to change) > + nsCOMPtr<nsISelection> selection; > + rv = editor->GetSelection(getter_AddRefs(selection)); > + NS_ENSURE_TRUE(rv,); > + rv = selection->GetFocusNode(getter_AddRefs(node)); > + NS_ENSURE_TRUE(rv,); > + rv = selection->GetFocusOffset(&offset); > + NS_ENSURE_TRUE(rv,); > + > + nsCOMPtr<nsIInlineSpellChecker> inlineSpChecker; > + rv = editor->GetInlineSpellChecker(PR_TRUE, getter_AddRefs(inlineSpChecker)); > + NS_ENSURE_TRUE(rv,); > + > + // if realtime spellcheck is on, we pass it to the inline checker > + // otherwise, it won't work, we have to do it ourselves > + PRBool enabled; > + rv = inlineSpChecker->GetEnableRealTimeSpell(&enabled); > + if (enabled) { > + nsAutoString word; > + [[[sender selectedCell] stringValue] assignTo_nsAString:word]; I think [sender selectedCell] needs some explanation. > + // pass a point in the highlighted word to the inline checker > + rv = inlineSpChecker->ReplaceWord(node, offset, newWord); > + } > + else { In this clause, 'offset' is not used, so maybe move the above code into here. > + // taken from nsTextServicesDocument::InsertText > + rv = editor->BeginTransaction(); > + if (NS_FAILED(rv)) > + return; > + nsCOMPtr<nsIPlaintextEditor> textEditor = do_QueryInterface(editor, &rv); > + // word is already the selection > + if (NS_SUCCEEDED(rv)) > + rv = textEditor->InsertText(newWord); > + editor->EndTransaction(); > + } > +} > + > +- (IBAction)chToggleContinuousSpellChecking:(id)sender > +{ > + NSMenuItem* rtSpell = (NSMenuItem*)sender; > + [rtSpell setState:([rtSpell state] == NSOnState ? NSOffState : NSOnState)]; > + int newState = [rtSpell state] == NSOnState ? 2 : 0; > + [[PreferenceManager sharedInstance] setPref:kRealTimeSpellPref toInt:newState]; So we're saving the pref separately from Cocoa. Should we, and does the right thing happen when native text fields (e.g. the url bar) have focus? > +// NSIgnoreMisspelledWords > +- (void)ignoreSpelling:(id)sender > +{ > + nsresult rv; > + > + // inline spellchecker will call CHEditorSpellCheck::IgnoreWordAllOccurrences > + // which will ignore the word with NSSpellChecker > + // inline checker will update the highlighting for this text frame,but > + // what about other text frames that have the word now ignored? Did you file a bug? > + nsCOMPtr<nsITextControlElement> activeElement = [self getFocusedTextControlElement]; > + if (!activeElement) { > + NSLog(@"Focus changed"); > + return; Remove NSLog. > + } > + nsCOMPtr<nsIContent> content = do_QueryInterface(activeElement); > + // NOTE |GetCurrentDoc| doesn't AddRef?! > + nsCOMPtr<nsIDocument> doc = content->GetCurrentDoc(); > + nsIPresShell* presShell = doc->GetShellAt(0); > + if (!presShell) > + return; > + > + nsIFrame* frame = presShell->GetPrimaryFrameFor(content); > + nsCOMPtr<nsITextControlFrame> textFrame = do_QueryInterface(frame); > + if (!textFrame) > + return; > + > + nsCOMPtr<nsIEditor> editor; > + rv = textFrame->GetEditor(getter_AddRefs(editor)); > + NS_ENSURE_TRUE(rv,); > + > + nsCOMPtr<nsIInlineSpellChecker> inlineSpChecker; > + rv = editor->GetInlineSpellChecker(PR_TRUE, getter_AddRefs(inlineSpChecker)); > + NS_ENSURE_TRUE(rv,); This hunk of code appears quite a bit. Factor it? > Index: src/history/HistoryOutlineViewDelegate.mm > =================================================================== > RCS file: /cvsroot/mozilla/camino/src/history/HistoryOutlineViewDelegate.mm,v > retrieving revision 1.10 > diff -u -6 -p -r1.10 HistoryOutlineViewDelegate.mm > --- src/history/HistoryOutlineViewDelegate.mm 15 Jan 2006 21:03:32 -0000 1.10 > +++ src/history/HistoryOutlineViewDelegate.mm 13 Feb 2006 06:44:54 -0000 > @@ -47,12 +47,14 @@ > #import "BrowserWindowController.h" > #import "BookmarkViewController.h" > #import "MainController.h" > > #import "HistoryOutlineViewDelegate.h" > > +#include <Carbon/Carbon.h> > + Put framework includes at the top, please. > +++ src/embedding/CHEditorSpellCheck.h 2006-02-12 19:54:13.000000000 -0800 > +/* > + * Class CHEditorSpellCheck: a replacement for the class > + * nsEditorSpellCheck, loaded into the spellcheck module at runtime. > + */ > +class CHEditorSpellCheck: public nsIEditorSpellCheck > +{ > +public: > + CHEditorSpellCheck(); > + virtual ~CHEditorSpellCheck(); > + > + NS_DECL_ISUPPORTS > + NS_DECL_NSIEDITORSPELLCHECK > + > + // get a shared spell checker that can be used (and recycled) during manual spellcheck > + static nsresult GetSharedSpellChecker(nsIEditorSpellCheck** aSpellCheck); > + void SetSpellDocumentTag(int spellDocTag); Why a setter and no getter? > --- /dev/null 2006-02-12 22:53:27.000000000 -0800 > +++ src/embedding/CHEditorSpellCheck.mm 2006-02-04 01:57:23.000000000 -0800 > +// static > +nsresult CHEditorSpellCheck::GetSharedSpellChecker(nsIEditorSpellCheck** aSpellCheck) > +{ > + if (!sSharedSpellCheck) { > + sSharedSpellCheck = new CHEditorSpellCheck; > + } > + *aSpellCheck = sSharedSpellCheck; > + return *aSpellCheck ? NS_OK : NS_ERROR_NULL_POINTER; > +} Someone needs to hold a reference to this object, otherwise it might get deleted. > +NS_IMETHODIMP > +CHEditorSpellCheck::InitSpellChecker(nsIEditor* editor, PRBool enableSelectionChecking) > +{ > + nsresult rv; > + > + // we try to reuse the same EditorSpellCheck if the user is only using manual check > + if (editor == mEditor) { > + NSLog(@"-[CHEditorSpellCheck InitSpellChecker]: editor the same, not reiniting"); > + return NS_OK; > + } Remove NSLog > +// spell check with wrapping > +NS_IMETHODIMP > +CHEditorSpellCheck::GetNextMisspelledWord(PRUnichar** nextMisspelledWord) > +{ > + // It would be nice to get the flat string of the text frame and pass the > + // whole thing to NSSpellChecker. However, > + // we wouldn't know the block index and offset of the misspelled word in > + // the TSD. > + nsresult rv = EnsureConverter(); > + NS_ENSURE_SUCCESS(rv, rv); > + if(!mWordUtil) > + return NS_ERROR_NULL_POINTER; > + > + // Here we build a range that has the entire editor contents and reinit > + // the TSD with it > + nsCOMPtr<nsIDOMRange> docRange = > + do_CreateInstance("@mozilla.org/content/range;1", &rv); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = docRange->SelectNodeContents(mDocRoot); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = mTsDoc->SetExtent(docRange); > + NS_ENSURE_SUCCESS(rv, rv); > + > + PRUint32 selOffset; > + rv = SetupDoc(&selOffset); > + NS_ENSURE_SUCCESS(rv, rv); > + > + PRInt32 begin,end; > + PRBool isMisspelled = PR_FALSE; > + PRBool docEOF = PR_FALSE; > + PRBool wrapped = PR_FALSE; > + nsCOMPtr<nsIDOMRange> wrapExtent; > + // get a range of the current cursor position which we might use for wrapping > + rv = mTsDoc->GetDOMRangeFor(selOffset, 0, getter_AddRefs(wrapExtent)); > + NS_ENSURE_SUCCESS(rv, rv); > + > + while (!isMisspelled && (!wrapped || !docEOF)) > + { > + nsString str; > + rv = mTsDoc->GetCurrentTextBlock(&str); > + > + if (NS_FAILED(rv)) > + return rv; Remove blank line before, add one after. > + do { > + rv = mWordUtil->FindNextWord(str.get(),str.Length(),selOffset,&begin,&end); Spaces after commas please. > + if(NS_SUCCEEDED(rv)&&(begin != -1)){ Space after if. > + const nsAString &currWord = Substring(str, begin, end - begin); > + rv = CheckWordInternal(currWord, &isMisspelled, YES); > + NS_ENSURE_SUCCESS(rv, rv); > + if(isMisspelled){ > + *nextMisspelledWord = ToNewUnicode(currWord); > + rv = mTsDoc->SetSelection(begin, end-begin); > + rv = mTsDoc->ScrollSelectionIntoView(); > + break; > + } > + } > + if (end != -1) > + selOffset = end; > + } while (end != -1); > + > + // wrapping stuff here: we've reached the end, gotta wrap > + // lazily set the start of the wrap range and set the TSD extent > + if (NS_SUCCEEDED(mTsDoc->IsDone(&docEOF)) && docEOF && !wrapped) { > + rv = wrapExtent->SetStart(mDocRoot, 0); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = mTsDoc->SetExtent(wrapExtent); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = mTsDoc->FirstBlock(); > + NS_ENSURE_SUCCESS(rv, rv); > + wrapped = PR_TRUE; > + docEOF = PR_FALSE; > + } > + else > + mTsDoc->NextBlock(); > + > + selOffset=0; > + } > + > + if (!isMisspelled) > + *nextMisspelledWord = ToNewUnicode(NS_LITERAL_STRING("")); Can't just return nsnull? > + return rv; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::GetSuggestedWord(PRUnichar** suggestedWord) > +{ > + nsAutoString suggestion; > + if (mCurGuessIndex < [mCurGuesses count]) { > + [[mCurGuesses objectAtIndex:mCurGuessIndex++] assignTo_nsAString:suggestion]; > + } What if mCurGuessIndex is still -1 ? > + *suggestedWord = ToNewUnicode(suggestion); > + return NS_OK; > +} > +NS_IMETHODIMP > +CHEditorSpellCheck::CheckWordInternal(const nsAString& word, PRBool* isMisspelled, > + BOOL computeSuggestions) > +{ > + // This is the case at the start of inline spellcheck > + if (mSpellDocTag == -1) { > + nsCOMPtr<nsIDOMDocument> domDoc; > + nsresult rv = mEditor->GetDocument(getter_AddRefs(domDoc)); > + nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc); > + nsCOMPtr<nsISupports> container = doc->GetContainer(); > + nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(container); > + > + CHBrowserView* browserView = [CHBrowserView browserViewFromDOMWindow:window]; > + if (!browserView) { > + NSLog(@"Failed to get current browser view"); > + return NS_ERROR_FAILURE; > + } > + mSpellDocTag = [browserView spellDocumentTag]; > + } This is pretty weird, that we reach over to get a CHBrowserView, just to getthe spell check doc tag. Is there a better way? > + NSSpellChecker* spChecker = [NSSpellChecker sharedSpellChecker]; > + NSString* cocoaWord = [NSString stringWith_nsAString:word]; > + NSRange spRange = [spChecker checkSpellingOfString:cocoaWord > + startingAt:0 > + language:nil > + wrap:NO > + inSpellDocumentWithTag:mSpellDocTag > + wordCount:NULL]; > + > + *isMisspelled = (spRange.length != 0); > + > + if (computeSuggestions) { > + [mCurGuesses release]; > + mCurGuesses = nil; > + mCurGuessIndex = -1; > + if (*isMisspelled) { > + mCurGuesses = [[spChecker guessesForWord:cocoaWord] retain]; > + mCurGuessIndex = 0; > + } > + } > + > + return NS_OK; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::CheckCurrentWordNoSuggest(const PRUnichar* prWord, > + PRBool* isMisspelled) > +{ > + nsresult rv = CheckWordInternal(nsDependentString(prWord), isMisspelled, NO); > + return rv; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::ReplaceWord(const PRUnichar* misspelledWord, > + const PRUnichar* replaceWord, > + PRBool allOccurrences) > +{ > + return NS_ERROR_NOT_IMPLEMENTED; > +} > + > +// this method IS used And the others aren't? > +NS_IMETHODIMP > +CHEditorSpellCheck::IgnoreWordAllOccurrences(const PRUnichar* prWord) > +{ > + if (mSpellDocTag < 0) > + NSLog(@"Whoa, IgnoreWordAllOccurrences: spell tag not set"); > + NSString* word = [NSString stringWith_nsAString:nsDependentString(prWord)]; > + [[NSSpellChecker sharedSpellChecker] ignoreWord:word inSpellDocumentWithTag:mSpellDocTag]; > + return NS_OK; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::GetPersonalDictionary() > +{ > + return NS_ERROR_NOT_IMPLEMENTED; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::GetPersonalDictionaryWord(PRUnichar **word) > +{ > + return NS_ERROR_NOT_IMPLEMENTED; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::AddWordToDictionary(const PRUnichar *word) > +{ > + return NS_ERROR_NOT_IMPLEMENTED; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::RemoveWordFromDictionary(const PRUnichar *word) > +{ > + return NS_ERROR_NOT_IMPLEMENTED; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::GetDictionaryList(PRUnichar ***dictionaryList, PRUint32 *count) > +{ > + return NS_ERROR_NOT_IMPLEMENTED; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::GetCurrentDictionary(PRUnichar** dictionary) > +{ > + return NS_ERROR_NOT_IMPLEMENTED; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::SetCurrentDictionary(const PRUnichar* dictionary) > +{ > + return NS_ERROR_NOT_IMPLEMENTED; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::UninitSpellChecker() > +{ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::SetFilter(nsITextServicesFilter* filter) > +{ > + mTxtSrvFilter = filter; > + return NS_OK; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::SetupDoc(PRUint32 *outBlockOffset) > +{ > + nsresult rv; > + > + nsITextServicesDocument::TSDBlockSelectionStatus blockStatus; > + PRInt32 selOffset; > + PRInt32 selLength; > + > + rv = mTsDoc->LastSelectedBlock(&blockStatus, &selOffset, &selLength); > + if (NS_SUCCEEDED(rv) && (blockStatus != nsITextServicesDocument::eBlockNotFound)) > + { > + switch (blockStatus) > + { > + case nsITextServicesDocument::eBlockOutside: // No TB in S, but found one before/after S. > + case nsITextServicesDocument::eBlockPartial: // S begins or ends in TB but extends outside of TB. > + // the TS doc points to the block we want. > + *outBlockOffset = selOffset + selLength; > + break; > + > + case nsITextServicesDocument::eBlockInside: // S extends beyond the start and end of TB. > + // we want the block after this one. > + rv = mTsDoc->NextBlock(); > + *outBlockOffset = 0; > + break; > + > + case nsITextServicesDocument::eBlockContains: // TB contains entire S. > + *outBlockOffset = selOffset + selLength; > + break; > + > + case nsITextServicesDocument::eBlockNotFound: // There is no text block (TB) in or before the selection (S). > + default: > + NS_NOTREACHED("Shouldn't ever get this status"); > + } > + } > + else //failed to get last sel block. Just start at beginning > + { > + rv = mTsDoc->FirstBlock(); > + *outBlockOffset = 0; > + } > + > + return rv; > +} > + > +NS_IMETHODIMP > +CHEditorSpellCheck::EnsureConverter() > +{ > + nsresult res = NS_OK; > + if (!mWordUtil) > + { > + nsCOMPtr<mozISpellI18NManager> manager(do_GetService("@mozilla.org/spellchecker/i18nmanager;1", &res)); > + if (manager && NS_SUCCEEDED(res)) > + { > + nsXPIDLString language; > + res = manager->GetUtil(language.get(), getter_AddRefs(mWordUtil)); > + } > + } > + return res; > +} General comments: I find it a little weird that both CHBrowserView and CHEditorSpellCheck are talking to the Cocoa spell checker. Can we push all the interaction with NSSpellChecker into one class? I'm concerned about adding all that code to CHBrowserView, especially since it sucks in so many non-embedding headers. And, we have to avoid including higher-level headers in lower-level files. I'd also be interested to hear about how the inline spell check "feels" compared to Cocoa's. Does it behave exactly the same way, or does it feel odd and clunky?

I was going to work on refactoring this so that it could be used by firefox as well and get it landed. Brett said the core stuff should be on the 18branch by the end of this week. I'll start with the patch julian posted and clean that up and report back.

julian, what was the reasoning behind replacing nsEditorSpellCheck rather than reimplementing just the code behind nsISpellChecker? Is it because you needed all that additional contextual info? I'm just worried about copying all that code, it's more to maintain and get out of sync.

(In reply to comment #74) > julian, what was the reasoning behind replacing nsEditorSpellCheck rather than > reimplementing just the code behind nsISpellChecker? Is it because you needed > all that additional contextual info? > > I'm just worried about copying all that code, it's more to maintain and get out > of sync. > Hoo boy, let's see. I think I was primarily concerned about all the dictionary stuff in the nsEditorSpellCheck. The CanSpellCheck method requires at least one dictionary, and I was kind of confused about how to handle it. However, I can't think of anything really compelling right now. I looked at the code and I'm not sure what contextual info I gained. I'll let you know if I think of anything else.

The latest comment on bug 302050 seems to indicate that the core patches from this bug should be going in to gecko 1.8.1. Is this the case?

Blocker as dep for blocker, and general goodness.

i have a re-implementation of the mozISpellCheckingEngine on top of NSSpellChecker. Much smaller patch, I'll include it a bit later once I get some of the context menu stuff hooked up for camino to make sure that part works as well.

brett: why does the mySpell spellchecker's makefile specifically disable static linkage? Any ideas? I can make it initially just a dylib, but at some point we'd like to make it static for Camino. Is it a license issue with mySpell, which the OSX impl wouldn't have, or is it for some other reason (like ease of disabling)?

I don't know why it's static. The license should be OK. All the spellchecking stuff is kind of separate, like an extension. I suspect it's that way just for disabling. I don't have any feeling of how it should be.

Mike, sweet. I'm eager to take a gander at that patch. =)

Here's the gecko part of the patch that replaces the mySpell engine with one based on NSSpellChecker. I'll attach the camino part separately once I get some of the dylib v. static issues dealt with.

here's the chunk for Camino that builds the suggestion context menu. Nib and project changes will come later, they're not too complicated. Note that this will only build on the trunk, there are api changes needed for the branch, but they're small.

Be sure you get the spelling actions into Edit->Spelling. I had code in the BroswserView to do this (didn't see any actions in the patch...) Make sure that the spellcheck wraps when you check manually; this didn't happen by default with the nsEditorSpellCheck stuff, I had to take care of that myself.

Comment on attachment 220399 [details] [diff] [review] replace mySpell engine * Ideally, you'd add the NSString category somewhere global and fix bug 334670. (We need mac string utilites in the tree.) Otherwise, please note in that bug that this category will need to be removed later. * There's some weird tab/space inconsistencies in the patch... :-) * Is the personal dictionary in OS X "global"? Will we need to hook up gecko's mozIPersonalDictionary to that later? Other than those things, r=me fwiw.

Comment on attachment 220401 [details] [diff] [review] Camino portion, sans nib and project changes (trunk) >@@ -3362,9 +3392,20 @@ > > const int kFrameRelatedItemsTag = 100; > const int kFrameInapplicableItemsTag = 101; > const int kSelectionRelatedItemsTag = 102; >+ const int kSpellingRelatedItemsTag = 103; > >+ if (showSpellingItems) >+ showSpellingItems = [self prepareSpellingSuggestionMenu:result tag:kSpellingRelatedItemsTag]; >+ >+ if (!showSpellingItems) { >+ // word spelled correctly or not applicable, remove all traces of spelling items >+ NSMenuItem* selectionItem; >+ while ((selectionItem = [result itemWithTag:kSpellingRelatedItemsTag]) != nil) >+ [result removeItem:selectionItem]; >+ } >+ How about: if (showSpellingItems) ... else { ... } Also, some weird whitespace inconsistencies in this patch too. 2 space vs 4 space indenting, and some tabs. >+- (BOOL)prepareSpellingSuggestionMenu:(NSMenu*)inMenu tag:(int)inTag >+{ >+ #define ENSURE_TRUE(x) if (!x) return NO; I'm not sure I like the temporary-#define stuff in this method and replaceMispelledWord:. For this one, perhaps you can define it globally (with the bool return), and for the #define in replaceMispelledWord:, define ENSURE_EXISTS() globally? ... >+ nsCOMPtr<nsISelection> selection; >+ editor->GetSelection(getter_AddRefs(selection)); >+ ENSURE_TRUE(selection); >+ PRInt32 anchorOffset; >+ selection->GetAnchorOffset(&anchorOffset); >+ nsCOMPtr<nsIDOMNode> anchorNode; >+ selection->GetAnchorNode(getter_AddRefs(anchorNode)); This chunk (+ the fetching of the spellchecker) is also done in replaceMispelledWord:, so I think it deserves a GeckoUtils.h helper. >+ nsCOMPtr<nsIDOMRange> mispelledRange; >+ inlineChecker->GetMispelledWord(anchorNode, (long)anchorOffset, getter_AddRefs(mispelledRange)); Wanna move this down a bit, where the if (mispelledRange) check is? >+ nsCOMPtr<nsIEditorSpellCheck> spellCheck; >+ inlineChecker->GetSpellChecker(getter_AddRefs(spellCheck)); >+ ENSURE_TRUE(spellCheck); * Is the spellchecker something that is per <editor>? If we're fetching a service, we could cache it. r=me

How about: if (showSpellingItems) ... else { ... } Re-read the code, the point is that |showSpellingItems| can change and we need to re-test it. It's not just 1 check that can do with a single if-then-else.

(In reply to comment #87) > How about: > > if (showSpellingItems) > ... > else { > ... > } > > Re-read the code, the point is that |showSpellingItems| can change and we need > to re-test it. It's not just 1 check that can do with a single if-then-else. > Ah, now I see that, thanks.

Comment on attachment 220399 [details] [diff] [review] replace mySpell engine >Index: osxspell/src/Makefile.in >+MOZILLA_INTERNAL_API = 1 Do you need that? >Index: osxspell/src/mozOSXSpell.mm >+// SetPersonalDictionary >+// >+// Hold onto the personal dictionare we're given. Typo >+NS_IMETHODIMP mozOSXSpell::Check(const PRUnichar *aWord, PRBool *aResult) >+{ >+ NS_ENSURE_ARG_POINTER(aWord); >+ NS_ENSURE_ARG_POINTER(aResult); >+ *aResult = PR_FALSE; Detab >Index: osxspell/src/mozOSXSpellFactory.cpp >+ * The Initial Developer of the Original Code is Mike Pinkerton. We usually put Google Inc. there. >+ * Contributor(s): Mike Pinkerton <mikepinkerton@mac.com> with your name on a new line? I don't care, I think you know how I feel about licensing...

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

Comment on attachment 220399 [details] [diff] [review] replace mySpell engine >+CPPSRCS = \ >+ mozOSXSpellFactory.cpp \ >+ $(NULL) >+ >+CMMSRCS = \ >+ mozOSXSpell.mm \ >+ $(NULL) I don't suppose it would be possible to just combine these files into one .mm file? >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ osxspell/src/mozOSXSpell.h 1 May 2006 16:18:07 -0000 >+class mozOSXSpell : public mozISpellCheckingEngine >+{ >+public: >+ NS_DECL_ISUPPORTS >+ NS_DECL_MOZISPELLCHECKINGENGINE >+ >+ mozOSXSpell(); >+ virtual ~mozOSXSpell(); >+ >+protected: Do you intend to subclass this? If not, make the destructor private and nonvirtual, and change protected: to private:. >+ >+ // NSSpellChecker provides the ability to add words to the local dictionary, >+ // but it's much easier to let the rest of Gecko handle that via the personal >+ // dictionary given to us and just be ignorant about new words. Tabs. >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ osxspell/src/mozOSXSpellFactory.cpp 1 May 2006 16:18:07 -0000 >+static nsModuleComponentInfo components[] = { >+ { NULL, MOZ_OSXSPELL_CID, MOZ_OSXSPELL_CONTRACTID, mozOSXSpellConstructor } It wouldn't hurt to have a name here instead of NULL, just for ease of debugging. Looks good otherwise.

trunk patch checked in there, should be available for firefox, i'll work on adding it to the camino build in the next step. I also need branch approval for that patch, as it seems to compile fine there.

Comment on attachment 224315 [details] [diff] [review] fixes all issues raised during review, landed on trunk requesting branch approval

landed on the branch (only for camino since it can't work with carbon firefox), set the pref on both trunk and branch to enable spellcheck for camino. i still have to land the patch that hooks up the context menu for suggestions, but that doesn't require project changes and can be done easily (i only have one mac that has XCode2.0 on it).

Could this have caused bug 341980 (Camino) and bug 341317 (Firefox/core) ? (tested with trunk builds only)

This doesn't appear to actually *work* on the branch (at least, no little red dots are drawn), or at least not on 10.3.9. Checked a clean profile and the latest branch nightly....

i can't see how it could have caused that regression, these patches do no drawing of their own. you're right, the branch doesn't work. we're missing spellcheck.xpt which i guess i biffed in the static target. I don't have access to an XCode2.0 machine this week in mtnView, so it will have to wait a week, unless mento can get to it.

The project file is fine, we've got spellchecker.xpt in both targets. The problem is that osxspell is in use instead of myspell, because the Makefile change wasn't made on the 1.8[.1] branch. Because myspell isn't finding its dictionaries (they were correctly not added to the project file), spell checking is disabled. This patch turns on osxspell when Cocoa widgets are in use, as on the trunk.

Comment on attachment 226615 [details] [diff] [review] Enable osxspell on 1.8 branch (checked in) wtf. i wonder how this got missed. thanks for catching it. r/sr=pink

Comment on attachment 226615 [details] [diff] [review] Enable osxspell on 1.8 branch (checked in) Checked in on MOZILLA_1_8_BRANCH before 1.8.1b1.

In the latest nightly (26-06-06) Camino underlines spellings when they are incorrect (which is excellent btw.) however it doesn't seem to be possible to bring up a list of suggested spelling corrections as in other programs.

(In reply to comment #102) > it doesn't seem to be possible to bring up a list of suggested spelling corrections as in other programs. Mike said in comment #95 that he _did not_ check in support for the contextual menu items, etc. etc. etc.

Is it just me, or does the spell checker(or at least the red underliner) not work in rich text boxes, such as those in GMail, Wordpress, and many forums? Those are generally the ones I need spell checking the most, and the ones that specifically don't work right in Safari(no rich text options).

(In reply to comment #104) > Is it just me, or does the spell checker(or at least the red underliner) not > work in rich text boxes, such as those in GMail, Wordpress, and many forums? That's bug 333038.

landed the camino part on the trunk, still doing branch builds to get that ready (on my dog-slow pbg4)

context menu suggestions landed on branch as well. enjoy.

Verified with latest trunk nightly.

I'm a bit confused now if this is a Camino-only bug or if morphed into a core bug, affecting Firefox and Tb as well? Anyway, my current OS X install doesn't provide a Norwegian spell checker. For a while i could add the necessary spell.xpi files to add that functionality. However, since we hooked in to the the spell checkers provided by OS X, i can't seem to use the spell check functionality by the xpi files anymore. They got installed successfully but i'm not able to activate them. Am i missing something, or should i file a spin-off bug?

(In reply to comment #110) > I'm a bit confused now if this is a Camino-only bug or if morphed into a core > bug, affecting Firefox and Tb as well? It's a Camino bug that happens to have required a "bit" of Core work. > Anyway, my current OS X install doesn't provide a Norwegian spell checker. ... > Am i missing something, or should i file a spin-off bug? No, we only use the Mac OS X dictionaries in Camino. You should check out cocoAspell <http://cocoaspell.leuski.net/>, which apparently plugs in to the Mac OS X spelling system and offers a wider selection of languages, including Norwegian. (For Hebrew speakers, there's Hebrew Spelling Service <http://www.mitzpettel.com/software/hspell.php> that does the same thing and is tweaked for Hebrew.)

Firefox *trunk* uses the OS X spellchecker as well, and would have this problem.

(In reply to comment #112) > Firefox *trunk* uses the OS X spellchecker as well, and would have this > problem. > I've filed bug 345859 for this.

In case anyone goes looking for trouble in fox-trunk, note that carbonfox uses myspell, while cocoafox uses osxspell. The trunk builds have not been switched to cocoafox yet. All nightly foxes and birds with spell checking are still using myspell. Sincerely, The zookeeper