1570719 - Speller checks forwarded content, should ignore just like citations in replies

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Greg Crider]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

requested by Jorg Knobloch to enter this as new bug; when forwarding emails, always spellchecks forwards; according to Jorg this should not happen, but it's been happening for quite a while through several versions; Might be helpful to add ability to enable/disable spellcheck on forwarding

Actual results:

Forwarding emails always stops at spellcheck; is unnecessary step
using Thunderbird for Mac 60.8.0, but has happened for a while through multiple versions; Mac Book Pro 2011, macOS 10.13.6

Expected results:

forwarding to go through without spellcheck window popping up

Comment 1

[WaltS48 [:walts48]]

Possible duplicate of bug 370346, or bug 389351, or bug 399791?

Then there is bug 7994755, a not fixed bug that appears to be requesting that sort of behavior and depends on bug 569397.

I may have missed some in this list https://mzl.la/2CiGXtQ

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1570719-no-spell-forward.patch - WIP - not working

I thought something like this might work, but no so :-(

Masayuki-san, do you know why blockquotes in e-mail replies aren't spellchecked? I can't find the code now but I think I saw it somewhere.

Comment 3

[Jorg K (CEST = GMT+2)]

Masayuki-san, I found it:
https://searchfox.org/mozilla-central/rev/b38e3beb658b80e1ed03e0fdf64d225bd4a40327/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1046

Looks like <blockquote type="cite"> and <pre type="moz-signature" aren't checked>.

Hmm, that latter is for plaintext signatures, but <div class="moz-signature"> are spell-checked. That's pretty wrong. I'll do a patch now.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1570719-skip-mailnews-stuff-in-spellcheck.patch

OK, this is an M-C patch, but since the bug is in TB, I can attach it like this and ask for review. Or do you prefer Phab?

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: Bug 1570719 - Skip more elements in spellcheck for MailNews.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1570719-skip-mailnews-stuff-in-spellcheck.patch

Changed the commit message.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

(In reply to Jorg K (GMT+2) from comment #4)

Created attachment 9082437 [details] [diff] [review]
1570719-skip-mailnews-stuff-in-spellcheck.patch

OK, this is an M-C patch, but since the bug is in TB, I can attach it like this and ask for review. Or do you prefer Phab?

Yes, you need to use Phab in my understanding. All patches should be landed via Lando.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Comment on attachment 9082439 [details] [diff] [review]
1570719-skip-mailnews-stuff-in-spellcheck.patch

>diff --git a/extensions/spellcheck/src/mozInlineSpellChecker.cpp b/extensions/spellcheck/src/mozInlineSpellChecker.cpp
>--- a/extensions/spellcheck/src/mozInlineSpellChecker.cpp
>+++ b/extensions/spellcheck/src/mozInlineSpellChecker.cpp
>@@ -1053,22 +1053,29 @@ boolIsAnyOfHTMLElement mozInlineSpellChecker::ShouldSpellC
>   if (aTextEditor->IsMailEditor()) {
>     nsIContent* parent = content->GetParent();
>     while (parent) {
>       if (parent->IsHTMLElement(nsGkAtoms::blockquote) &&
>           parent->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,
>                                            nsGkAtoms::cite, eIgnoreCase)) {
>         return false;
>       }
>-      if (parent->IsHTMLElement(nsGkAtoms::pre) &&
>+      if ((parent->IsHTMLElement(nsGkAtoms::pre) ||
>+           parent->IsHTMLElement(nsGkAtoms::div)) &&

You should use `IsAnyOfHTMLElements()` instead.

>           parent->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::_class,
>                                            nsGkAtoms::mozsignature,
>                                            eIgnoreCase)) {
>         return false;
>       }
>+      if (parent->IsHTMLElement(nsGkAtoms::div) &&
>+          parent->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::_class,
>+                                           nsGkAtoms::mozfwcontainer,
>+                                           eIgnoreCase)) {
>+        return false;
>+      }
> 
>       parent = parent->GetParent();
>     }
>   } else {
>     // Check spelling only if the node is editable, and GetSpellcheck() is true
>     // on the nearest HTMLElement ancestor.
>     if (!content->IsEditable()) {
>       return false;
>diff --git a/xpcom/ds/StaticAtoms.py b/xpcom/ds/StaticAtoms.py
>--- a/xpcom/ds/StaticAtoms.py
>+++ b/xpcom/ds/StaticAtoms.py
>@@ -44,17 +44,19 @@ STATIC_ATOMS = [
>     Atom("mozdisallowselectionprint", "mozdisallowselectionprint"),
>     Atom("mozdonotsend", "moz-do-not-send"),
>     Atom("mozeditorbogusnode", "_moz_editor_bogus_node"),
>     Atom("mozgeneratedcontentbefore", "_moz_generated_content_before"),
>     Atom("mozgeneratedcontentafter", "_moz_generated_content_after"),
>     Atom("mozgeneratedcontentmarker", "_moz_generated_content_marker"),
>     Atom("mozgeneratedcontentimage", "_moz_generated_content_image"),
>     Atom("mozquote", "_moz_quote"),
>+    # The following two are used in Mailnews.
>     Atom("mozsignature", "moz-signature"),
>+    Atom("mozfwcontainer", "moz-forward-container"),

I think that the comment should be added to end of each line, like "used by mailnews", and you should keep the order as a to z.

And please request review from Phabricator for patches touching mozilla-central.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 9082439 [details] [diff] [review]
1570719-skip-mailnews-stuff-in-spellcheck.patch

Thanks, Masayuki-san, I'll upload a new revision into Phab.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1570719-skip-mailnews-stuff-in-spellcheck.patch - Same patch as in Phab

Carrying over Masayuki's review from Phab. Please leave the reviewer "as is" in the patch.

Dear Sheriff,
please land this the "old-fashioned" way. As you know, patches uploaded via the Phab wrb UI can't be landed in Lando, see bug 1553124 comment #4 for details.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

That patch cannot be landed with Lando, it claims "Repository is not supported by Lando" but I am not sure if that is not from the patch format. Can you resubmit it with this in your .hgrc, please?

[diff]
git = 1
showfunc = 1
unified = 8

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Nevermind, saw the old style patch.

Comment 13

[Jorg K (CEST = GMT+2)]

The issue with Lando is described in comment #10. I keep pasting the same text into each M-C bug I work on. The ultimate explanation is in bug 1529506 comment #4.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a0763d975aa
Skip more elements in spellcheck for MailNews. r=masayuki

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8a0763d975aa

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 9082546 [details] [diff] [review]
1570719-skip-mailnews-stuff-in-spellcheck.patch - Same patch as in Phab

Beta/Release Uplift Approval Request

• User impact if declined: Mail signatures and forwarded parts are spellchecked. They should be ignored just as citation in replies.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Doesn't affect Firefox at all. Simple code addition for Thunderbird. I'll put it onto our branch on TB 68 ESR. Nice to have on beta, but not essential since it's been broken from day one.
• String changes made/needed: None.

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9082546 [details] [diff] [review]
1570719-skip-mailnews-stuff-in-spellcheck.patch - Same patch as in Phab

Too late in the cycle for taking that fix into beta, let's have it ride the trains.

Comment 18

[Jorg K (CEST = GMT+2)]

Hmm, as I said, that code doesn't even run in Firefox, see:
https://hg.mozilla.org/mozilla-central/rev/8a0763d975aa#l1.4

Anyway, your call, I'll take if for TB 68 ESR onto our own branch.

Comment 19

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/09b929d34cbee7b747485c5977663726d321cef0 on THUNDERBIRD_68_VERBRANCH

Comment 20

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/66dd0d3bd890d15c14f34183929261fa47cc360a on THUNDERBIRD_68_VERBRANCH for >= 68.1