Closed Bug 880261 Opened 7 years ago Closed 6 years ago

Attachment reminder triggers on "attachment" in a (formatted) URL

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 32.0

People

(Reporter: mikeh, Assigned: mkmelin)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

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).
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.
OS: Linux → All
Hardware: x86_64 → All
Version: 17 → Trunk
Blocks: 244455
(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
Attached patch proposed fixSplinter Review
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #8398916 - Flags: review?(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?
Attachment #8398916 - Flags: review?(bwinton) → review-
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 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...
Attachment #8398916 - Flags: review- → review?(bwinton)
Comment on attachment 8398916 [details] [diff] [review]
proposed fix

Okay, that sounds reasonable.  r=me, and thanks for investigating it!
Attachment #8398916 - Flags: review?(bwinton) → review+
https://hg.mozilla.org/comm-central/rev/a56084a67047 -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 32.0
Depends on: 1201273
You need to log in before you can comment on or make changes to this bug.