Spell Check should ignore reply header text on check before send

NEW
Unassigned

Status

Thunderbird
Message Compose Window
P3
normal
15 years ago
2 years ago

People

(Reporter: Scott MacGregor, Unassigned)

Tracking

unspecified
Thunderbird 3.0rc1
Dependency tree / graph
Bug Flags:
wanted-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

15 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird0.2
(Reporter)

Comment 1

15 years ago
I'll talk to Kin about this for thunderbird 0.3
Target Milestone: Thunderbird0.2 → Thunderbird0.3
(Reporter)

Comment 2

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

Comment 3

15 years ago
Created attachment 131299 [details] [diff] [review]
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".
(Reporter)

Updated

15 years ago
Attachment #131299 - Flags: review?(kin)
(Reporter)

Updated

15 years ago
Blocks: 219336
(Reporter)

Updated

15 years ago
Attachment #131299 - Flags: superreview?(bienvenu)

Updated

15 years ago
Attachment #131299 - Flags: superreview?(bienvenu) → superreview+

Comment 4

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

Comment 5

15 years ago
Yup, this is generic; changing to All.
OS: Windows XP → All
Hardware: PC → All

Updated

15 years ago
QA Contact: asa
(Reporter)

Comment 6

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

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

Comment 8

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

Updated

15 years ago
Blocks: 119232

Comment 9

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

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

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

14 years ago
(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

13 years ago
See also Bug 190660 for a related issue.

Comment 14

13 years ago
*** Bug 309602 has been marked as a duplicate of this bug. ***
Comment on attachment 131299 [details] [diff] [review]
the patch

Scott, does Kin still do reviews? This patch has been open for 3,5 years.
QA Contact: message-compose

Comment 16

11 years ago
I don't think so...
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.
Attachment #131299 - Flags: review?(kinmoz)

Comment 18

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

Comment 19

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

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

11 years ago
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"


Duplicate of this bug: 414988

Comment 23

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

Comment 24

11 years ago
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.
Nominating wanted-thunderbird3 to keep it on the radar.
Flags: wanted-thunderbird3?

Updated

10 years ago
Flags: wanted-thunderbird3? → wanted-thunderbird3+

Comment 26

10 years ago
changing summary to reflect remaining open issue.
Summary: Spell Check should ignore reply header text → Spell Check should ignore reply header text on check before send

Updated

10 years ago
Priority: -- → P3
Target Milestone: Thunderbird0.3 → Thunderbird 3.0rc1

Updated

10 years ago
Assignee: mscott → nobody
Status: ASSIGNED → NEW

Comment 27

9 years ago
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?
Duplicate of this bug: 1107651

Comment 29

4 years ago
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
See Also: bug 1128851
Duplicate of this bug: 1128851
You need to log in before you can comment on or make changes to this bug.