Closed Bug 1131122 Opened 9 years ago Closed 9 years ago

You have to click "Recheck Page" twice to get it to spell check again

Categories

(MailNews Core :: Composition, defect)

Product:
MailNews Core
Component:
Composition
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird42 fixed, thunderbird_esr3840+ fixed, seamonkey2.35 affected, seamonkey2.36 affected, seamonkey2.37 affected, seamonkey2.38 affected, seamonkey2.39 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 42.0
Tracking Flags:
Tracking Status
thunderbird42 --- fixed
thunderbird_esr38 40+ fixed
seamonkey2.35 --- affected
seamonkey2.36 --- affected
seamonkey2.37 --- affected
seamonkey2.38 --- affected
seamonkey2.39 --- fixed

People

(Reporter: steve.chessin, Assigned: neil)

Details

Attachments

(1 file)

Proposed patch
924 bytes, patch
iannbugzilla
: review+
jorgk-bmo
: feedback+
rkent
: approval-comm-esr38+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150122214805

Steps to reproduce:

This is with 31.4.0, but I think it started with 31.0.

Make sure you set "Preferences->Composition->Spelling->Check spelling before sending".

Compose a message with spelling errors (multiple occurrences of "asdf asdf"), then click Send. Click "Ignore All" on each error it finds. When it says "Completed spell checking." and Send lights up, click on "Recheck Page" instead.


Actual results:

Nothing. If you click on "Recheck Page" a second time, it then rechecks the page.


Expected results:

It should have rechecked the spelling on the first click of Recheck Page. Earlier releases (I forget how far back) did this.
Component: Untriaged → Message Compose Window
OK, TB 38.1.
Four misspelled words: huhu hoho hihi haha.
Send. Hit "Ignore" four times. Panel says: "Completed spell checking.".
Hit "Recheck Page".
Checking goes back to "huhu".

I can't see a problem.
I've also tried a TB 41 (Earlybird) and TB 31.6, I can't see the problem there either.
I've also started TB is "safe mode" (Help > Restart with Add-ons Disables). No problem.

Can the reporter please check a recent version and also try in "safe mode".

So unless someone provides a reproducible case, I will close this bug as "Works for me".
(In reply to Jorg K from comment #1)
> OK, TB 38.1.
> Four misspelled words: huhu hoho hihi haha.
> Send. Hit "Ignore" four times. Panel says: "Completed spell checking.".
> Hit "Recheck Page".
> Checking goes back to "huhu".
> 
> I can't see a problem.
> I've also tried a TB 41 (Earlybird) and TB 31.6, I can't see the problem
> there either.
> I've also started TB is "safe mode" (Help > Restart with Add-ons Disables).
> No problem.
> 
> Can the reporter please check a recent version and also try in "safe mode".
> 
> So unless someone provides a reproducible case, I will close this bug as
> "Works for me".

Redo your test case using "Ignore All" (as I said in the original report) instead of "Ignore". I was able to reproduce this in "safe mode" as well. (I haven't tried the Earlybird version.) (I also got interesting results if I used a mixture of "Ignore" and "Ignore All". If I use "Ignore All" on all but the last error, and "Ignore" the last error, "Recheck Page" starts the check on the last word it found. If I use "Ignore" on any word other than the last, and "Ignore All" on the rest, "Recheck Page" works as intended.)
Terribly sorry about not reading the original description properly. It said "Ignore all".
Therefore now I looked at the problem a bit harder.

First of all, it has nothing to do with the "Check spelling before sending". The normal spell check triggered via the "Spelling" button behaves just the same.

If you click "Ignore all" on all misspelled words, then indeed, after everything is checked the "Recheck Page" seems to do nothing, until you click it a second time. What it really does is resume the spell check with the last "(simply) ignored" word. Since you "ignored (all)" all words, it does nothing. It seems to forget the ignored words, if you click a second time.

Try this: Use four words and click "Ignore all" on 1, 3 and 4, and "Ignore" on 2. When we come to recheck, it starts on the second word. Is this how it's intended? I don't think so.

I'll ask some people who should know.

Thomas and Neil: "Recheck Page" should forget all ignored words and start from the beginning, right?
Flags: needinfo?(neil)
Flags: needinfo?(bugzilla2007)
Here's an even simpler test.

Put in at least two different misspelled words.

Check spelling, Ignore the first word, and then Recheck Page.

Now check spelling, Ignore All the first word, and then Recheck Page.
Sorry, can you state this more precisely:
>  Check spelling, Ignore the first word, and then Recheck Page.
Then what? Close the panel? Your next step is 
> Now check spelling
Well, the panel is already open, unless you close it, so I don't understand the step.
Sorry, I was sloppy. You can close the spell-check panel after clicking Recheck Page, or you can just Ignore All the first word and then click Recheck Page. The actual steps I did were:

Check spelling, Ignore the first word, Recheck Page. It goes back to the first word.
Ignore All that word, Recheck Page. It doesn't do anything.
Recheck Page again, it goes back to the first word.

Anyway, I'm glad you were able to duplicate the bug.
So, what used to happen was that rechecking the page would uninitialise the editor spell check object, which would drop its reference the moz spell checker object, which would get destroyed, which would end the spelling session, which would make the personal dictionary forget its ignored word list.

Now in bug 569504 ehsan made the spell checker stop leaking by adding it to the cycle collector, however this means that when the editor spell check object drops its reference to the moz spell checker object it doesn't get destroyed until the next cycle collection.

It looks as if we can work around the problem by explicitly ending the spelling session.
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk from comment #7)
> So, what used to happen was that rechecking the page would uninitialise the
> editor spell check object, which would drop its reference the moz spell
> checker object, which would get destroyed, which would end the spelling
> session, which would make the personal dictionary forget its ignored word
> list.
> 
> Now in bug 569504 ehsan made the spell checker stop leaking by adding it to
> the cycle collector, however this means that when the editor spell check
> object drops its reference to the moz spell checker object it doesn't get
> destroyed until the next cycle collection.

How does that explain the difference in behavior between using "Ignore" and "Ignore All"?
(In reply to Steve Chessin from comment #8)
> How does that explain the difference in behavior between using "Ignore" and
> "Ignore All"?

"Ignore" is "once only" and is handled locally in TB. "Ignore all" is passed on to the M-C spell checker, so all future occurrences are ignored. In other words, the M-C spell checker never hears about the "(simply) ignored" words. It only collects the "ignored (all)" words.

The M-C data structure needs to be dropped when starting again.

(In reply to neil@parkwaycc.co.uk from comment #7)
> It looks as if we can work around the problem by explicitly ending the
> spelling session.

Neil, thank you for the analysis. This should be an easy fix, right? Can you provide it, since it's in /editor and also affects SeaMonkey?
Flags: needinfo?(bugzilla2007) → needinfo?(neil)
Attached patch Proposed patch — — Splinter Review 
Assignee: nobody → neil
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(neil)

        Attachment #8640806 -
        Flags: review?(iann_bugzilla)

        Attachment #8640806 -
        Flags: feedback?(mozilla)

    
    

    
  
Comment on attachment 8640806 [details] [diff] [review]
Proposed patch

Works fine for me.

On a different subject:
In bug 345852 M-C changed the way the personal dictionary is stored. Before it was stored at the end of the session, now it's stored when additions and removals are made. Does that affect TB or SM?

https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#190
https://dxr.mozilla.org/comm-central/source/suite/mailnews/compose/MsgComposeCommands.js#158

I guess it doesn't, but I wanted to mention it.

        Attachment #8640806 -
        Flags: feedback?(mozilla) → feedback+

    
    

    
  
I guess it might become a problem if the inline spell check ignore list stops resetting.

    
  
Component: Message Compose Window → Composition
OS: Mac OS X → All
Product: Thunderbird → MailNews Core
Hardware: x86 → All
Version: 31 → Trunk

    
  

        Attachment #8640806 -
        Flags: review?(iann_bugzilla) → review+

    
    

    
  
Thanks, but I have level 3 Mercurial access.
Keywords: checkin-needed

    
    

    
  
(In reply to Jorg K from comment #3)
> Thomas and Neil: "Recheck Page" should forget all ignored words and start
> from the beginning, right?

Looks like this has already been dealt with, but ftr, yes, I would expect "Recheck Page" to forget all ignored-once and ignored-all words and start again from scratch.

    
    

    
  
(In reply to neil@parkwaycc.co.uk from comment #13)
> Thanks, but I have level 3 Mercurial access.

Which means that you can do the checkin yourself, but unless the checkin is actually done immediately, checkin-needed looks like the only flag we can set to avoid forgetting to checkin...?
And if somebody else followed up on the checkin flag faster than you, that wouldn't be disadvantageous either?
Flags: needinfo?(neil)

    
    

    
  
Hi Neil, I know that your have level 3 access, but I don't know why you haven't checked it in yet. ;-)

    
    

    
  
(In reply to Thomas D. from comment #15)
> Which means that you can do the checkin yourself, but unless the checkin is
> actually done immediately, checkin-needed looks like the only flag we can
> set to avoid forgetting to checkin...?
If you think I've forgotten to check in then needinfo? works; checkin-needed means that I can't or don't want to check in. It also means that the attached patch is suitable for checking in by someone else, which it isn't.

> And if somebody else followed up on the checkin flag faster than you, that
> wouldn't be disadvantageous either?
That's hardly likely to happen given that all trees are busted right now. (This also answers JorgK's question of course.)
Flags: needinfo?(neil)

    
    

    
  
Presumably we should take this simple patch in esr38 once it bakes.

          tracking-thunderbird_esr38:
          --- → +

    
    

    
  
So let this bake by landing it!
Neil, I've seen some checkins in /mailnews, so the tree should be good to land this, right?
(I'm on holidays, so I haven't refreshed my local tree recently and I don't know the status.)
Flags: needinfo?(neil)

    
    

    
  
Pushed comm-central changeset b8b4cb495066.

(Thanks for reminding me to recheck the tree status.)

          status-seamonkey2.35:
          --- → affected

          status-seamonkey2.36:
          --- → affected

          status-seamonkey2.37:
          --- → affected

          status-seamonkey2.38:
          --- → affected

          status-seamonkey2.39:
          --- → fixed

          status-thunderbird42:
          --- → fixed

          status-thunderbird_esr38:
          --- → affected
Flags: needinfo?(neil)
Target Milestone: --- → Thunderbird 42.0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED

    
    

    
  
Oops, I got so carried away setting the tracking flags that I forgot about that.

    
    

    
  
I know you were targeting TB 42.0, but it seems to have been fixed in 38.2.0 or earlier.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: