http://totallyunprofessional.com/blog/ contains a "Flooble Pad", a kind of mini-wiki within a blog for visitors to jot on. When I'm just visiting the blog (not editing the pad), I don't need the visual distraction or the confusion created by Firefox thinking "hornymanatee.com" is a misspelling. I'm not sure whether Firefox should spell check the field and not put in red underlines until I focus it, or whether it should not spell check it at all until I focus it.

This is the design. Although it is annoying in this specific case, I think it would be even more annoying in cases and people would promptly file bugs on it. This is especially true since it's not always clear what is focused. It is much better to err on the side of checking too much than checking too little.

Can you give an example of where it's useful for Firefox to spell-check a field that has never had focus?

Btw, I saw this odd behavior on a porn site too. In that case, I was amused at the number of typos in the paragraph.

Hmm, I guess if you keep it spellchecked once it has been focused no matter what, that would be a good change.

Yeah, that sounds right to me.

Alex, how do you feel about this bug?

over in bug 56397 I wrote: >Only displaying the red >lines when we initialize the editor violates ux-implementation-level, because >the user doesn't know that the editor can be in a state when it is not >initialized (or that it even exists). In terms of not spell checking until >they click in as an intentional feature (irrespective of the initialization >consideration), this could result in a ux-mode-error where the user doesn't >realize that the application is in a special state, so they incorrectly form >the conclusion that the text does not have any spelling errors, since no >indicators are present. however, that was based on the assumption that the text in question was created by the user. For instance, you are writing a forum comment, click submit, get an error page saying that you need to specify a group to post to, and on that error page your comment is not being spell checked because the editor wasn't active for the field containing the comment. Unfortunately there isn't any easy way to determine if the text was initially provided by the user, right? If it's the case that most pre-populated text is being provided by web sites, and disappearing when you focus the field, then I think Jesse is right that we might be doing more harm than good by displaying the developer's spelling errors.

sorry, bug 569397

(In reply to comment #9) > If it's the case that most pre-populated text is > being provided by web sites, and disappearing when you focus the field, The placeholder attribute, when used more widely, should fix this case. (We haven't shipped a release with support for it yet.)

Dao is right. And moreover, we have no way to know if the value is coming from the user or from the website (well, sometime we prefill the value from things such as the bfcache or passwordmgr, but this is not the case for textarea's.) FWIW, I don't remember the last time that I saw a textarea with a fake "placeholder" value injected by a website.

Limi points out that a lot of sites use text areas for terms of service, basically just so that they can get a scrollable area of text.

(In reply to comment #13) > Limi points out that a lot of sites use text areas for terms of service, > basically just so that they can get a scrollable area of text. Such a textarea is very likely to be readonly as well. Are readonly fields being spell-checked?

Apparently a lot of them aren't (even though editable text areas for terms of service is pretty ridiculous). I'm not necessarily against checking their spelling if they do that, but wanted to add the additional consideration.

(In reply to comment #14) > (In reply to comment #13) > > Limi points out that a lot of sites use text areas for terms of service, > > basically just so that they can get a scrollable area of text. > > Such a textarea is very likely to be readonly as well. Are readonly fields > being spell-checked? No. (In reply to comment #15) > Apparently a lot of them aren't (even though editable text areas for terms of > service is pretty ridiculous). I'm not necessarily against checking their > spelling if they do that, but wanted to add the additional consideration. Well, for this particular case, I think that most of the website owners make sure that the spelling in their TOS is correct! ;-)

This is an HTML document containing a heading and a textarea with words that are misspelled.

I have started working on this enhancement. I hope to find a solution.

This patch prevents the spellchecker from running on editor (<textarea>) boxes that are not focused. It does this via a comparison with the current focused element and the editor box. I hope it works correctly and in a desirable way. Please test it.

(In reply to comment #9) > over in bug 56397 I wrote: > > >Only displaying the red > >lines when we initialize the editor violates ux-implementation-level, because > >the user doesn't know that the editor can be in a state when it is not > >initialized (or that it even exists). In terms of not spell checking until > >they click in as an intentional feature (irrespective of the initialization > >consideration), this could result in a ux-mode-error where the user doesn't > >realize that the application is in a special state, so they incorrectly form > >the conclusion that the text does not have any spelling errors, since no > >indicators are present. > > however, that was based on the assumption that the text in question was > created by the user. For instance, you are writing a forum comment, click > submit, get an error page saying that you need to specify a group to post > to, and on that error page your comment is not being spell checked because > the editor wasn't active for the field containing the comment. > > Unfortunately there isn't any easy way to determine if the text was > initially provided by the user, right? If it's the case that most > pre-populated text is being provided by web sites, and disappearing when you > focus the field, then I think Jesse is right that we might be doing more > harm than good by displaying the developer's spelling errors. I just created a patch for this, but with this patch, any existing text is not spellchecked, even when the textarea comes into focus. Instead, new words typed by the user are spellchecked. Is it OK for it to behave this way? The reason this happens is because the DoSpellCheck method is called only when a textarea is created, not when it comes into focus.

It should really spell check everything in the textarea once it is focused. Is there no way we can flag it as not having been spell checked yet (or hide its underlining) until it gets focus?

(In reply to comment #21) > It should really spell check everything in the textarea once it is focused. > Is there no way we can flag it as not having been spell checked yet (or hide > its underlining) until it gets focus? That makes sense. Where in the source are "on focus" events handled for textareas, or any element for that matter? I might be able to hook into an event like that and order FF to perform a spell check then. There shouldn't be a need to worry about performance, because the spell check is going to be performed anyway. It might even save performance because it won't need to spell check at creation time, but just when the user needs it to.

We do a similar thing with the validity UI for HTML5 form elements (ie. they aren't marked as being invalid until the first unfocus or something). That appears to be controlled by http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#2008 and http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLTextAreaElement.cpp#733 .

Comment on attachment 550009 [details] [diff] [review] Patch for enhancement This patch is not correct, because it disables spellchecking for the existing contents of a text control after it being focused, and also because it affects spellchecking for contentEditable elements too. Here is what I think should be done. You should define a new flag in nsIPlaintextEditor called something like eEditorSkipSpellCheck to indicate the case in which the editor should skip spellchecking. Then, in nsTextEditorState::PrepareEditor, when we're setting up the initial editor flags <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#1147>, you should also set that flag. In order to make sure that spellchecking is turned on when the editor is first focused, in nsEditorEventListener::Focus, you should check to see if the editor has that flag set, and unset it if it does. Now, to make the flag actually work, all you have to do is to edit nsEditor::CanEnableSpellCheck to return false if that flag has been set. Once you do that, nsEditor::SetFlags can handle the case where that flag changes, and it can turn spellchecking on correctly. Does this make sense?

This patch fixes the problem. When the textarea is first loaded, spell checking is disabled on it. As soon as the user clicks the box, bringing it into focus, the spell checking is enabled, the spell checker runs, and all of the misspelled words are underlined. Thank you for your guidance Ehsan. Please let me know if this works are desired.

Comment on attachment 550289 [details] [diff] [review] New, updated patch This looks very good, thanks! >+ //If the spell check skip flag is still enabled from creation time, >+ //disable it because focused editors are allowed to spell check. Nit: please add a space after the second slash on the above lines. r=me on the code. Now, we should address two more things. The first one is existing tests which make sure that spell checking works. We have some tests in <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/editor/> (look for the file names beginning with spellcheck"), which rely on spellchecking happening as soon as the document is loaded. This will break with your patches, so if you run them with your patch (see https://developer.mozilla.org/en/Creating_reftest-based_unit_tests if you're not familiar with the reftest framework, and let me know if you need help with that!), you should see that a number of them fail. So, what you want to do is to focus and blur these fields in those tests and make sure to wait long enough for the spellchecking to complete. In order to do that, you should add class="reftest-wait" to the html element in those test files to signal the reftest framework to wait for our job to finish after loading the test before taking a screenshot of the document, and then in an onload handler for the document, do the following: var textarea = document.querySelector("textarea"); // or an input element if we're dealing with one textarea.focus(); textarea.blur(); setTimeout(function() { setTimeout(function() { // We need to hit the event loop twice to make sure that spellchecking happens in time document.documentElement.removeAttribute("class"); // signal the reftest framework that we're done, and it can take its screenshot }, 0); }, 0); Then, you need to add a test to make sure that the textareas/inputs are not spellchecked until focused. In order to test that, you can just create a reftest which doesn't focus/blur the element, and then compare it with one that does, and assert that they should not be equal. Let me know if you need help with this!

Updated the last patch by adding spaces to two comment lines.

I am going to work on the unit test changes. Once I finish those, I will submit a second patch containing the changes.

I am trying to modify these reftests to incorporate my new code changes. I notice that when I run firefox.exe and open the spellcheck-input-ref.html file in it, the textbox is not spellchecked, even though the javascript in the file focused the box. When I remove the blur code, it opens with the misspelled words underlined because it is in focus. Is it OK if i remove the bluring code from spellcheck-input-ref.html? (In reply to comment #26) > Comment on attachment 550289 [details] [diff] [review] [diff] [details] [review] > New, updated patch > > This looks very good, thanks! > > >+ //If the spell check skip flag is still enabled from creation time, > >+ //disable it because focused editors are allowed to spell check. > > Nit: please add a space after the second slash on the above lines. > > r=me on the code. > > Now, we should address two more things. The first one is existing tests > which make sure that spell checking works. We have some tests in > <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/editor/> > (look for the file names beginning with spellcheck"), which rely on > spellchecking happening as soon as the document is loaded. This will break > with your patches, so if you run them with your patch (see > https://developer.mozilla.org/en/Creating_reftest-based_unit_tests if you're > not familiar with the reftest framework, and let me know if you need help > with that!), you should see that a number of them fail. So, what you want > to do is to focus and blur these fields in those tests and make sure to wait > long enough for the spellchecking to complete. In order to do that, you > should add class="reftest-wait" to the html element in those test files to > signal the reftest framework to wait for our job to finish after loading the > test before taking a screenshot of the document, and then in an onload > handler for the document, do the following: > > var textarea = document.querySelector("textarea"); // or an input element if > we're dealing with one > textarea.focus(); > textarea.blur(); > setTimeout(function() { > setTimeout(function() { > // We need to hit the event loop twice to make sure that spellchecking > happens in time > document.documentElement.removeAttribute("class"); // signal the reftest > framework that we're done, and it can take its screenshot > }, 0); > }, 0); > > Then, you need to add a test to make sure that the textareas/inputs are not > spellchecked until focused. In order to test that, you can just create a > reftest which doesn't focus/blur the element, and then compare it with one > that does, and assert that they should not be equal. > > Let me know if you need help with this!

While waiting for your reply to my last question, I went ahead and made some changes to the reftests. First I removed the blurring code from the main input and textarea spell check reference documents. In all of the spell check documents, I made it so that the boxes are focused when the test loads. I created one more test that makes sure the textareas and input boxes don't spell check unless they are focused first. I'm not sure if you like it done this way, but I thought I would give it a shot. Let me know what you think.

Comment on attachment 550634 [details] [diff] [review] Unit Test Patches This looks great, thanks!

I submitted your patch to the try server to make sure that it doesn't break any tests. If the results come back green, we can land it. :-)

Quentin: can you please investigate the test failures in http://tbpl.mozilla.org/?tree=Try&rev=9f4a1fbedad6 ?

I've unassigned myself from this bug (for now). I was working on this bug and another bug I was assigned to before. Since I am a beginner with the Mozilla project, I should only work on one bug at a time. So someone else can take it if they want to. If this bug is still unassigned when I finish working on the other bug, I will assign myself to it again.

Arno, would you be interested in this bug?

Actually, once I finish with my assigned bugs, I'll probably want to make a spellcheckbugs-break :) So, once I'm in the mood for it again, I'll check if Quentin has reassigned the bug to himself. But I've a few comments about the bug: I think updated reftests are wrong: just setting i.focus() in not enough. When I put a printf in nsEditorSpellCheck::InitSpellChecker and run the tests, I see that that method is not called. Actually, the test will exit before spellcheck is inited. Tests seem to succeed because both refs and tests will not spell. What needs to be done is something similar to what I've done in bug #338427: put a class="reftest-wait" to html element, then wait in focus handler + a timeout to remove it. That way we known spellcheck has been inited. http://hg.mozilla.org/mozilla-central/file/6dc468c41136/layout/reftests/editor/338427-2-ref.html Also, for the same reason, some other tests appear to pass, but are currently wrong. For example, 338427-1-ref.html http://hg.mozilla.org/mozilla-central/file/6dc468c41136/layout/reftests/editor/338427-1-ref.html Other tests to investigate include: layout/reftests/editor/spellcheck-dotafterquote-valid-ref.html layout/reftests/editor/spellcheck-hyphen-multiple-valid-ref.html layout/reftests/editor/spellcheck-hyphen-valid-ref.html layout/reftests/editor/spellcheck-slash-valid-ref.html layout/reftests/editor/spellcheck-comma-valid-ref.html layout/reftests/editor/spellcheck-period-valid-ref.html Same thing (focus handler + timeout) needs to be done for them. I known all this probably sounds like a lot of work at first sight, but it's quite easy once you got it. Failing reftest on the try server are just two forgotten tests who needs to be updated. Actually, I have a patch that fixes them. test_bug636465.xul was slightly tricky: I needed to focus to init spellcheck, then blur at the correct moment, before snapshots were made. Two other things: - I may be wrong, but shouldn't uuid be changed whenever and idl interface is modified ? - With this patch, is nsEditorSpellCheck::InitSpellChecker always called within focus handler ? I think so, but I'm not totally sure. Because if that's the case, updateCurrentDictionary should not be called from within nsEditorSpellCheck::InitSpellChecker because it will called from focus handler anyway (with this patch, it's called twice in row at first focus).

Well, I've come back to this bug to see if I can tackle it. If anyone isn't working on it, reassign me.

I pushed pretty much the same changes to the Tinderbox, and I (of course) got many failures. I am trying to investigate these failures, but I really don't understand how to read them. For example, I notice on the end many reftest failures "load failed: null". What does this mean? Does it mean that a reftest html document failed to load? Here is the log: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=25392e1f035c

If you look closely at the full log, you'll see lines like: JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIWebNavigation.loadURI]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: chrome://reftest/content/reftest-content.js :: LoadURI :: line 655" data: no] For example, it seems like your patch removes the file layout/reftests/editor/spellcheck-input-disabled.html, but doesn't edit the reftest.list file to remove that file from the manifest too. This causes the reftest framework to try to load a non-existing file, which results in the error message that you were seeing. The other files hitting this error are instances of similar problems. You can test the reftests locally before pushing to try to make sure that you don't have any obvious bugs in them. You can run the editor reftests by issuing this command: make -C objdir reftest TEST_PATH=layout/reftests/editor/reftest.list Let me know if you need more help. :-)

Thanks a lot for your help Ehsan. I'm slowly but surely learning Mozilla. The problems should have been obvious to me. :) I fixed the issues, and I have submitted a second patch to the try server.

I'm trying to fix some of the tests I am breaking with my patch. One of the tests I broke was editor/libeditor/text/tests/test_bug596333.html. I am breaking the "is()" check on line 70. Basically, the editor box starts with the contents "I can haz cheezburger", which of course should cause the spellchecker to show two incorrect words, haz and cheezburger, when the test starts. But this check seems to go against the purpose of my patch. My patch prevents spellchecking on page-load until you click the editor box. So can I remove this line?

Oh nevermind. I just noticed that arno had made a patch for the test I just mentioned above. I will just merge his patch with mine.

I just want to let everyone know I am still working on this bug. I was able to bring my failing job count from 46 down to 34, and I just submitted a patch that will hopefully cut that count down even more.

Ok, I was able to cut my job failure count down to 16 jobs. Here is the pushlog report: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=07ec9300a355 Are any or most of these job failures intermittent? Thanks

(In reply to Quentin Headen from comment #44) > Ok, I was able to cut my job failure count down to 16 jobs. Here is the > pushlog report: > https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=07ec9300a355 > > Are any or most of these job failures intermittent? > > Thanks Yes! Awesome. :-) I think your patch is ready for review now. Would you mind attaching it to the bug? Thanks!

OK, here is the patch that passed the tests. The patch contains both the bug fix and the changes needed to the spellchecking tests. Also, credit to arno for helping me see how to fix a couple of them. :)

Comment on attachment 565342 [details] [diff] [review] Latest patch with bug fix and test fixes Review of attachment 565342 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! If you could address the following comments, and also make sure that the patch applies cleanly on the trunk, I will go ahead and land this for you. ::: browser/base/content/test/test_contextmenu.html @@ +22,5 @@ > > const Cc = Components.classes; > const Ci = Components.interfaces; > > +function openContextMenuFor(element, shiftkey, waitForFocus) { Using the name waitForFocus is confusing here, because we also have SimpleTest.waitForFocus. Could you please change this to shouldWaitForFocus or something like that? ::: editor/libeditor/text/tests/test_bug596333.html @@ +72,1 @@ > gMisspeltWords = ["haz", "cheezburger"]; Nit: please fix the indentation here.

Here is a revision of the patch with the changes you requested. I did a checkout of the latest code and applied the patch. It seems to apply well. You can check for yourself to make sure.

Comment on attachment 565414 [details] [diff] [review] Patch Revision 1 Thanks! I'll push it to try, and then will land it for you. :-)

Great! Let me know when you land it. :)

D'oh! Seems like I've lost the link to the try server job, and the results were not reported here either. :( Pushed to try again: http://tbpl.mozilla.org/?tree=Try&rev=6ae77375abe7

Try run for 6ae77375abe7 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=6ae77375abe7 Results (out of 193 total builds): success: 168 warnings: 15 failure: 10 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-6ae77375abe7

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/1906abbc9caf

https://hg.mozilla.org/mozilla-central/rev/1906abbc9caf https://hg.mozilla.org/mozilla-central/rev/ec7577dec4fc

Alright! I squashed my first bug! :) Last question. What release version of Firefox will contain my fix?

See "Target Milestone" field: Mozilla 10 -> Firefox 10 https://wiki.mozilla.org/RapidRelease/Calendar

Thanks a lot Quentin for your patch! I've got to tell you, this first bug was not easy at all, and you did a great job! :-) Let me know if you're looking to do more stuff in Mozilla. I can always point you to some interesting stuff. I've already pointed you to at least one such bug. ;-)

Can this new behavior be disabled (reverting to the old behavior of spell-checking all text fields when the page is loaded)? I have a use case for this behavior: proofreading submitted form data. We have relied on FF spell checker to quickly spot misspelled words in the form data. Now we have to click on every field to activate the spell checker. I have no issue if this new behavior is the default, as long as I can disable it as needed.

Is this a web site that you control? If so, the textarea can be made autofocus.

We do have control over the website, however, many of the forms have multiple textareas. We have tried to work around this with javascript to no avail, so far.

Maybe a spellcheck="true" attribute should trigger spellcheck before focus. http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#spelling-and-grammar-checking

(In reply to rbauer from comment #60) > We have tried to work around this with javascript to no avail, so far. Should be as simple like this: var X= document.getElementsByTagName("textarea"); for ( i=0; X[i]; i++ ) { X[i].focus() } Place it on bottom or in an onload handler.

You may want to add a blur(): for ( i=0; X[i]; i++ ) { X[i].focus(); X[i].blur() }

(In reply to Jesse Ruderman from comment #61) > Maybe a spellcheck="true" attribute should trigger spellcheck before focus. > > http://www.whatwg.org/specs/web-apps/current-work/multipage/editing. > html#spelling-and-grammar-checking Yeah that makes sense. I'll do that in bug 734530.

I have the same use case as rbauer. Multiple fields need to be proof-read for grammar/spelling errors. Having to focus all fields via javascript is a poor solution, especially considering the page will jump to the last focused textarea unless I scroll to top. Either method breaks maintaining the current scroll position on refresh. I tried adding spellcheck="true" to force it, but that doesn't appear to be implemented. A config setting would be much appreciated to toggle the spellcheck behavior. What do other browsers do?

I think we just need to force a spell check when the spellcheck property is set on a textarea.

For comparison, Chrome 21 checks each individual word only after the caret moves through it. However, there is an asynchronous spellcheck flag in chrome://flags that seems to replicate Firefox's current behavior (spellchecking the entire field on focus). Adding the "spellcheck" property also does nothing.