216235 - Spell Check should ignore reply header text on check before send

Status: NEW

(Thunderbird :: Message Compose Window, defect, P3)

Description

[Scott MacGregor]

The spell checker currently ignores quoted message bodies. But we don't ignore
the reply header text we insert into the editor instance right before the quoted
text.

i.e. "John Doe wrote:"
 **insert quoted text which we dont spell check**

Here's how we currently insert a quoted message into editor:

1) We call InsertHTML or InsertText to insert the reply header
2) Then we call InsertQuotationWithCitation to insert the actual reply. This
reply gets marked with a block="cite" attribute on it so the spell checker knows
to ignore it.

In order to fix this bug I think I need to do the following:

Extend nsIEditorMailSupport.idl to add reply text support. I could see this
happening two ways:
  1) add a new method, say InsertReplyText or InsertQuotationPrefix which
creates a dom text fragment just like the existing InsertQuotedCitation does and
sets an attribute on this dom fragment which the spell checker filter code could
 look for and then ignore the element.

  2) Extend insertAsCitedQuotation to pass in a prefix string which gets created
as a separate dom fragment from the quotation dom node (we don't want it being
part of the block quote of course). Then set an attribute on this prefix dom
node so the spell checker can ignore it. One problem with this approach is that
insertAsCitedQuotation returns a DOMNode. I'd be changing it to create two dom
nodes but only returning the second one (the actual quoted one). That's probably
not going to cause a problem but could be considered inconsistent.

cc'ing Kin in case he has a suggestion or a preference for one of these approaches.

Comment 1

[Scott MacGregor]

I'll talk to Kin about this for thunderbird 0.3

Comment 2

[Scott MacGregor]

I decided it was a lot easier to solve this without changing any of the mail
editor APIs. I can just wrap the reply text with span that has a special class
set on it in mail compose directly. The spell checker filter checks for this
attribute on spans and skips the contents if appropriate.

Patch coming up.

This patch also includes a small change to support skipping signatures as well. 

Comment 3

[Scott MacGregor]

Attachment: the patch

The patch looks a little more complicated than it is because I collapsed a
couple nested if clauses in nsComposeTxtSrvFilter.

In short it does the following:

1) the filter used to look for pre or span tags with _moz_quote=true on it.
_moz_quote is used by editor to denote and style quoted text. We now also look
for _moz_skip_spellcheck (is this name okay? do we want another name here?)
which is an attribute mail compose sets on elements that are not part of the
quoted text but which we want to ignore when it comes to spell checking. For
HTML signatures, we also look for divs in addition to pre and span tags with
this attribute set on it. 

2) In mail compose, we wrap the reply text with a span (is that the right
element to use? I think it is the most harmless), with an attribute of
_moz_skip_spellcheck="true". 

3) In mail compose, the div and the pre (for plain text) used for the signature
now includes the attribute/value pair: _moz_skip_spellcheck="true".

Comment 4

[xolaware.llc]

i've noticed this on macOS X, too, but can't change the HW/OS fields since i'm not owner.

Comment 5

[Calum Mackay]

Yup, this is generic; changing to All.

Comment 6

[Scott MacGregor]

This has been fixed on the 0.3 branch. Will wait for the review from Kin before
landing on the trunk for all mozilla mail customers.

Comment 7

[kinmoz]

So your changes look fine to me ... but one concern I have is that reply header
text isn't accounted for in the edit rules, so that means that if the user
places the caret at the  beginning of:

  |Kin wrote:

and started typing, his content would end up in the span, and not be
spellchecked. Same thing with the div used for the sig.

Comment 8

[Scott MacGregor]

for historical purposes, I have never checked in this patch because I ran into
exactly what Kin mentioned in his review comments. 

It is too easy for user text to get inserted inside the reply header span and
suddenly that text gets missed by the spell checker.

I don't know enough about editor yet to know if there is a way to keep stuff out
of that span....

Comment 9

[Peter Kupfer]

I would like to see this issue picked back up again.

The issue, at least what I am having in 0.8, is that when I reply, the text is
quoted with > and the spell checker ignores it, but it checks the name of the
previous author. This is very irriatating when that is the only word spelled wrong.

I think it should just add the previous author to the quoted text. This would
solve another problem that arises in long threads, because the person who wrote
something is quoted on a different level. For example:

Hi am Peter
>Paul Smith Wrote
> :I am Paul Smith
> :
> : John Does wrote
> : >I am John Doe.

It can start to become unclear who wrote what as you go further through several
posts in a newsgroup.

Thanks,

Peter

Comment 10

[Josh Wand]

What about this comment 2 at bug 245897 ? Seems like a much simpler solution to
me (and shouldn't someone consolidate these two bugs?)

https://bugzilla.mozilla.org/show_bug.cgi?id=245897#c2

Comment 11

[Peter Kupfer]

It might work every now and then if it is a frequent contact, but when replying
to newgroups and heavily used help lists, it doens't really work.

Thanks for checking though.

I just thought they could put a little > in front of that part too.

Comment 12

[Josh Wand]

(In reply to comment #11)
> It might work every now and then if it is a frequent contact, but when replying
> to newgroups and heavily used help lists, it doens't really work.

Presumably, when TB composes the reply and inserts the header text, it knows the
text it is inserting (e.g. "Foo Bar wrote:). It's just a question of holding
onto that text, and when the spellchecker is initialized, putting that in the
"ignore all" list.

Comment 13

[Tal Cohen]

See also Bug 190660 for a related issue.

Comment 14

[Jo Hermans]

*** Bug 309602 has been marked as a duplicate of this bug. ***

Comment 15

[Simon Paquet [:sipaq]]

Comment on attachment 131299 [details] [diff] [review]
the patch

Scott, does Kin still do reviews? This patch has been open for 3,5 years.

Comment 16

[Magnus Melin [:mkmelin]]

I don't think so...

Comment 17

[Phil Ringnalda (:philor)]

Comment on attachment 131299 [details] [diff] [review]
the patch

See comment 7 and comment 8 - Kin's review was "do you really want to have an invisible area where people might type with no spellcheck?" and the answer was no, we don't.

Canceling the request to avoid waves of stale review request triagers.

Comment 18

[Ronald Killmer]

I have a possible design for resolving this spellcheck bug.

Presently spellcheck uses two dictionaries, the master and personal dictionaries.

I suggest creation of a third Scratch pad dictionary in RAM that would be populated with the identity of the person replied to and the content of the Sig block. This gives spellcheck a third source of known good words.

Then when the reply is sent, the scratch pad is flushed to be ready for the next reply to be composed.

Comment 19

[Scott MacGregor]

Wasn't this fixed by Bug 280826 a long time ago? When I reply to a message, the person's name in the citation field isn't spell checked.

Comment 20

[Ronald Killmer]

Sorry Scott but I still see the original behavior that you generated this bug for. Tb 2.0.0.6 release.  I do not know about trunk, as I have not used one of them since the 0.4 release.  OS Win ME.  Saw this was still open when searching spellcheck bugs because I may have a new one in this build. Still testing.

Comment 21

[Joe Sabash [:JoeS1]]

I see this In current trunk also, the name in the citation field is spell checked but only if you have the option ticked to "spellcheck before you send"

Comment 23

[Kris Linquist]

Thanks for pointing me to the right bug Phil.  Bug still exists in 2.0.0.9, both in Win and OSX.

Comment 24

[Ronald Killmer]

Went and looked at Bug 280826 referenced in Comment #19.  That bug was patched by generating an 'ignore list' for inline spell check.  That patch does not make an ignore list that is also usable by 'spell check on send' if the 'Inline' option is not also a selected preference.

This bug is still unresolved for the case of only the 'check on Send' being the users preference.  This bug needs an ignore list that is auto populated with the %s variable for name of original sender within the reply header.  Whether this will need to be expanded to ignore message ID depends on how that new variable gets added to reply headers in Tb 3.

Comment 25

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Nominating wanted-thunderbird3 to keep it on the radar.

Comment 26

[David :Bienvenu]

changing summary to reflect remaining open issue.

Comment 27

[Mitra Ardron]

This bug is still active, i.e. replies typically have the "On xx/xx/xx Yyy Zzz wrote: 

and the Zzz usually triggers a spelling error. 

I also notice that "Forward" is tested differently, i.e. the body is also checked for errors. I think it should be treated the same - or is that a different bug?

Comment 29

[Or Schiro]

Hi there,

Thanks for trying to improve the spell-check!

As an additional request, may I request to also exclude the signature from spell-check? Maybe through an optional checkbox or about:config entry?

Robert