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

RESOLVED FIXED in Thunderbird 42.0

Status

MailNews Core
Composition
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Steve Chessin, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
Thunderbird 42.0

Thunderbird Tracking Flags

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

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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

Comment 1

2 years ago
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".
(Reporter)

Comment 2

2 years ago
(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.)

Comment 3

2 years ago
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)
(Reporter)

Comment 4

2 years ago
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.

Comment 5

2 years ago
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.
(Reporter)

Comment 6

2 years ago
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.
(Assignee)

Comment 7

2 years ago
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)
(Reporter)

Comment 8

2 years ago
(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"?

Comment 9

2 years ago
(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)
(Assignee)

Comment 10

2 years ago
Created attachment 8640806 [details] [diff] [review]
Proposed patch
Assignee: nobody → neil
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(neil)
Attachment #8640806 - Flags: review?(iann_bugzilla)
Attachment #8640806 - Flags: feedback?(mozilla)

Comment 11

2 years ago
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+
(Assignee)

Comment 12

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

Updated

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

Updated

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

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 13

2 years ago
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)

Comment 16

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

Comment 17

2 years ago
(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)

Comment 18

2 years ago
Presumably we should take this simple patch in esr38 once it bakes.
tracking-thunderbird_esr38: --- → +

Comment 19

2 years ago
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)
(Assignee)

Comment 20

2 years ago
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

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

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

Comment 22

2 years ago
Comment on attachment 8640806 [details] [diff] [review]
Proposed patch

http://hg.mozilla.org/releases/comm-esr38/rev/e2b0ea36cfd1
Attachment #8640806 - Flags: approval-comm-esr38+

Updated

2 years ago
status-thunderbird_esr38: affected → fixed
tracking-thunderbird_esr38: + → 40+
(Reporter)

Comment 23

2 years ago
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.