880261 - Attachment reminder triggers on keywords like "attachment" in links / URLs

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Mike Habicher [:mikeh] (high bugzilla latency)]

Observed in Ubuntu's 17.0.6.

STR:
1. Open a new compose window
2. Move to the body of the new email
3. Press Ctrl+L and press Tab to move to the "Link Location" text box
4. Paste in, e.g., https://bugzilla.mozilla.org/attachment.cgi?id=759006&action=diff
5. Click OK

Observed behavior: the Attachment Reminder button bar appears at the bottom of the compose window.

Expected behaviour: the compose window should probably ignore "attachment" and other attachment reminder trigger words if they occur in formatted URLs (and maybe unformatted ones as well).

Comment 1

[rsx11m]

Same on Windows and on trunk. I thought that URLs aren't parsed?

It's sufficient already to just paste the link into the composed text for auto-linkification, works in either plain-text or HTML composition mode and prompts for the attachment.

Comment 2

[Magnus Melin [:mkmelin]]

(In reply to rsx11m from comment #1)
> Same on Windows and on trunk. I thought that URLs aren't parsed?

They aren't supposed to be included no:
http://mxr.mozilla.org/comm-central/source/mail/base/modules/attachmentChecker.js#71

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2085

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8398916 [details] [diff] [review]
proposed fix

Review of attachment 8398916 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going with r-, because of the bigger question below.  If there's a good reason for it, then I could be convinced to change it to an r+ (with nits fixed).

::: mail/base/modules/attachmentChecker.js
@@ +72,5 @@
> +    else {
> +      var start = IsCJK(kw.charCodeAt(0)) ? "" : ("((^|\\s)\\S*)");
> +      var end = IsCJK(kw.charCodeAt(kw.length - 1)) ? "" : "(" + NOT_W + "|$)";
> +      var re = new RegExp(start + kw + end, "ig");
> +      var matching;

So, since these are the same as the variables above (because we're using "var" not "let", I think we should either manually hoist them to make that clear, or, even better, change them to "let".  :)

I also wonder if there's a way to merge these two cases, so that we don't make this mistake next time?

Comment 5

[Magnus Melin [:mkmelin]]

The reason they are var and not let is because let doesn't work. I don't recall if anyone knows why, I do recall it's been brought up before somethings. Maybe it's just something to do with it being a worker...? I changed declaration to be inline since it's really painful to have to keep state in mind ("oh, that was actually used outside?") when really just used locally.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 8398916 [details] [diff] [review]
proposed fix

Review of attachment 8398916 [details] [diff] [review]:
-----------------------------------------------------------------

Found bug 487070 regarding the use of let in web workers. Apparently there is now a pref dom.workers.latestJSVersion that should let it work, but we would have to do it globally and the win is almost nothing, so I'd just keep the vars.

As for merging the cases - it doesn't look to be easily doable...

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8398916 [details] [diff] [review]
proposed fix

Okay, that sounds reasonable.  r=me, and thanks for investigating it!

Comment 8

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/a56084a67047 -> FIXED

Comment 10

[Thomas D. (:thomas8)]

Adjusting summary to match the new behaviour implemented here, and improve retrievability by adding keyword "keywords".