Open
Bug 216235
Opened 21 years ago
Updated 2 years ago
Spell Check should ignore reply header text on check before send
Categories
(Thunderbird :: Message Compose Window, defect, P3)
Thunderbird
Message Compose Window
Tracking
(Not tracked)
NEW
Thunderbird 3.0rc1
People
(Reporter: mscott, Unassigned)
References
Details
Attachments
(1 file)
(deleted),
patch
|
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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•21 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird0.2
Reporter | ||
Comment 1•21 years ago
|
||
I'll talk to Kin about this for thunderbird 0.3
Target Milestone: Thunderbird0.2 → Thunderbird0.3
Reporter | ||
Comment 2•21 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•21 years ago
|
||
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•21 years ago
|
Attachment #131299 -
Flags: review?(kin)
Reporter | ||
Updated•21 years ago
|
Attachment #131299 -
Flags: superreview?(bienvenu)
Updated•21 years ago
|
Attachment #131299 -
Flags: superreview?(bienvenu) → superreview+
Comment 4•21 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•21 years ago
|
||
Yup, this is generic; changing to All.
OS: Windows XP → All
Hardware: PC → All
Updated•21 years ago
|
QA Contact: asa
Reporter | ||
Comment 6•21 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.
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•21 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....
Comment 9•20 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•20 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•20 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•20 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•19 years ago
|
||
See also Bug 190660 for a related issue.
Comment 14•19 years ago
|
||
*** Bug 309602 has been marked as a duplicate of this bug. ***
Comment 15•18 years ago
|
||
Comment on attachment 131299 [details] [diff] [review]
the patch
Scott, does Kin still do reviews? This patch has been open for 3,5 years.
Updated•18 years ago
|
QA Contact: message-compose
Comment 16•17 years ago
|
||
I don't think so...
Comment 17•17 years ago
|
||
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•17 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•17 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•17 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•17 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"
Comment 23•17 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•17 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.
Comment 25•17 years ago
|
||
Nominating wanted-thunderbird3 to keep it on the radar.
Flags: wanted-thunderbird3?
Updated•16 years ago
|
Flags: wanted-thunderbird3? → wanted-thunderbird3+
Comment 26•16 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•16 years ago
|
Priority: -- → P3
Target Milestone: Thunderbird0.3 → Thunderbird 3.0rc1
Updated•16 years ago
|
Assignee: mscott → nobody
Status: ASSIGNED → NEW
Comment 27•15 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?
Updated•11 years ago
|
Comment 29•10 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
Updated•9 years ago
|
Updated•9 years ago
|
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•