Last Comment Bug 364914 - Don't spell check a <textarea>'s initial contents until I focus it
: Don't spell check a <textarea>'s initial contents until I focus it
Status: RESOLVED FIXED
: polish, ux-minimalism
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: Trunk
: All All
: -- enhancement with 2 votes (vote)
: mozilla10
Assigned To: Quentin Headen [:qheaden]
:
: Jet Villegas (:jet)
Mentors:
Depends on: 799475 569397 734530
Blocks: 712871
  Show dependency treegraph
 
Reported: 2006-12-24 11:24 PST by Jesse Ruderman
Modified: 2012-10-09 07:01 PDT (History)
14 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenshot - twitpic.com (25.05 KB, image/png)
2009-11-03 20:43 PST, Jesse Ruderman
no flags Details
screenshot - lala.com (9.00 KB, image/png)
2009-11-03 20:44 PST, Jesse Ruderman
no flags Details
A testcase for this bug (213 bytes, text/html)
2011-08-01 17:57 PDT, Quentin Headen [:qheaden]
no flags Details
Patch for enhancement (2.20 KB, patch)
2011-08-01 22:13 PDT, Quentin Headen [:qheaden]
no flags Details | Diff | Splinter Review
New, updated patch (4.65 KB, patch)
2011-08-02 20:26 PDT, Quentin Headen [:qheaden]
ehsan: review+
Details | Diff | Splinter Review
Patch Revision 2 (4.65 KB, patch)
2011-08-03 16:44 PDT, Quentin Headen [:qheaden]
ehsan: review+
Details | Diff | Splinter Review
Unit Test Patches (16.21 KB, patch)
2011-08-04 03:04 PDT, Quentin Headen [:qheaden]
ehsan: review+
Details | Diff | Splinter Review
fix test_bug596333.html and test_bug636465.xul reftests (5.45 KB, patch)
2011-08-20 12:33 PDT, arno renevier
no flags Details | Diff | Splinter Review
Latest patch with bug fix and test fixes (17.93 KB, patch)
2011-10-06 13:49 PDT, Quentin Headen [:qheaden]
ehsan: review+
Details | Diff | Splinter Review
Patch Revision 1 (20.06 KB, patch)
2011-10-06 17:41 PDT, Quentin Headen [:qheaden]
ehsan: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2006-12-24 11:24:22 PST
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.
Comment 1 Brett Wilson 2007-01-22 17:08:10 PST
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.
Comment 2 Jesse Ruderman 2007-01-22 17:11:46 PST
Can you give an example of where it's useful for Firefox to spell-check a field that has never had focus?
Comment 3 Jesse Ruderman 2007-01-22 17:13:08 PST
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.
Comment 4 Brett Wilson 2007-01-22 17:34:11 PST
Hmm, I guess if you keep it spellchecked once it has been focused no matter what, that would be a good change.
Comment 5 Jesse Ruderman 2007-01-22 17:40:24 PST
Yeah, that sounds right to me.
Comment 6 Jesse Ruderman 2009-11-03 20:43:45 PST
Created attachment 410125 [details]
screenshot - twitpic.com
Comment 7 Jesse Ruderman 2009-11-03 20:44:18 PST
Created attachment 410126 [details]
screenshot - lala.com
Comment 8 :Ehsan Akhgari 2010-07-22 16:52:31 PDT
Alex, how do you feel about this bug?
Comment 9 Alex Faaborg [:faaborg] (Firefox UX) 2010-07-22 18:01:35 PDT
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.
Comment 10 Alex Faaborg [:faaborg] (Firefox UX) 2010-07-23 00:21:14 PDT
sorry, bug 569397
Comment 11 Dão Gottwald [:dao] 2010-07-23 01:37:01 PDT
(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.)
Comment 12 :Ehsan Akhgari 2010-07-23 08:43:25 PDT
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.
Comment 13 Alex Faaborg [:faaborg] (Firefox UX) 2010-07-23 16:30:25 PDT
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.
Comment 14 Dão Gottwald [:dao] 2010-07-24 03:10:44 PDT
(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?
Comment 15 Alex Faaborg [:faaborg] (Firefox UX) 2010-07-24 15:33:03 PDT
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.
Comment 16 :Ehsan Akhgari 2010-07-25 10:14:56 PDT
(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!  ;-)
Comment 17 Quentin Headen [:qheaden] 2011-08-01 17:57:04 PDT
Created attachment 549986 [details]
A testcase for this bug

This is an HTML document containing a heading and a textarea with words that are misspelled.
Comment 18 Quentin Headen [:qheaden] 2011-08-01 21:14:53 PDT
I have started working on this enhancement. I hope to find a solution.
Comment 19 Quentin Headen [:qheaden] 2011-08-01 22:13:24 PDT
Created attachment 550009 [details] [diff] [review]
Patch for enhancement

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.
Comment 20 Quentin Headen [:qheaden] 2011-08-01 22:31:06 PDT
(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.
Comment 21 Alex Limi (:limi) — Firefox UX Team 2011-08-01 22:38:01 PDT
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?
Comment 22 Quentin Headen [:qheaden] 2011-08-01 23:14:16 PDT
(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.
Comment 23 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-08-01 23:44:18 PDT
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 24 :Ehsan Akhgari 2011-08-02 08:11:29 PDT
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?
Comment 25 Quentin Headen [:qheaden] 2011-08-02 20:26:21 PDT
Created attachment 550289 [details] [diff] [review]
New, updated patch

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 26 :Ehsan Akhgari 2011-08-03 11:48:16 PDT
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!
Comment 27 Quentin Headen [:qheaden] 2011-08-03 16:44:50 PDT
Created attachment 550556 [details] [diff] [review]
Patch Revision 2

Updated the last patch by adding spaces to two comment lines.
Comment 28 Quentin Headen [:qheaden] 2011-08-03 16:46:37 PDT
I am going to work on the unit test changes. Once I finish those, I will submit a second patch containing the changes.
Comment 29 Quentin Headen [:qheaden] 2011-08-04 02:24:10 PDT
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!
Comment 30 Quentin Headen [:qheaden] 2011-08-04 03:04:23 PDT
Created attachment 550634 [details] [diff] [review]
Unit Test Patches

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 31 :Ehsan Akhgari 2011-08-08 14:35:12 PDT
Comment on attachment 550634 [details] [diff] [review]
Unit Test Patches

This looks great, thanks!
Comment 32 :Ehsan Akhgari 2011-08-08 14:57:22 PDT
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.  :-)
Comment 33 :Ehsan Akhgari 2011-08-08 19:46:50 PDT
Quentin: can you please investigate the test failures in http://tbpl.mozilla.org/?tree=Try&rev=9f4a1fbedad6 ?
Comment 34 Quentin Headen [:qheaden] 2011-08-19 14:00:06 PDT
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.
Comment 35 :Ehsan Akhgari 2011-08-19 15:19:13 PDT
Arno, would you be interested in this bug?
Comment 36 arno renevier 2011-08-20 12:33:17 PDT
Created attachment 554655 [details] [diff] [review]
fix test_bug596333.html  and test_bug636465.xul reftests

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).
Comment 37 Quentin Headen [:qheaden] 2011-09-23 22:08:07 PDT
Well, I've come back to this bug to see if I can tackle it. If anyone isn't working on it, reassign me.
Comment 38 Quentin Headen [:qheaden] 2011-09-24 20:14:02 PDT
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
Comment 39 :Ehsan Akhgari 2011-09-26 14:35:12 PDT
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.  :-)
Comment 40 Quentin Headen [:qheaden] 2011-09-26 20:19:19 PDT
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.
Comment 41 Quentin Headen [:qheaden] 2011-10-01 19:29:48 PDT
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?
Comment 42 Quentin Headen [:qheaden] 2011-10-01 19:31:41 PDT
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.
Comment 43 Quentin Headen [:qheaden] 2011-10-04 18:46:46 PDT
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.
Comment 44 Quentin Headen [:qheaden] 2011-10-05 20:34:25 PDT
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
Comment 45 :Ehsan Akhgari 2011-10-06 12:02:43 PDT
(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!
Comment 46 Quentin Headen [:qheaden] 2011-10-06 13:49:59 PDT
Created attachment 565342 [details] [diff] [review]
Latest patch with bug fix and test fixes

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 47 :Ehsan Akhgari 2011-10-06 14:03:20 PDT
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.
Comment 48 Quentin Headen [:qheaden] 2011-10-06 17:41:23 PDT
Created attachment 565414 [details] [diff] [review]
Patch Revision 1

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 49 :Ehsan Akhgari 2011-10-07 06:28:24 PDT
Comment on attachment 565414 [details] [diff] [review]
Patch Revision 1

Thanks!  I'll push it to try, and then will land it for you.  :-)
Comment 50 Quentin Headen [:qheaden] 2011-10-09 15:47:50 PDT
Great! Let me know when you land it. :)
Comment 51 :Ehsan Akhgari 2011-10-16 19:07:31 PDT
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
Comment 52 Mozilla RelEng Bot 2011-10-16 23:20:50 PDT
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
Comment 53 :Ehsan Akhgari 2011-10-17 07:12:34 PDT
Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1906abbc9caf
Comment 55 Quentin Headen [:qheaden] 2011-10-17 11:21:33 PDT
Alright! I squashed my first bug! :)

Last question. What release version of Firefox will contain my fix?
Comment 56 j.j. 2011-10-17 11:59:31 PDT
See "Target Milestone" field: Mozilla 10 -> Firefox 10
https://wiki.mozilla.org/RapidRelease/Calendar
Comment 57 :Ehsan Akhgari 2011-10-17 21:06:10 PDT
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.  ;-)
Comment 58 rbauer 2012-03-12 08:44:38 PDT
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.
Comment 59 :Ehsan Akhgari 2012-03-12 16:47:13 PDT
Is this a web site that you control?  If so, the textarea can be made autofocus.
Comment 60 rbauer 2012-03-13 03:07:44 PDT
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.
Comment 61 Jesse Ruderman 2012-03-13 07:24:40 PDT
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
Comment 62 j.j. 2012-03-13 08:13:45 PDT
(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.
Comment 63 j.j. 2012-03-13 08:41:59 PDT
You may want to add a blur():

  for ( i=0; X[i]; i++ ) { X[i].focus(); X[i].blur() }
Comment 64 :Ehsan Akhgari 2012-03-14 16:42:45 PDT
(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.
Comment 65 Chris Jaure 2012-09-24 08:24:19 PDT
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?
Comment 66 :Ehsan Akhgari 2012-09-24 09:00:24 PDT
I think we just need to force a spell check when the spellcheck property is set on a textarea.
Comment 67 Chris Jaure 2012-09-25 06:49:09 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.