The canonical examples for this are the GMail subject line field (which you do want to spell-check, even though it's a single-line text area that isn't checked by default), or the template editing field in Wordpress (which we probably don't want to spell-check).

This seems ripe for some kind of css extension: -moz-spellcheck : value with values of "none" No spell checking on this field "default" Follow default behavior (spell check on multi-line fields, none on single line fields) "always" Spell check this field

Here's the current thinking: We use the accept= attribute which already exists in the spec. If this attribute is not present (the normal case) we do the default, which currently is spellcheck textareas and not input elements. If this element is present and contains "text/plain", then we always spellcheck even if we wouldn't have by default. If the element is present but does NOT contain "text/plain", we DON'T spellcheck, even if we would have by default. This attribute is a comma separated list of MIME types, so it could also contain other content types. The algorithm for parsing the accept attribute would be: 1. Separate on commas (input is ASCII) 2. Trim text for each item after the semicolon. 3. Trim whitespace from the result. 4. Case-insensetive ASCII compare agains "text/plain".

That sounds pretty cool. It's basically a simple extension of the "accept" attribute from HTML4 (where it applies to <input type=file>) and HTML5 (where it applies to <textarea> as well). I'll post to whatwg about this to see if anyone sees any problems with it, but in general I'd say this is great and we should go with it.

Is matching the string literal "text/plain" the best solution? In principle a field could specify (say) text/html (e.g. a weblog application with a title field that allows markup). So maybe support anything that starts "text/" and any other known types? Or would that be better left for some distant future age in which the spellcheck knows to ignore markup in text/html content ;) Even without that, I think this is an excellent idea.

a) Currently, no user agent uses accept on input fields so it doesn't matter. b) You could always say "text/html,text/plain" which would satisfy your case. The blog can accept either html or plain text.

You wouldn't want to do our current spell checking with text/html input. You'd need to handle tags, etc, with that type.

I like the idea of adding spell checking to text fields and textareas, but I don't particularly the idea of using the accept attribute as the only switch to turn it on or off. In fact, I don't think the author of the page should have any direct control over this feature. There are many cases where text/plain would be the sematically correct MIME type for a field, yet still not desire spell checking. There are also cases where text/plain wouldn't be correct, but intelligent spell checking would be appropriate. The following are cases where plain text would be entered, but spell checking would not be desired: * People's Names * Organisation Names * Usernames * Addresses (streets, suburbs, cities, countries, etc.) * E-Mail addresses * URIs * Phone numbers * Credit Card Numbers The following are cases where text/plain wouldn't be correct, but intelligent spell checking would be useful: * text/html * application/xml and other XML MIME types * BBCode, Markdown, Textile, etc. (there is no MIME type for these, but they're a commonly used for blog comment forms) The following are cases where ordinary spell checking would be a bad idea, but syntax checking may still be useful: * CSS * JavaScript For everything else, spell checking should just be turned on by default. This would include things like: * Search forms * Subject lines * Any textarea that can't be detected as accepting a specific type of input, like text/html, XML, BBCode, CSS, etc. For some of those, like phone numbers, card nubmers, etc. it probably wouldn't matter if spell checking was on or not because sequences of numbers should never be marked as an error anyway. For e-mail address and URIs, Web Forms 2.0 will be introducing type="email" and type="url", which can be used to turn off spell checking in those controls, and even provide more intelligent autocomplete features. However, for existing forms where type="text" is already used for those 2 cases, as well as names, usernames and addresses, more logic would be required to intelligently detect such cases and turn spell checking off. Existing browsers already have password managers that detect usernames fairly accurately, similar logic could be used to detect them for the purpose of disabling spell checking. I outlined this idea on the WHATWG mailing list. http://listserver.dreamhost.com/pipermail/whatwg-whatwg.org/2006-June/006633.html There are a variety of methods that could be used by some form of Artificial Intelligence (AI) to detect the above cases. Although much more research would be needed, here's a brief list I could come up with: * Common attribute values like name="first-name", "address1", "line1", "suburb", etc. * Common label text like: <label>Organisation</label>, First Name, Last Name, Email address, Search, etc. * accept attributes for the MIME type related ones * If the text the user enters looks like a URI or an e-mail address, then don't mark it as an error (Thunderbird's Compose window already handles this fairly well). If a user constantly enables or disables spell checking for particular fields, the UA could remember the decisions and possibly even learn and make decisions about other forms. I have another idea to make the spell checking more intelligent, especially for cases where the user is entering a lot of terms that aren't in the browser's dictionary (e.g. acroymns and terms like CSS, JavaScript, Ajax, Hijax and other common web developer terms). The browser could learn those words either based on the fact the user types them frequently, or that they're frequently used elsehere in the pages read by the user (e.g. A blog entry about Ajax/Hijax would commonly use those terms, and so it could be expected that the user would enter them too. In such cases, it would make sense the the spell checker either doesn't bother marking them as errors, or marks them as warnings instead (e.g. a yellow underline, instead of red). I realise that would be a very advanced and difficult feature to implement and I wouldn't expect it to be implemented right away, it's just something to think about for the future.

The WHATWG has had some discussions on the proper way to implement this. It looks like we will just add a "spellcheck" attribute to control spellchecking. In the future, perhaps we can use the "accept" attribute to control the spellchecking mode. This attribute may or may not be implemented in Firefox 2 depending on whether we find somebody to do the work. Remembering the spellcheck state will be implementable in Firefox 3 where we can use the annotation service. This is not practical for Forefox 2.

http://www.damowmow.com/playground/spellcheck.txt is the candidate spec we're looking at now.

Setting blocking1.8.1?. Adding swag, which really is a guess in this case as I've never looked at any of this stuff before.

Given that we don't yet support contentEditable, should we really be supporting the full DOM stuff in that spec, or only DOM changes to input and textarea? The code you should start looking at is: content/html/content/src/nsHTMLInputElement.cpp content/html/content/src/nsHTMLTextAreaElement.cpp dom/public/idl/html/nsIDOM(either HTMLElement or HTMLInputElement and HTMLTextAreaElement).idl [all interfaces must be considered 1.8-branch-frozen; many are trunk frozen]

Er, that should be: dom/public/idl/html/nsIDOMNS(...) (note added "NS")

Brett notes this should be on body elements too, for designmode, I guess. dbaron notes the right way to deal with frozen branch interfaces is to make new ones like *_MOZILLA_1_8_BRANCH.idl or whatever. (I think.)

interface *_MOZILLA_1_8_BRANCH can be in *.idl; you don't need to (and in fact should not) create *_MOZILLA_1_8_BRANCH.idl .

OK, I'm on the run, but here's a first cut patch. This isn't fully tested, but seems to do the right thing in the cases I've tried. Much more info coming next week.

Updated version. Gets spellchecking set up properly for frames whose content isn't hooked to them right away; I saw this on the testcase for bug 339434. The solution here is a gruesome hack, but it's the best thing I can come up with after a couple days of trying :( Still not done: * Test and handle designmode properly * More sanity checks about whether the thing we're spellchecking still exists (bug 339434, bug 342748) * Default to not spell checking XUL/chrome editors? (bug 336799)

OK, this patch leaves the pref values meaning the same thing they used to (which the prior two patches didn't do), and fixes bug 3336799, bug 339434, and bug 342748 as well. (I could probably fork those other fixes out into their own patch; I was just working on that code already, so...) In general, the architecture of this is: * Content and DOM setters update a tristate member on HTML elements, and also ping the associated editor to update itself * Content and DOM getters return member if set, ancester member if that's set, or UA default * UA default is to not check elements which aren't editable, are chrome, are readonly, or are passwords. Aditionally, the user can toggle layout.spellcheckDefault between checking only multi-line inputs or single- and multi-line inputs; that is reflected here too. * UI checkboxes talk directly to the editor and bypass the element * Editor will enable spellchecking if the user has forced it on, or if the element says it should be on and the user has not forced it off (either locally or globally through layout.spellcheckDefault). Note that checking the element (to get the UA default) required some irritating changes to nsIContent et. al. * When anything about the editor changes which might change the spellchecking default state, we resync immediately. However, there is no immediate resync on updates of preferences, so changing layout.spellcheckDefault will only be reflected in subsequently-created editors. * Once the editor has been PreDestory()ed, it is no longer possible to enable or get the spellchecker on the object There are lots of other little details in this crazy patch. Questions welcome, but usually if something looks architecturally bizarre, it's because it needed to be in order to satisfy part of the unusually-complex spellchecking spec :(

All specs are unusually complex :-)

Why are the content attribute and DOM attribute only enabled on HTML elements? Shouldn't they be supported on all XML elements?

+ enum Tristate { + NOT_SET = -1, + FALSE = 0, + TRUE = 1 + } mSpellcheck; FALSE and TRUE seem like really bad names. I think they're #defines on Windows. How about TRI_UNSET, TRI_TRUE, TRI_FALSE? Also, is it really necessary to declare two of these? That seems really bad. Can't you put it in nsIEditor.h or something? + /** + * Returns the default spellchecking state on this node, given an editor. + */ + virtual PRBool GetSpellcheckWithEditor(nsIEditor* aEditor) const = 0; I would really like to avoid adding this. If we can have one implementation for all nodes, then we won't need this. If we must special-case for HTML, you can test by QIing to nsIDOMHTMLElement. + /** + * Locates the nsIEditor "controlled by" this node. In general this is + * equivalent to GetEditorInternal(), but it may do something different for + * designmode, contenteditable, etc. + */ + virtual nsresult GetControlledEditor(nsIEditor** aEditor); Need more explanation here. I think GetAssociatedEditor is a better name. You should make it clear what it's behaviour should be for contenteditable and designmode. Why bother storing the mSpellcheck tristate? You could just query the attribute every time. Is there a reason not to? You can use FindAttrValueIn, it should be efficient enough. +nsresult +nsHTMLBodyElement::GetControlledEditor(nsIEditor** aEditor) I don't think this needs to be here. Why can't nsGeneric(HTML)Element check if it's the root element and if it is, check designmode? I assume this is supposed to affect designmode. If so, then it clearly doesn't implement "spellcheck" on child content. I assume that's something for followup work.

(In reply to comment #19) > Why are the content attribute and DOM attribute only enabled on HTML elements? > Shouldn't they be supported on all XML elements? One issue is that we need an nsIDOMNSElement to put generic element extensions on. I'm actually creating this in bug 174397. I don't mind if you beat me to it :-).

(In reply to comment #19) > Why are the content attribute and DOM attribute only enabled on HTML elements? > Shouldn't they be supported on all XML elements? I don't know. My spec says HTML, so that's what I was implementing. I'm 0% familiar with XML. (In reply to comment #20) > + enum Tristate { > + NOT_SET = -1, > + FALSE = 0, > + TRUE = 1 > + } mSpellcheck; > > FALSE and TRUE seem like really bad names. I think they're #defines on Windows. > > How about TRI_UNSET, TRI_TRUE, TRI_FALSE? > > Also, is it really necessary to declare two of these? That seems really bad. > Can't you put it in nsIEditor.h or something? Maybe, but then nsGenericElement.h (I think... from memory) would need to pull in that header. Maybe that's not bad. I asked around if we had a generic tristate type sitting somewhere, but no go... > + /** > + * Returns the default spellchecking state on this node, given an editor. > + */ > + virtual PRBool GetSpellcheckWithEditor(nsIEditor* aEditor) const = 0; > > I would really like to avoid adding this. If we can have one implementation for > all nodes, then we won't need this. If we must special-case for HTML, you can > test by QIing to nsIDOMHTMLElement. Well, we'll still need it, we'll just need it to be implemented up in nsIContent or somewhere instead of down where it is in genericElement. The point is it has to be in some abstract class accessible from inside the Editor :(. Unless I'm missing the point of what you're saying... > + /** > + * Locates the nsIEditor "controlled by" this node. In general this is > + * equivalent to GetEditorInternal(), but it may do something different for > + * designmode, contenteditable, etc. > + */ > + virtual nsresult GetControlledEditor(nsIEditor** aEditor); > > Need more explanation here. I think GetAssociatedEditor is a better name. You > should make it clear what it's behaviour should be for contenteditable and > designmode. GetAssociatedEditor() would be fine. I don't know what the behavior should be for contenteditable, and wasn't really worried about that since it's not yet implemented; I mostly wanted it as grepbait for future contenteditable implementers. I can probably word stuff in a more technically accurate way, I dunno if that will be helpful to those reading the .h file. I'll give it a shot. > Why bother storing the mSpellcheck tristate? You could just query the attribute > every time. Is there a reason not to? You can use FindAttrValueIn, it should be > efficient enough. Yes. There's an irritating bit in the spec where the content attribute can be set to an invalid string and the DOM attribute must remain at whatever its previous value was. So if we set content to "on" and then " off", the DOM getter must still return true, and the content getter must return " off". :( > +nsresult > +nsHTMLBodyElement::GetControlledEditor(nsIEditor** aEditor) > > I don't think this needs to be here. Why can't nsGeneric(HTML)Element check if > it's the root element and if it is, check designmode? It probably could. I was purposefully trying to use BODY rather than the root, to follow the spec closely, but doing it your way would probably better parallel what I did in the editor. Also I'm always less confused by overriding behavior in virtual functions than conditionals in a single parent class :) > I assume this is supposed to affect designmode. If so, then it clearly doesn't > implement "spellcheck" on child content. I assume that's something for followup > work. If we only use one editor for the whole document when it's in designmode, then no, I can't support spellcheck on child content... that would require more sophisticated support in the editor. If there's more than one editor, things should "just work". I'm willing to live with this limitation, seems like we discussed it briefly on IRC a few days back.

(In reply to comment #22) > I don't know. My spec says HTML, so that's what I was implementing. I'm 0% > familiar with XML. Can you get an answer from Hixie on this? > Maybe, but then nsGenericElement.h (I think... from memory) would need to pull > in that header. Maybe that's not bad. I asked around if we had a generic > tristate type sitting somewhere, but no go... Another place you could put it would be nsIContent.h. That's probably better. > > + /** > > + * Returns the default spellchecking state on this node, given an editor. > > + */ > > + virtual PRBool GetSpellcheckWithEditor(nsIEditor* aEditor) const = 0; > > > > I would really like to avoid adding this. If we can have one implementation for > > all nodes, then we won't need this. If we must special-case for HTML, you can > > test by QIing to nsIDOMHTMLElement. > > Well, we'll still need it, we'll just need it to be implemented up in > nsIContent or somewhere instead of down where it is in genericElement. The > point is it has to be in some abstract class accessible from inside the Editor > :(. Unless I'm missing the point of what you're saying... I want to avoid adding the new virtual function which bloats the vtable of all element classes (which there are a lot of). There is really only one implementation, in nsGenericHTMLElement, and as far as I can tell, only one caller. So the caller could just do its work inline, QIing to HTMLElement first. > > Why bother storing the mSpellcheck tristate? You could just query the attribute > > every time. Is there a reason not to? You can use FindAttrValueIn, it should be > > efficient enough. > > Yes. There's an irritating bit in the spec where the content attribute can be > set to an invalid string and the DOM attribute must remain at whatever its > previous value was. So if we set content to "on" and then " off", the DOM > getter must still return true, and the content getter must return " off". :( I don't see where it says that. > > +nsresult > > +nsHTMLBodyElement::GetControlledEditor(nsIEditor** aEditor) > > > > I don't think this needs to be here. Why can't nsGeneric(HTML)Element check if > > it's the root element and if it is, check designmode? > > It probably could. I was purposefully trying to use BODY rather than the root, > to follow the spec closely, The spec doesn't mention BODY. > behavior in virtual functions than conditionals in a single parent class :) In this case, you're already traversing up the parent chain looking for an element with spellcheck set. You should already be checking for the case where there is no parent! > > I assume this is supposed to affect designmode. If so, then it clearly doesn't > > implement "spellcheck" on child content. I assume that's something for followup > > work. > > If we only use one editor for the whole document when it's in designmode, then > no, I can't support spellcheck on child content... that would require more > sophisticated support in the editor. If there's more than one editor, things > should "just work". I'm willing to live with this limitation, seems like we > discussed it briefly on IRC a few days back. Yep.

(In reply to comment #23) > > > + /** > > > + * Returns the default spellchecking state on this node, given an editor. > > > + */ > > > + virtual PRBool GetSpellcheckWithEditor(nsIEditor* aEditor) const = 0; > > > > > > I would really like to avoid adding this. If we can have one implementation for > > > all nodes, then we won't need this. If we must special-case for HTML, you can > > > test by QIing to nsIDOMHTMLElement. > > > > Well, we'll still need it, we'll just need it to be implemented up in > > nsIContent or somewhere instead of down where it is in genericElement. The > > point is it has to be in some abstract class accessible from inside the Editor > > :(. Unless I'm missing the point of what you're saying... > > I want to avoid adding the new virtual function which bloats the vtable of all > element classes (which there are a lot of). There is really only one > implementation, in nsGenericHTMLElement, and as far as I can tell, only one > caller. So the caller could just do its work inline, QIing to HTMLElement > first. There are callers in both nsGenericHTMLElement and nsEditor, and the call from nsEditor is the reason I needed to add this to nsIContent in the first place. > > > Why bother storing the mSpellcheck tristate? You could just query the attribute > > > every time. Is there a reason not to? You can use FindAttrValueIn, it should be > > > efficient enough. > > > > Yes. There's an irritating bit in the spec where the content attribute can be > > set to an invalid string and the DOM attribute must remain at whatever its > > previous value was. So if we set content to "on" and then " off", the DOM > > getter must still return true, and the content getter must return " off". :( > > I don't see where it says that. Huh, I just checked again, and Hixie's changed the spec and removed that part. That would probably make my life easier. I'll get in touch with him about that to make sure it was intentional. > > > +nsresult > > > +nsHTMLBodyElement::GetControlledEditor(nsIEditor** aEditor) > > > > > > I don't think this needs to be here. Why can't nsGeneric(HTML)Element check if > > > it's the root element and if it is, check designmode? > > > > It probably could. I was purposefully trying to use BODY rather than the root, > > to follow the spec closely, > > The spec doesn't mention BODY. I should have said "IRC directions from Hixie" rather than "spec" in this case. It goes along with the "what do we do for XML" question, I'll ask about this too. > > behavior in virtual functions than conditionals in a single parent class :) > > In this case, you're already traversing up the parent chain looking for an > element with spellcheck set. You should already be checking for the case where > there is no parent! Don't BODY elements have a parent node? I wasn't sure GetRoot() (or whatever) was quite the same as "somewhere GetParent() == nsnull". Aren't those distinct questions?

(In reply to comment #23) > (In reply to comment #22) > > I don't know. My spec says HTML, so that's what I was implementing. I'm 0% > > familiar with XML. > > Can you get an answer from Hixie on this? <Hixie> to answer the HTML/XML question, "because this is an HTML spec and not a spec of every XML vocabulary on the planet" <Hixie> they should be on HTML and XHTML elements and that's all <Hixie> just like contenteditable (In reply to comment #24) > > > Yes. There's an irritating bit in the spec where the content attribute can be > > > set to an invalid string and the DOM attribute must remain at whatever its > > > previous value was. So if we set content to "on" and then " off", the DOM > > > getter must still return true, and the content getter must return " off". :( > > > > I don't see where it says that. > > Huh, I just checked again, and Hixie's changed the spec and removed that part. > That would probably make my life easier. I'll get in touch with him about that > to make sure it was intentional. <Hixie> yes, i removed the "remember the state" thing, you asked me to :-) ...so I'll recode that part for clarity. That will also eliminate one of the tristate definitions, yay. > > > > +nsresult > > > > +nsHTMLBodyElement::GetControlledEditor(nsIEditor** aEditor) > > > > > > > > I don't think this needs to be here. Why can't nsGeneric(HTML)Element check if > > > > it's the root element and if it is, check designmode? > > > > > > It probably could. I was purposefully trying to use BODY rather than the root, > > > to follow the spec closely, > > > > The spec doesn't mention BODY. > > I should have said "IRC directions from Hixie" rather than "spec" in this case. > It goes along with the "what do we do for XML" question, I'll ask about this > too. <Hixie> designMode is exactly equivalent to setting contenteditable on the body element <Hixie> that's why you should use the body element So whatever we do should be exactly the same as whatever we'll do when we support contenteditable. Which I don't know what it is :(

> There are callers in both nsGenericHTMLElement and nsEditor, and the call from > nsEditor is the reason I needed to add this to nsIContent in the first place. Hmm, there should be a way ... Perhaps you could make it a method in nsIEditor instead, that would reduce bloat because there's only a few editor implementations. > Don't BODY elements have a parent node? Generally, but someone might be able to break this using the DOM. Unless the spec forces us to treat BODY specially, it is cleaner to not make assumptions about document structure and look for designmode when we're at the root element, the element that has no parent. But, since we apparently do have to treat BODY speciallly, I guess what you have is OK. I'll think more about this the next time around. <Hixie> they should be on HTML and XHTML elements and that's all OK

(In reply to comment #26) > > There are callers in both nsGenericHTMLElement and nsEditor, and the call from > > nsEditor is the reason I needed to add this to nsIContent in the first place. > > Hmm, there should be a way ... Perhaps you could make it a method in nsIEditor > instead, that would reduce bloat because there's only a few editor > implementations. One problem with that is that then either: (1) We move the entire routine into the editor, and you can't use the DOM getter at all until we have an editor set up and hooked to the content node, which may be too late (I'm not sure), OR (2) We only move the editor-specific portions in, in which case the editor _still_ needs to call into the DOM to get the UA defaults, and THEN calls an additional function inside the editor, so we've made things worse rather than better. darin and I tried for a while to come up with a good solution... I also asked dbaron on IRC. So far none of us have a better answer, but I'd still love to find one.

I'm not worried about making additional method calls. I'm worried about extending the vtable of a class that has a large number of implementations, and having to add a few dummy implementations because it only applies to HTML elements. How about putting GetSpellcheckWithEditor onto nsIDocument? Not many implementations of that.

This patch should address all the review feedback from the first patch. Specifically, I made the following changes: * Moved GetSpellcheckWithEditor() from nsIContent (and all its subclasses) to nsIDocument and renamed it to GetSpellcheckForElement(). This should reduce the vtable impact. * Constified IsChromeDoc() because it should have been in the first place, and this lets me keep GetSpellcheckForElement() const * Removed nsGenericHTMLElement::Tristate enum * Simplified nsGenericHTMLElement DOM getter function to just check the content attribute, to match the simplification in the spec since my first implementation * Renamed GetControlledEditor() to GetAssociatedEditor() and modified comment to be a tiny bit more descriptive * Changed nsEditor::Tristate enum values to better names, for consistency with other enums and to avoid possible clashes with Windows #defines * Included mozInlineSpellChecker.cpp, which somehow didn't get included in the previous patch Also, part of this patch is also available as a separate patch for bug 339434, and this has been updated to the latest version of that code (which clears nsEditor::mInlineSpellChecker on nsEditor::PreDestroy()).

uriloader \ + editor \ Need tabs here + return !(flags & (((spellcheckLevel == SpellcheckAll) ? + 0 : nsIPlaintextEditor::eEditorSingleLineMask) | + nsIPlaintextEditor::eEditorPasswordMask | + nsIPlaintextEditor::eEditorReadonlyMask)); This is really confusing. Please break it up. Preferably switch on spellcheckLevel. + // Return member state if it's been set + nsAutoString target; + GetAttr(kNameSpaceID_None, nsHTMLAtoms::spellcheck, target); + if (target.EqualsLiteral("true")) { + *aEnable = PR_TRUE; + return PR_TRUE; + } + if (target.EqualsLiteral("false")) { + *aEnable = PR_FALSE; + return PR_TRUE; + } Use FindAttrValueIn + // Otherwise, walk up the parents + nsIContent* parent = GetParent(); + if (!parent) { + return PR_FALSE; + } + + nsGenericHTMLElement* parentElement = FromContent(parent); + return parentElement ? parentElement->GetSpellcheckState(aEnable) : PR_FALSE; So HTML children with non-HTML parent with HTML ancestors will return false. Really I think in that situation we should walk upwards looking for the innermost HTML ancestor. +nsresult +nsHTMLBodyElement::GetAssociatedEditor(nsIEditor** aEditor) You should check that this is the actual body of the document. QI the document to nsIHTMLDocument and call GetBody and compare using COMEquals or whatever that helper is called. + if (!mDidPreDestroy) { Just return if (mDidPreDestroy) so you don't have to indent stuff. otherwise looks good!

So will users be able to override this and disable spellcheck even if the site is trying to force it on (or enable it even if the site is trying to force it off)? And will we remember said user overrides?

(In reply to comment #31) > So will users be able to override this and disable spellcheck even if the site > is trying to force it on (or enable it even if the site is trying to force it > off)? Yes. > And will we remember said user overrides? No.

It's probably worth remembering them, since we're basically letting the site override user spell-checking prefs here... Otherwise I'll end up getting annoyed with spellchecking every time I visit a site that wants to foist it on me, say. ;)

(In reply to comment #33) > It's probably worth remembering them, since we're basically letting the site > override user spell-checking prefs here... Otherwise I'll end up getting > annoyed with spellchecking every time I visit a site that wants to foist it on > me, say. ;) Associating manual enable/disable with particular form fields across page loads is a problem. Hmm, perhaps we could leverage whatever the form filler does here?

(In reply to comment #33) > It's probably worth remembering them, since we're basically letting the site > override user spell-checking prefs here... Otherwise I'll end up getting > annoyed with spellchecking every time I visit a site that wants to foist it on > me, say. ;) That seems worth remembering (somehow), but also something that should be in a followup bug. (We do allow users to override spellchecking globally as a pref, so obviously we remember that.) This seems low-priority to me; the likelihood seems low that I'll find spellchecking useful enough to want it on, but so annoyed with it on a particular site that I'll want it off there, even though the site author has it on, AND yet still like that site enough to visit it frequently. It definitely shouldn't block the release (or any subsequent release), is what I'm getting at, as long as we give users both the global override and the non-remembered per-site override. But yes, nice to have.

Saving spellcheck state was going to be the premiere application for the annotation service, which was pushed to Firefox 3.

(In reply to comment #35) > That seems worth remembering (somehow), but also something that should be in a > followup bug. (We do allow users to override spellchecking globally as a > pref, so obviously we remember that.) We do? Seems to me the pref is overridden by the spellcheck attribute.

(In reply to comment #37) > > (We do allow users to override spellchecking globally as a > > pref, so obviously we remember that.) > > We do? Seems to me the pref is overridden by the spellcheck attribute. If you set layout.spellcheckDefault to 0, nothing will be spellchecked no matter what the attributes say. If you set it to 1 or 2, then the attributes take effect. The only difference between 1 and 2 is that in 2 the UA defaults to spellchecking single-line inputs and in 1 it does not. But an attribute will override these.

Right, gotcha. Will there be UI for this?

(In reply to comment #39) > Right, gotcha. Will there be UI for this? Yes, there will be a checkbox to toggle it on and off. We are likely not going to expose the ability to turn it to "2".

Ah, ok. As long as the user can disable spellcheck altogether, I'm happy.

This patch should address all roc's previous comments. It also no longer includes the fix for bug 339434 as that has been checked in already.

It'll probably take me about a week and a half to get to this...

Comment on attachment 230200 [details] [diff] [review] candidate patch v3 (In reply to comment #43) > It'll probably take me about a week and a half to get to this... Hmmm... then I need to pick a different sr?, since this is must-fix for B2 and it needs to get onto trunk fairly soon in that case. I'm picking jst, but if any other content owners/peers have a better idea feel free to edit the flags, give sr, etc...

Comment on attachment 230200 [details] [diff] [review] candidate patch v3 >+nsGenericHTMLElement::GetSpellcheck(PRBool* aSpellcheck) >+{ >+ nsIDocument* ownerDoc = GetOwnerDoc(); >+ if (!ownerDoc) { >+ return NS_ERROR_NOT_AVAILABLE; >+ } >+ >+ nsCOMPtr<nsIEditor> editor = nsnull; >+ GetAssociatedEditor(getter_AddRefs(editor)); You might want to check for error here. > nsresult > nsGenericHTMLElement::AfterSetAttr(PRInt32 aNamespaceID, nsIAtom* aName, > const nsAString* aValue, PRBool aNotify) > { >- if (aNamespaceID == kNameSpaceID_None && IsEventName(aName) && aValue) { >- nsresult rv = AddScriptEventListener(aName, *aValue); >- NS_ENSURE_SUCCESS(rv, rv); >+ if (aNamespaceID == kNameSpaceID_None) { >+ if (IsEventName(aName) && aValue) { >+ nsresult rv = AddScriptEventListener(aName, *aValue); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } else if (aName == nsHTMLAtoms::spellcheck) { >+ nsCOMPtr<nsIEditor> editor = nsnull; >+ nsresult rv = GetAssociatedEditor(getter_AddRefs(editor)); Since you declare it in two places, I would declare rv at the top of the function. >+nsHTMLBodyElement::GetAssociatedEditor(nsIEditor** aEditor) ... >+ nsCOMPtr<nsISupports> container = presContext->GetContainer(); >+ nsCOMPtr<nsIEditorDocShell> editorDocShell = do_QueryInterface(container); I think you can just do do_QueryInterface(presContext->GetContainer()) I wouldn't have reindented the attributes in nsIDOMNSHTMLElement.idl. The module owner might have more feedback on this. + /** Called when the user manualls overrides the spellchecking state for this "user manually" The name "setSpellcheckCheckbox" seems a little strange to me. Can you think of a more descriptive name? Otherwise, looks good. The design seems overly complicated, but necessary given the requirements.

Comment on attachment 230200 [details] [diff] [review] candidate patch v3 Shifting sr? to sicking as he's promised to take a look at this.

(In reply to comment #45) > Otherwise, looks good. The design seems overly complicated, but necessary given > the requirements. Are you saying that the spec seems overly complicated given the requirements, that the code seems overly complicated given the spec, or both?

(In reply to comment #47) > Are you saying that the spec seems overly complicated given the requirements, > that the code seems overly complicated given the spec, or both? I'm saying that the spec requires what seems to be a complicated implementation.

Comment on attachment 230200 [details] [diff] [review] candidate patch v3 >Index: content/base/src/Makefile.in > plugin \ > prefetch \ > xuldoc \ > uriloader \ >+ editor \ > $(NULL) Fix indentation. >Index: content/base/src/nsDocument.cpp >+nsDocument::GetSpellcheckForElement(nsIContent* aContent, >+ nsIEditor* aEditor) const >+{ >+ nsGenericHTMLElement* element = nsGenericHTMLElement::FromContent(aContent); >+ if (!element) { >+ // Not an HTML element, so not spellchecked by default >+ return PR_FALSE; >+ } >+ >+ // See if the state has been explicitly set >+ PRBool enable; >+ if (element->GetSpellcheckState(&enable)) { >+ return enable; >+ } So this is unneccesarily complicated code-flow wise. Editor calls the element to check if spellchecking is enabled. The element then calls the document to ask the same thing. The document then calls back into the elment to ask yet the same thing. It seems like you could just implement all of this inside nsGenericHTMLElement::GetSpellcheck (i.e. both the code in nsDocument::GetSpellcheckForElement and the code in nsGenericHTMLElement::GetSpellcheckState) and let editor QI to nsIDOMNSHTMLElement and call GetSpellcheck. It'd be a tad slower, but I doubt it'll matter. >Index: content/html/content/src/nsGenericHTMLElement.cpp >+nsGenericHTMLElement::GetSpellcheck(PRBool* aSpellcheck) >+{ >+ nsIDocument* ownerDoc = GetOwnerDoc(); >+ if (!ownerDoc) { >+ return NS_ERROR_NOT_AVAILABLE; >+ } >+ >+ nsCOMPtr<nsIEditor> editor = nsnull; nsCOMPtrs automatically initialize to null, so you don't need to. >+ GetAssociatedEditor(getter_AddRefs(editor)); Do you need to propagate any errors from GetAssociatedEditor? > nsGenericHTMLElement::AfterSetAttr(PRInt32 aNamespaceID, nsIAtom* aName, > const nsAString* aValue, PRBool aNotify) > { >- if (aNamespaceID == kNameSpaceID_None && IsEventName(aName) && aValue) { >- nsresult rv = AddScriptEventListener(aName, *aValue); >- NS_ENSURE_SUCCESS(rv, rv); >+ if (aNamespaceID == kNameSpaceID_None) { >+ if (IsEventName(aName) && aValue) { >+ nsresult rv = AddScriptEventListener(aName, *aValue); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } else if (aName == nsHTMLAtoms::spellcheck) { Please follow the bracing style of the file and put the |else| on a new line. I think I saw the same thing in a couple of other places. >+ nsCOMPtr<nsIEditor> editor = nsnull; No need to init to nsnull. >+nsGenericHTMLElement::GetAssociatedEditor(nsIEditor** aEditor) >+{ >+ *aEditor = nsnull; >+ >+ // If contenteditable is ever implemented, it might need to do something different here? >+ >+ return GetEditorInternal(aEditor); You shouldn't need to init *aEditor to nsnull, GetEditorInternal should do that for you. >Index: content/html/content/src/nsGenericHTMLElement.h >+ static const nsGenericHTMLElement* FromContent(const nsIContent *aContent) >+ { >+ if (aContent->IsNodeOfType(eHTML)) >+ return NS_STATIC_CAST(const nsGenericHTMLElement*, aContent); >+ return nsnull; >+ } Why is this needed? We generally have very few const XPCOM pointers. >Index: content/html/content/src/nsHTMLBodyElement.cpp >+nsresult >+nsHTMLBodyElement::GetAssociatedEditor(nsIEditor** aEditor) This function never appears to return an error. Could you change the signature to return an already_AddRefed<nsIEditor> instead? That would avoid having to check for error at every callsite. >Index: dom/public/idl/html/nsIDOMNSHTMLElement.idl >- readonly attribute long offsetTop; >- readonly attribute long offsetLeft; >- readonly attribute long offsetWidth; >- readonly attribute long offsetHeight; >- readonly attribute nsIDOMElement offsetParent; >- attribute DOMString innerHTML; >- >- attribute long scrollTop; >- attribute long scrollLeft; >- readonly attribute long scrollHeight; >- readonly attribute long scrollWidth; >+ readonly attribute long offsetTop; >+ readonly attribute long offsetLeft; >+ readonly attribute long offsetWidth; >+ readonly attribute long offsetHeight; >+ readonly attribute nsIDOMElement offsetParent; >+ attribute DOMString innerHTML; I'm not convinced that all these whitespace changes are significantly for the better, not a big deal though. > NS_IMETHODIMP > nsEditor::PreDestroy() > { >- if (!mDidPreDestroy) { >- // Let spellchecker clean up its observers etc. >- if (mInlineSpellChecker) { >- mInlineSpellChecker->Cleanup(); >- mInlineSpellChecker = nsnull; >- } >+ if (mDidPreDestroy) >+ return NS_OK; > >- // tell our listeners that the doc is going away >- NotifyDocumentListeners(eDocumentToBeDestroyed); >+ // Let spellchecker clean up its observers etc. >+ if (mInlineSpellChecker) { >+ mInlineSpellChecker->Cleanup(); >+ mInlineSpellChecker = nsnull; >+ } When doing indenting changes like these, please include a -w patch as well as a normal patch. > >- // Unregister event listeners >- RemoveEventListeners(); >+ // tell our listeners that the doc is going away >+ NotifyDocumentListeners(eDocumentToBeDestroyed); > >- mDidPreDestroy = PR_TRUE; >- } >+ // Unregister event listeners >+ RemoveEventListeners(); > >+ mDidPreDestroy = PR_TRUE; > return NS_OK; > } > > NS_IMETHODIMP > nsEditor::GetFlags(PRUint32 *aFlags) > { > *aFlags = mFlags; > return NS_OK; > } > > NS_IMETHODIMP > nsEditor::SetFlags(PRUint32 aFlags) > { > mFlags = aFlags; >- return NS_OK; >+ >+ // Changing the flags can change whether spellchecking is on, so re-sync it >+ return SyncRealTimeSpell(); Wouldn't you want to return a success value even if syncing fails? Since the flag was indeed set. I guess it only matters if SyncRealTimeSpell could fail for non-fatal reasons. >+NS_IMETHODIMP nsEditor::SyncRealTimeSpell() >+{ >+ PRBool enable; Might be cleaner to init this to false and just follow the paths that can lead to a true. >+ // Check user override on this element >+ if (mSpellcheckCheckboxState != eTriUnset) { >+ enable = (mSpellcheckCheckboxState == eTriTrue); >+ } else { >+ // Check user preferences >+ enum SpellcheckDefaultState { >+ SpellcheckNever = 0, >+ SpellcheckMulti = 1, >+ SpellcheckAll = 2 >+ }; I'm not exited about having this enum in two places. Either just nuke it and use integers directly (hopefully making people think twice before changing them), or place it in a header somewhere.

(In reply to comment #49) > I'm not convinced that all these whitespace changes are significantly for the > better, not a big deal though. Indentation of property names to 38 characters seems to be a common cross-file minimum indentation in our DOM IDL files, although there are a few violations going smaller, plus violations going larger when needed.

(In reply to comment #49) > >Index: content/base/src/Makefile.in > Fix indentation. In Makefiles (and nowhere else), that means to use real tabs.

The main difference in this version is that it implements the solution sicking and I discussed on IRC for how to avoid some of the crazy architecture of the previous patches, by reading data off content instead of the editor when possible. This should address all of brettw's and sicking's review comments except the following: (In reply to comment #45) > Since you declare it in two places, I would declare rv at the top of the function. Since I still have to initialize the value in both places, this actually takes more lines of code, and since the variable ranges are completely disjoint, it just implies scoping/continuity that isn't actually present. (In reply to comment #49) > When doing indenting changes like these, please include a -w patch as well as a normal patch. Since I stripped out all the other whitespace/indent changes, this is now the only indenting level change in the whole giant patch. I can attach a -w diff of this file if you want.

Comment on attachment 231018 [details] [diff] [review] candidate patch v4 >Index: content/html/content/src/nsGenericHTMLElement.cpp >+nsGenericHTMLElement::GetSpellcheck(PRBool* aSpellcheck) >+{ >+ NS_ENSURE_ARG_POINTER(aSpellcheck); >+ *aSpellcheck = PR_FALSE; // Default answer is to not spellcheck >+ >+ // Has the state has been explicitly set? >+ for (nsGenericHTMLElement* element = this; element; >+ element = element->GetHTMLParent()) { Don't declare variables inside the loop conditions. Unfortunately different compilers scope such variables differently, often leading to compile errors down the road. (see the XPCOM code portability guide). No need for the GetHTMLParent function. Just do nsIContent* cont; for (cont = this; cont; cont = cont->GetParent()) { if (cont->IsNodeOfType(nsINode::eHTML)) { ... } } You could even use nsINode and GetNodeParent() which is slightly faster/smaller. However you'd need to cast to nsIContent at some point (which is safe after the IsNodeOfType check). >+ static nsIContent::AttrValuesArray strings[] = >+ {&nsHTMLAtoms::_true, &nsHTMLAtoms::_false, nsnull}; >+ switch (element->FindAttrValueIn(kNameSpaceID_None, >+ nsHTMLAtoms::spellcheck, strings, >+ eCaseMatters)) { >+ case 0: // spellcheck = "true" >+ *aSpellcheck = PR_TRUE; >+ // Fall through >+ case 1: // spellcheck = "false" I'm not super exited about this since it's easy to mess up if this code is updated. But i'm fine either way. >+ // Is this a chrome element? >+ if (nsContentUtils::IsChromeDoc(GetOwnerDoc())) { >+ return NS_OK; // Not spellchecked by default >+ } >+ >+ // Is this element disabled or readonly? >+ if (HasAttr(kNameSpaceID_None, nsHTMLAtoms::disabled) || >+ HasAttr(kNameSpaceID_None, nsHTMLAtoms::readonly)) { >+ return NS_OK; // Not spellchecked by default >+ } Nit: You could move this to the top to save cycles without adding code. >+ // Is this element editable? >+ nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(this); >+ if (!formControl) { >+ return NS_OK; // Not spellchecked by default >+ } >+ >+ // Is this a multiline plaintext input? >+ PRInt32 controlType = formControl->GetType(); >+ if (controlType == NS_FORM_TEXTAREA) { >+ *aSpellcheck = PR_TRUE; // Spellchecked by default >+ return NS_OK; >+ } >+ >+ // Is this anything other than a single-line plaintext input? Currently no (and hasn't been for the lifetime of the web :) >+nsGenericHTMLElement::IsCurrentBodyElement() >+{ >+ nsCOMPtr<nsIDOMHTMLBodyElement> bodyElement = do_QueryInterface(this); >+ if (!bodyElement) { >+ return PR_FALSE; >+ } |nsIDOMHTMLElement::GetBody| returns a frame element for framed documents. Not sure if we support spellchecking in such documents though. >Index: editor/libeditor/base/nsEditor.cpp >+nsEditor::GetDesiredSpellCheckState() ... >+ // Check DOM >+ nsIContent* content; >+ CallQueryInterface(GetRoot(), &content); >+ if (!content) { >+ return PR_FALSE; >+ } >+ >+ if (content->IsNativeAnonymous()) { >+ content = content->GetParent(); >+ } >+ >+ nsIDOMNSHTMLElement* element; >+ CallQueryInterface(content, &element); >+ if (!element) { >+ return PR_FALSE; >+ } >+ >+ PRBool enable; >+ element->GetSpellcheck(&enable); >+ return enable; This will leak since you never release the nsIDOMNSHTMLElement. Instead just do nsCOMPtr<nsIDOMNSHTMLElement> element = do_QI(content); With that fixed, sr=sicking

This takes the previous SR comments into account and also makes some changes discussed with Hixie regarding how to handle disabled and readonly textfields. It also makes one other "ensure passwords aren't spellchecked" change which I'm not sure is needed but I feel much better including.

Comment on attachment 231179 [details] [diff] [review] candidate patch v5 >Index: content/html/content/src/nsGenericHTMLElement.cpp >+nsGenericHTMLElement::SetSpellcheck(PRBool aSpellcheck) >+{ >+ return SetAttrHelper(nsHTMLAtoms::spellcheck, aSpellcheck ? >+ NS_LITERAL_STRING("true") : NS_LITERAL_STRING("false")); >+} Sorry I didn't catch this earlier. This will have trouble compiling on all platforms. Using references in :? syntax is a known problem in some compilers and usually show up when using strings in combination with :?. I would suggest simply using an if statement. Another problem with the patch is that when the spellcheck attribute is set on a node, we need to resync the spellcheck state on all descendants of that node, not just the node itself. Bug I guess that could wait for a later patch. sr=sicking

Fixed on trunk, including the nitpick in comment 55. Filed followup bug 346373 for the inherited editor resyncing stuff. /mozilla/browser/app/profile/firefox.js 1.139 /mozilla/content/base/src/nsGkAtomList.h 3.29 /mozilla/content/html/content/src/nsGenericHTMLElement.cpp 1.663 /mozilla/content/html/content/src/nsGenericHTMLElement.h 1.244 /mozilla/content/html/content/src/nsHTMLBodyElement.cpp 1.157 /mozilla/content/html/document/src/Makefile.in 1.39 /mozilla/content/html/document/src/nsHTMLDocument.cpp 3.691 /mozilla/content/html/document/src/nsHTMLDocument.h 3.196 /mozilla/dom/public/idl/html/nsIDOMNSHTMLElement.idl 1.9 /mozilla/editor/idl/nsIEditor.idl 1.26 /mozilla/editor/libeditor/base/nsEditor.cpp 1.472 /mozilla/editor/libeditor/base/nsEditor.h 1.179 /mozilla/editor/ui/composer/content/editorInlineSpellCheck.js 1.5 /mozilla/layout/forms/nsTextControlFrame.cpp 3.230 /mozilla/layout/forms/nsTextControlFrame.h 3.87 /mozilla/mail/components/compose/content/MsgComposeCommands.js 1.98 /mozilla/mailnews/compose/resources/content/MsgComposeCommands.js 1.382 /mozilla/toolkit/content/inlineSpellCheckUI.js 1.8

Followup commit made to change UUID of nsIEditor since I added more methods to its IDL file. Thanks to biesi for pointing this out. /mozilla/editor/idl/nsIEditor.idl 1.27

We'd need a branch version of this patch that doesn't change nsIEditor or nsIDOMNSHTMLElement, right?

(In reply to comment #58) > We'd need a branch version of this patch that doesn't change nsIEditor or > nsIDOMNSHTMLElement, right? Yes, I'm going to need to rewrite the patch for the branch.

Followup commit made on bzbarsky's request to check aNotify before syncing with editor. content/html/content/src/nsGenericHTMLElement.cpp 1.664

I have a branch patch for this which seems to work. Unfortunately right as I finished it off almost everything network-related at work went down, so it's sitting lonely on my hard drive. I'll try and post it in the morning.

The branch patch is by Peter whose computer is still not working correctly.

Comment on attachment 231593 [details] [diff] [review] Branch patch r/sr=sicking

Comment on attachment 231593 [details] [diff] [review] Branch patch Requesting approval to land on branch

Second version of branch patch, at sicking's request on IRC, to catch two tweaks: * I'd forgotten the comment 55 tweak in my first patch version * Needed to add my new nsIDOMNSHTMLElement_MOZILLA_1_8_BRANCH interface to the DOMClassInfo file. Hopefully I did it right. Both sicking and I wanted me to look at the diffs between this patch and my trunk patch, and the only other major differences are: * Rewriting nsINode*/FindAttrValueIn() code to use nsIContent*/GetAttr() * Making additional QIs to nsIEditor_MOZILLA_1_8_BRANCH in several places ...both of which should be safe.

It would help if I included all the files in the patch

Comment on attachment 231700 [details] [diff] [review] Branch patch v3 >Index: dom/src/base/nsDOMClassInfo.cpp >=================================================================== >RCS file: /cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v >retrieving revision 1.292.2.47 >diff -u -u -8 -p -r1.292.2.47 nsDOMClassInfo.cpp >--- dom/src/base/nsDOMClassInfo.cpp 28 Jul 2006 01:00:55 -0000 1.292.2.47 >+++ dom/src/base/nsDOMClassInfo.cpp 2 Aug 2006 01:29:30 -0000 >@@ -1670,16 +1670,17 @@ nsDOMClassInfo::RegisterExternalClasses( > DOM_CLASSINFO_MAP_ENTRY(nsIDOMDocumentTraversal) \ > DOM_CLASSINFO_MAP_ENTRY(nsIDOMDocumentXBL) \ > DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget) \ > DOM_CLASSINFO_MAP_ENTRY(nsIDOM3Document) \ > DOM_CLASSINFO_MAP_ENTRY(nsIDOM3Node) > > #define DOM_CLASSINFO_GENERIC_HTML_MAP_ENTRIES \ > DOM_CLASSINFO_MAP_ENTRY(nsIDOMNSHTMLElement) \ >+ DOM_CLASSINFO_MAP_ENTRY(nsIDOMNSHTMLElement_MOZILLA_1_8_BRANCH) \ > DOM_CLASSINFO_MAP_ENTRY(nsIDOMElementCSSInlineStyle) \ > DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget) \ > DOM_CLASSINFO_MAP_ENTRY(nsIDOM3Node) You want to replace nsIDOMNSHTMLElement with nsIDOMNSHTMLElement_MOZILLA_1_8_BRANCH here, since the branch interface inherits the main one. with that fixed, r/sr=sicking

Fixed that tweak and committed to the 1.8 branch: /mozilla/browser/app/profile/firefox.js 1.71.2.60 /mozilla/content/html/content/src/nsGenericHTMLElement.cpp 1.596.2.13 /mozilla/content/html/content/src/nsGenericHTMLElement.h 1.221.6.3 /mozilla/content/html/content/src/Attic/nsHTMLAtomList.h 3.73.4.4 /mozilla/content/html/content/src/nsHTMLBodyElement.cpp 1.153.8.1 /mozilla/content/html/document/src/Makefile.in 1.36.8.2 /mozilla/content/html/document/src/nsHTMLDocument.cpp 3.615.2.26 /mozilla/content/html/document/src/nsHTMLDocument.h 3.182.4.3 /mozilla/dom/public/idl/html/nsIDOMNSHTMLElement.idl 1.7.24.2 /mozilla/dom/src/base/nsDOMClassInfo.cpp 1.292.2.48 /mozilla/editor/idl/nsIEditor.idl 1.24.10.2 /mozilla/editor/libeditor/base/nsEditor.cpp 1.447.4.6 /mozilla/editor/libeditor/base/nsEditor.h 1.167.4.4 /mozilla/editor/ui/composer/content/editorInlineSpellCheck.js 1.2.4.2 /mozilla/layout/forms/nsTextControlFrame.cpp 3.197.10.8 /mozilla/layout/forms/nsTextControlFrame.h 3.68.10.4 /mozilla/mail/components/compose/content/MsgComposeCommands.js 1.72.2.19 /mozilla/mailnews/compose/resources/content/MsgComposeCommands.js 1.369.2.6 /mozilla/toolkit/content/inlineSpellCheckUI.js 1.2.10.7