498519 - make attachment reminder an inline notification bar

Status: VERIFIED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: inline attachment reminder

This is a follow up polish to bug 244455.

Coming out of bug 244455 comment #36 where it was suggested to use an inline notification bar at the bottom of the compose window instead of a a dialog that pauses you from sending.

Ignore the color and wording of these
http://clarkbw.net/designs/clippy-helper/inline/clippy-helper-0.png
http://clarkbw.net/designs/clippy-helper/inline/clippy-helper-1.png
http://clarkbw.net/designs/clippy-helper/inline/clippy-helper-2.png

Also see attachment

I'm not sure what the exact text is but as bug 492438 explains we want something that will explain why it was triggered.  Something along the lines of:

"You mentioned '$word' and don't have any attachments"

With two buttons:

( Edit Options ) from bug 492573 - though I'd like to make this less prominent than the add attachment button.

( Add Attachment ) - simply opens the add attachment dialog

This notification bar should disappear automatically if the person attaches a file without clicking on it as well as if they attach a file after clicking ( Add Attachment ) otherwise it should stick around until it is closed or the message sent.

Comment 1

[Magnus Melin [:mkmelin]]

Doesn't this go against the whole purpose of the reminder?
It's never that i don't know i should add an attachment; it's that i know, but forgot. At least for me, i doubt a notification bar would make much difference.

Comment 2

[Ben Bucksch (:BenB)]

My first reaction was the same, but the infobar would be underneath the Send button, so there's a good chance to see it when you hit Send. OTOH, for the reasons you mentioned, I may ignore it at first when writing them mail, then get used to ignoring it, so that I forget it anyways. I don't know how/whether it would work out.

An alternative is what I suggested in bug 244455 comment 37: Use some trigger like mouseover/hover over Send button, or leaving/onblur of the body textfield, to run the attachment recognition at this moment (that's assuming the detector completes within <50ms), and if we want to remind, change the Send button to a different, animated icon which has an attachment icon and a "?" somehow animated (on top of the usual Send button icon).

Technically, that should be even easier: For the infobar, we'd need to detect the attachment words while the user types, which may be a complicated code change. Right now, we run the detector only once, when the user clicked Send. To change this to some other trigger (like hover or onblur of some widget) wouldn't be too hard. The remind action would also be trivial to implement: Just set an attachmentreminder="true" attribute on the Send button, and change the icon (and maybe do other stuff) in CSS.

For trigger actions, I could imagine:
- hover of Send button
- hover of the whole Composer toolbars
- onblur of the body textfield (i.e. the focus leaving the
  main text compose part of the window)
- mouse leaves the body textfield
- <insert your idea>

Comment 3

[Ben Bucksch (:BenB)]

The animation should be very catchy/"jumpy". If we feel that's still too subtle, we could pop up a tooltip (without delay), or even additionally add the infobar   Bryan suggests.

I think the key is to choose the trigger wisely, to not come up while the user types (that's the whole issue: the user doesn't attach immediately, because he wants to finish typing first), but come up at an appropriate moment when he can attach. OTOH, not too late of course (just Send button hover is probably too late), and not too early, which would annoy people.

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

The problem with the dialog, as I see it, is that when we're wrong it's annoying; this does not make people like us.  If we're wrong but not in the way I don't think people mind as much, we were trying to be helpful but just like the spell checker we're not always right.

I assume that we're going to be wrong some what often since it's only doing a simple word scan.  I would like to improve the scanning system as well but I also think we can improve the way we remind people.

Attachment reminding as I've realized now is very similar to spell checking; if the inline system works well people will correct it and some people will want an option to not let them send without fixing errors.  However if our inline check is good enough we don't need to default to a warning dialog before send.

I probably should have titled this "improve the inline checking for attachments".

What I think we need here is a combination of what Ben is talking about with good triggers for our inline suggestion and a good inline notification.  I like the notification bar because it animates it way in and I believe that if we have it include the word you typed in *bold* then it will be pretty obvious what it is talking about.

I chose the location at the bottom of the compose window because I wanted to avoid shifting the users text and buttons around.  If we put a notification bar in the top area, like near the send button, we'll have to be very careful that our trigger methods are watching for mouse action to prevent moving things and causing miss-clicks.  

I think if our animation is sufficient then the bottom area will work well.  In my tests it wasn't hard to have the bar animate in as a yellow warning background and then fade to a normal attachment bar seen in the mockup.

Comment 5

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

Attachment: First implementation of UI ideas.

So, here's a first cut at the UI.  I think there are some improvements to be made, but it should be good for people to play around with, and suggest ways to make it better.

Some notes:
* I haven't been able to include the word(s) you typed in *bold*.  I'll keep working on that, if you feel it's important.
* I've gone with Magnus' wording from https://bugzilla.mozilla.org/show_bug.cgi?id=492438#c1
* I'm still letting people send without fixing errors, but I've removed the warning dialog.
* The triggers are okay, I think.  Basically it pops up when the content changes, an attachment is added or removed, or you click in the box.
* I'm not currently saving the previous notification, so if you close the notification, and continue typing, it'll pop up again.  That should be easy enough to change, if we want to.
* I'm also not fading to a normal attachment bar.

So, let me know what you think, and what I should change to make it even nicer to use.

Thanks,
Blake.

Comment 6

[Magnus Melin [:mkmelin]]

After some thought I'm not sure specifying what matched is important  especially if we're going to have an inline reminder. 

I'd also suggest the notification should let you say yes/no i won't add an attachment later, and then if yes, and you try to send without, you should get the alert.

The inline reminder could look like

+-------------------------------------------------------------------------------
| Remind me to add an attachment!  [ Configure... ] [ Sure ] [ No Thanks ] [ x ]
+-------------------------------------------------------------------------------

Just closing it, or no action could act as today.

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

Ok, I've had a chance to work with this all day today it's looking good so far.  Here are my thoughts.

The animation is plenty of notification, it's actually a little surprising when it does come in but I can't imagine it would be missed.  I'd like to work on some kind of a timeout or something that will delay the notification bar a little.  Maybe until there is a pause in typing or a space is typed.  At least for me I typed "attachment" and got two notifications for "attach" and then it went away and another for "attachment".  I'm thinking if we listened for word separators then we'd have a better idea they are done typing a trigger word.

I like the idea of "add now" or "remind later" and I think that's a good direction to try for this notification.  I'm not sure people will really need the remind later option but it might be a nice touch so I think it's worth trying.  We can just enable the existing dialog when remind later is chosen.

Remind Later could also be "Remind Before Sending" but that's a lot of text.  Also "Check Later" could be used instead of "Remind".  Those are easy to try and change.

I think we could stick to the 'This message contains the word:' as our only text since using action buttons should provide the rest of the context.

+-------------------------------------------------------------------------------
| Your message contains the word: *$WORD* ( Add Attachment ) ( Remind Later ) [x]
+-------------------------------------------------------------------------------

This layout gives you:
* why we are notifying you
* action to add attachment
* action to remind later ( use dialog before sending )
* action to close / ignore

In a similar fashion to the remote image/content notification bar...
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#1762

I'd like to convert the options button into a small text sentence with a link that opens the preference for this.  With smaller, lighter text for the secondary sentence I don't think it distracts too much from the original purpose.

+-------------------------------------------------------------------------------
| This message contains the word: '$WORD'  ( Add Attachment ) ( Remind Later )
|   The list of words can be configured from your <a>preferences</a>
+-------------------------------------------------------------------------------

I agree that we probably don't need to show the word with this inline reminder.  However I think to remove the 'word blame' we'll need to be better at only scanning the words you wrote and improve the scanner with either a much more conservative word list or more advanced scanning technique. (e.g. multi-word matching)  Without the 'word blame' I think we wouldn't really need to link to the preferences for the service either.

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 384240 [details] [diff] [review]
First implementation of UI ideas.

putting this as a minus for now.  I've already added my comments in Comment #7

Comment 9

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

Attachment: The previous patch,

(In reply to comment #7)
> The animation is plenty of notification, it's actually a little surprising
> when it does come in but I can't imagine it would be missed.  I'd like to
> work on some kind of a timeout or something that will delay the notification
> bar a little.  Maybe until there is a pause in typing or a space is typed.

We now display the notification after the user hasn't typed anything for .5 seconds.  (We can tune this, and that's part of why I'm posting this patch.)

> I like the idea of "add now" or "remind later" and I think that's a good
> direction to try for this notification.

I've added something that's a step closer to this.  If you dismiss the notification, it won't pop up again until you add (or remove) one of the keywords it found.

While we're getting feedback, I'll work on the "Remind Later" button.

> I'd like to convert the options button into a small text sentence with a link
> that opens the preference for this.  With smaller, lighter text for the
> secondary sentence I don't think it distracts too much from the original
> purpose.

I've made the whole sentence into a link, but it's not particularly noticeable.

+-------------------------------------------------------------------------------
| This message contains the word: '$WORD'  ( Add Attachment ) ( Remind Later )
|   The list of words can be configured from your <a>preferences</a>
+-------------------------------------------------------------------------------

> I agree that we probably don't need to show the word with this inline
> reminder.

I kind of like the word, and we've got a lot of space at the bottom there.

So, as we discussed, it would be cool to get a try-server build of this, and ask people to test it out to see what they think.  Would you mind taking that on, Bryan?

Thanks,
Blake.

Comment 10

[Bryan Clark (DevTools PM) [:clarkbw]]

This is looking good!  I like the delay, it feels pretty good right now.  Here are some notes.

* We should switch to use the attachment icon for our notification icon instead of the /!\ warning icon.

* To get the $WORD bold you'll probably have to use an hbox and two description elements.

<hbox flex="1">
  <description>This message contains the word:</description>
  <description style="font-weight:bold;">$WORD</description>
</hbox>

The secondary sentence part has some additional margin settings to override in order for it to look normal.

<hbox flex="1" style="font-size: 90%; color: graytext; -moz-margin-start: 1em;">
  <description>The list of words can be configured from your</description>
  <label onClick="" class="text-link">preferences</label>
</hbox>

The "text-link" class makes links look like our standard link display.  I'm not sure about the graytext color as it looks weird with the blue and is hard to read but I wanted the text to standout less than the other text.  Maybe some other combination would work better.

All those inline styles should be in CSS files w/ id selectors since we'll only have one notification bar per document and themes may want to override the look.


I'll figure out how to get us some try server build action.

Comment 11

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

Attachment: Apply most of Bryan's suggestions.

(In reply to comment #10)
> * We should switch to use the attachment icon for our notification icon
> instead of the /!\ warning icon.

Done.

> * To get the $WORD bold you'll probably have to use an hbox and two
> description elements.
> <hbox flex="1"> <description>This message contains the
> word:</description> <description
> style="font-weight:bold;">$WORD</description> </hbox>

Done.

> The secondary sentence part has some additional margin settings to
> override in order for it to look normal.
> <hbox flex="1" style="font-size: 90%; color: graytext; -moz-margin-start:
> 1em;"> <description>The list of words can be configured from
> your</description> <label onClick=""
> class="text-link">preferences</label> </hbox>

I worry about that a little, because it's been my experience that counting
on word order makes it hard for folks to localize...  What do you think
about making it all a link, perhaps in gray, but underlined, or something?

Also, it only lined up when I used "-moz-margin-start: 0em;"

> All those inline styles should be in CSS files w/ id selectors since
> we'll only have one notification bar per document and themes may want to
> override the look.

Done.

> I'll figure out how to get us some try server build action.

Cool, thanks.

Note: There are still some functionality bugs in this patch.  (Specifically, clicking the "Attach" button, then failing to attach a file will dismiss the alert, and it won't come back until you add or remove a keyword.)  This patch is more for getting feedback on the UI.

Thanks,
Blake.

Comment 12

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: slight changes to the patch

I make a couple small changes to the patch (mostly layout/CSS and string changes) just to avoid people commenting on the button access key and other random things.  so here's the patch with my changes.  

I put it up on the try server for building so we can get some testing on it and I'll pump in the URLs of the build into this bug when they come out.

http://tinderbox.mozilla.org/showbuilds.cgi?tree=ThunderbirdTry

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

Still waiting for Windows to finish but here are the Mac & Linux builds

Mac
http://s3.mozillamessaging.com/build/try-server/2009-06-25_12:[e-mail]-bug_498519/[e-mail]-bug_498519-mail-try-mac.dmg

Linux
http://s3.mozillamessaging.com/build/try-server/2009-06-25_12:[e-mail]-bug_498519/[e-mail]-bug_498519-mail-try-linux.tar.bz2

Comment 14

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 385031 [details] [diff] [review]
Apply most of Bryan's suggestions.

clearing my reviews.  I made some slight changes to this that are in attachment 385178 [details] [diff] [review]

It might be good to send a message to the newsgroup looking for some final testing since these try builds have been out for a while.

Comment 15

[Daniel F]

First, quick disclosure: I'm the developer of the Attachment Reminder addon for tbird (https://addons.mozilla.org/en-US/thunderbird/addon/5759), whose functionality has been integrated into tb3 with the fixing of https://bugzilla.mozilla.org/show_bug.cgi?id=244455.

Just did some light testing on the attachment reminder functionality using the linux build posted by clarkbw in comment #13, and it looks pretty good.

A few comments: I love the notification bar UI for attachment reminder. A lot less intrusive than the dialog. 

It would be nice however if there still was a way to show the dialog. In my discussion with bwinton on irc, he suggested another button in the notification bar to "remind me later", which would show the dialog on send. I think that would be great!

Finally, I really think that there should be a way to provide regular expression keywords. Either add a checkbox to the configuration panel to "enable regular expressions", or do something along the lines of what adblock plus does, and have a special marker that means "this filter is a regexp". ABP has regexp filters enclosed in slashes. But it could be any special marker. E.g., bwinton suggested keywords starting with "re:" could be interpreted as regexps - that would work too. The "discoverability" of this approach could be improved through informative tooltips. 

I will add that I feel strongly that this feature should be present. The whole reason I made my extension, even though there were one or two others that did the same thing, is because they didn't support regexps, and thus could not be as smart about the keyword search as I wanted the reminder to be. I've gotten a few comments from the users of my extension praising it precisely because of regexp support. 

So, please consider this, and let me know what you think. :)

I'll do some more thorough testing in a bit and report my findings here.

Comment 16

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #15)
> Just did some light testing on the attachment reminder functionality using the
> linux build posted by clarkbw in comment #13, and it looks pretty good.

Awesome, that's good to hear coming from you.

> A few comments: I love the notification bar UI for attachment reminder. A lot
> less intrusive than the dialog. 
> 
> It would be nice however if there still was a way to show the dialog. In my
> discussion with bwinton on irc, he suggested another button in the notification
> bar to "remind me later", which would show the dialog on send. I think that
> would be great!

Yes, I've been wondering if this would make the most sense as an add-on.  Assuming it was easy to extend our current approach I'd like to look into that route.  I want the attachment reminder to be very simple and I'm having a hard time fitting the 'remind me later' option into the default notification bar.  However an extension could easily offer this 'extra reminder' ability.

If it's not possible to easily do this as an add-on we need to file a bug with the necessary fix.

> Finally, I really think that there should be a way to provide regular
> expression keywords. Either add a checkbox to the configuration panel to
> "enable regular expressions", or do something along the lines of what adblock
> plus does, and have a special marker that means "this filter is a regexp". ABP
> has regexp filters enclosed in slashes. But it could be any special marker.
> E.g., bwinton suggested keywords starting with "re:" could be interpreted as
> regexps - that would work too. The "discoverability" of this approach could be
> improved through informative tooltips. 
> 
> I will add that I feel strongly that this feature should be present. The whole
> reason I made my extension, even though there were one or two others that did
> the same thing, is because they didn't support regexps, and thus could not be
> as smart about the keyword search as I wanted the reminder to be. I've gotten a
> few comments from the users of my extension praising it precisely because of
> regexp support. 
> 
> So, please consider this, and let me know what you think. :)

Maybe I should try your extension to see how it works with regex support.  If the detection is a lot better with regex's then I think we could look into supporting that.  We might try to differentiate them with names like 'smart detectors' and 'detectors'.

I would be concerned with regular people interacting with the regex detectors though.  If someone who didn't know what they were doing was to change the regex a little bit it would be very hard to detect and correct.  With simple words people can understand what is being searched for and turn certain ones off.  However that might just mean that we have a display issue with regex's.

It would be nice to protect the editing of a regex from our casual users.  Having an extension that provides you with the regex editing seems fine however it's not an interface I would want to expose normally.

> I'll do some more thorough testing in a bit and report my findings here.

Thanks that would be great.  I'd like to get these changes landed soon, though I'm not sure we'd make the b3 deadline.  Really good to see you're interested in making this better and feel free to file bugs where extending things like this are difficult.

Comment 17

[Daniel F]

(In reply to comment #16)
> (In reply to comment #15)
> > Just did some light testing on the attachment reminder functionality using the
> > linux build posted by clarkbw in comment #13, and it looks pretty good.
> 
> Awesome, that's good to hear coming from you.

:)

> > A few comments: I love the notification bar UI for attachment reminder. A lot
> > less intrusive than the dialog. 
> > 
> > It would be nice however if there still was a way to show the dialog. In my
> > discussion with bwinton on irc, he suggested another button in the notification
> > bar to "remind me later", which would show the dialog on send. I think that
> > would be great!
> 
> Yes, I've been wondering if this would make the most sense as an add-on. 
> Assuming it was easy to extend our current approach I'd like to look into that
> route.  I want the attachment reminder to be very simple and I'm having a hard
> time fitting the 'remind me later' option into the default notification bar. 
> However an extension could easily offer this 'extra reminder' ability.
> 
> If it's not possible to easily do this as an add-on we need to file a bug with
> the necessary fix.

Well, it seems that there is plenty of space to just add another button to the left of the current "add attachment" button in the notification bar. Does that not sound reasonable to you?

And by the way, here's a UI bug I discovered while resizing the composition window, with the notification bar showing. If window is reduced to a width smaller than the "minimum" notification bar width, the notification bar gets a horizontal scrollbar - and that's nice. But the scrollbar takes up part of the vertical space taken up by the notification bar, so it is half-hidden, and not possible to scroll vertically either. Try it and see.

Either (a), the notification bar should get a vertical scrollbar too, or (b), it should become taller so that it is fully shown even with the horizontal scrollbar in place. I personally would suggest the latter, since a vertical scrollbar on a thin horizontal element like that would look weird... but that's just my first instinct. :)

> Maybe I should try your extension to see how it works with regex support.  If
> the detection is a lot better with regex's then I think we could look into
> supporting that.  We might try to differentiate them with names like 'smart
> detectors' and 'detectors'.

Here are some cases which would demonstrate the extra precision possible with regexps:

You want to trigger on "attach", but don't want to trigger the reminder on words like "attachable", "reattach", "attache" (that e might have an accent), "nonattachment", "unattached", or "semiattached" and stuff like that. If you have a non-regexp matcher simply looking for "attach" (which is what you have now), all of these will trigger a likely-bogus reminder.

You want to trigger on "file" or "files" (I find that's one of the more useful filters for me), but you don't want to trigger on "profile", "defile", "filesystem", "filet", "misfiled", "unfiled", "fileable", and numerous variations thereof. Again, with a non-regexp filter for "file" and/or "files", you trigger a bunch of bogus reminders.

There are a few other common examples that I won't go into such detail with, but perpend simply "photo" vs "photovoltaics" or "photographer", "here is/are ..." vs "there is/are ...", "pictures" vs "picturesque", "pics" vs "tropics", "nootropics", or "olympics". 

In other words, there's simply no way to have as precise a set of filters without regexps as with regexps, in particular because there's no simple way to match on "word boundary" without them. (or you could end up with a dozen different filters, like " photos ", " photos.", " photos: ", " photos; ", etc...)

> I would be concerned with regular people interacting with the regex detectors
> though.  If someone who didn't know what they were doing was to change the
> regex a little bit it would be very hard to detect and correct.  With simple
> words people can understand what is being searched for and turn certain ones
> off.  However that might just mean that we have a display issue with regex's.
> 
> It would be nice to protect the editing of a regex from our casual users. 
> Having an extension that provides you with the regex editing seems fine however
> it's not an interface I would want to expose normally.

I agree completely with what you are saying, that there must be some barrier between "regular" users and regexp keywords. I think (and hope!) that this is a design problem that can be solved. Will throw out some thoughts here.

So, say some keywords have regexp in them, and some don't. (this could be marked with a checkbox on the add/edit keyword dialog, which has ample space for that, e.g.). So then, say a user clicks "edit" on a regexp keyword. Before showing the actual edit dialog, a pre-edit dialog comes up and says something to the effect of "this keyword uses regular expressions, are you sure you know what you are doing?" if user says yes, he proceeds to the real edit box. if user says no, the action is canceled. 

You know how firefox (and thunderbird) has this "are you sure you know what you're doing" window when you try to go to "about:config"? Basically we would use the same idea here. 

Another alternative (i like this less than the above, but putting it out there since it provides even greater shield between regular users and regexps), is to have another about:config key, mail.compose.attachment_reminder_regexp_keywords, which is blank by default, but users can edit it from about:config, and enter regexps, separated by commas or semicolons or whatnot. That way, only "advanced" users can find and edit the regexps, and regular users never even see them anywhere in the gui. Would add a bit of a hassle to advanced users, and it would suck that they couldn't use the nifty keyword editor (did I mention I like the nifty keyword editor? :) ), and you couldn't include any regexp filters in the default filter list.

> > I'll do some more thorough testing in a bit and report my findings here.
> 
> Thanks that would be great.  I'd like to get these changes landed soon, though
> I'm not sure we'd make the b3 deadline.  Really good to see you're interested
> in making this better and feel free to file bugs where extending things like
> this are difficult.

Sure will do. I'll stick to this bug thread for anything relating to the attachment reminder though, if that's ok :)

Speaking of which... here's a quick-fix buglet. You have four keywords with the following: attach, attaching, attached, attachment. All four of these can be replaced with simply "attach", since they all contain it. Without more specific regexps, they are all equivalent in triggering the reminder.

Comment 18

[Magnus Melin [:mkmelin]]

Actually, we now match on word boundaries so "attach" wouldn't match them all. People didn't want a plain match since it caused too much false positives. There are special cases it would be ok of course.

Comment 19

[David Ascher (:davida)]

The current dialog is fairly annoying I find, and this is such an improvement, I think it'd be really good if we could get it into b3.  I guess it's too late to _block_ b3, but getting the current patch in would be good.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 385178 [details] [diff] [review]
slight changes to the patch

This patch has bitrotted. 

Didn't look at it in detail yet, but please add spaces around "=" for places like 
+    msgLabel.id="attachmentPreferences";

Comment 21

[Daniel F]

(In reply to comment #18)
> Actually, we now match on word boundaries so "attach" wouldn't match them all.
> People didn't want a plain match since it caused too much false positives.
> There are special cases it would be ok of course.

ah, indeed that is the case, now that I look at this. that's much better than plain match :)

would be nice to also automatically treat all spaces as \s+, that way if a keyword is "here it is" and you type "here  it is", it still matches.

Comment 22

[Daniel F]

also, found some weird behavior (aka, bug) while playing with this. the message text when composing wraps at some 73 characters. I added a keyword "here it is" to my reminder keywords, then I start typing, and make sure that "here it is" gets word-wrapped in the middle. when that happens, the reminder doesn't pop up. BUT, if you type another matching word after it at some point (e.g., "attach"), then when the reminder comes up, it lists both the "here it is" and the "attach". 

to reproduce the behavior, add the keyword "here it is" to the list of keywords, open a new compose window, and type (or paste) this text into a message compose window:
aoeu aoeu aoeuhsnhtaoeu nstha eou snth oaue snthaeu snt haeou snht here it is help me ha 

Notice that "here it is" gets wrapped after "here", and that the reminder doesn't come up.

Comment 23

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

Attachment: clarkbw's patch, unbitrotted and with mkmelin's suggestions.

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 387280 [details] [diff] [review]
clarkbw's patch, unbitrotted and with mkmelin's suggestions.

The inline reminder does look quite nice. However, as is, this feature isn't useful at all IMO. I really think we need the "Remind Later" or similar button before this patch is really reviewable. As I wrote in comment 6 it's kind of a "duh" experience to be told "you wrote attachment" - yes of course I did, I know what i wrote. There's usually a reason i don't add it at that time. I want to be reminded later if i forget, not told the obvious.

I'm not sure where to put the info about matching word, but currently the reminder has too much text and too little functionality. Maybe we could have a [configure...] button and have the matching words passed (and shown/highlighted in the list?) to the keywords dialog. BTW, the current button label also needs "..."

If I write "attachment" and the erase that, the reminder goes away and doesn't come back even if i write it again. 

>+  var keywordsFound = [];
>+  for (var i = 0; i < keywordsArray.length; i++)
>+  {
>+      var kw = escapeRegxpSpecials(keywordsArray[i]);
>+      var re = new RegExp("(([^\\s]*)\\b|\\s*)" + kw + "\\b", "i");
>+      var matching = re.exec(mailData);
>+      // Ignore the match if it was a URL.
>+      if (matching && !(/^http|^ftp/i.test(matching[0])))
>+        keywordsFound.push(keywordsArray[i]);
>+  }
>+  return keywordsFound;

Use 2 space indent, and make those |let|

There's also a few places you should swith to single quotes.

Use the plural js module to handle attachmentReminderSingleMsg/attachmentReminderMultipleMsg (if we use that)

Comment 25

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

Attachment: The previous patch, with most of mkmelin's suggestions.

(In reply to comment #24)
> (From update of attachment 387280 [details] [diff] [review])
> The inline reminder does look quite nice. However, as is, this feature isn't
> useful at all IMO. I really think we need the "Remind Later" or similar button
> before this patch is really reviewable.

Added.

> I'm not sure where to put the info about matching word, but currently the
> reminder has too much text and too little functionality. Maybe we could have a
> [configure...] button and have the matching words passed (and shown/highlighted
> in the list?) to the keywords dialog.

I would like to save that for beta 4/polish, if you don't mind.

As it is (and I realize that it's not obvious), you can click the
"Attachment reminder words can be configured in your preferences" text to
open the preferences.

> BTW, the current button label also needs "..."

Fixed.

> If I write "attachment" and the erase that, the reminder goes away and doesn't
> come back even if i write it again. 

Fixed.

> >+  for (var i = 0; i < keywordsArray.length; i++)
> >+  {
> >+      var kw = escapeRegxpSpecials(keywordsArray[i]);
> >+      var re = new RegExp("(([^\\s]*)\\b|\\s*)" + kw + "\\b", "i");
> Use 2 space indent,

Fixed.

> and make those |let|

I tried that initially, but whenever I change them, I get the error:
Attachment Notification Worker error!!! missing ; before statement
when I try to launch it as a worker.

> There's also a few places you should switch to single quotes.

There are?  I checked, but all of the occurrences of '\"' (which would
escape the ", causing me to want to use 's) were actually '\\"' (escaping
the \, and ending the string, which I don't think I want to change.)

I did switch a couple of places from ' to ", though.

> Use the plural js module to handle
> attachmentReminderSingleMsg/attachmentReminderMultipleMsg (if we use that)

Fixed.

Thanks for the review,
Blake.

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 387660 [details] [diff] [review]
The previous patch, with most of mkmelin's suggestions.

This is looking quite good now! There's just a few minor things

>+ * @returns the (possibly-empty) list of attachment keywords in this message.

AFAIK the correct tag is actually @return (at least in java). Though I know @returns is used too...

>+ **/
>+function GetAttachmentKeywords(mailData,keywordsInCsv)
>+{
>+  // empty keywords is still going to get split to an array of size 1.

Capitalize empty

>+  // Avoid that...
>+  var keywordsArray = (keywordsInCsv) ? keywordsInCsv.split(",") : [];
>+
>+  function escapeRegxpSpecials(inputString)
>+  {
>+    const specials = [".", "\\", "^", "$", "*", "+", "?", "|",
>+                      "(", ")" , "[", "]", "{", "}"];
>+    var re = new RegExp("(\\"+specials.join("|\\")+")", "g");
>+    return inputString.replace(re, "\\$1");
>+  }
>+
>+  var keywordsFound = [];
>+  for (var i = 0; i < keywordsArray.length; i++) {
>+    var kw = escapeRegxpSpecials(keywordsArray[i]);
>+    var re = new RegExp("(([^\\s]*)\\b|\\s*)" + kw + "\\b", "i");
>+    var matching = re.exec(mailData);
>+    // Ignore the match if it was a URL.
>+    if (matching && !(/^http|^ftp/i.test(matching[0])))
>+      keywordsFound.push(keywordsArray[i]);
>+  }
>+  return keywordsFound;
>+}
>+
>+onmessage = function(event)
>+{
>+  var keywordsFound = GetAttachmentKeywords(event.data[0], event.data[1]);
>+  postMessage(keywordsFound);
>+};

I'm not sure what this is, but shouldn't it just be
function onmessage(event) {..... }


>+attachmentWorker.onmessage = function (event)

function(event)

>+    msgText.textContent = PluralForm.get(keywordsFound.length, bundle.getString(

The plurals isn't working, don't know why.

>+    this.lastMessage = keywords;
>+  } else {

Should be
}
else {


>+function ShouldShowAttachmentNotification(async)

Needs documentation. (Especially that it returns stuff.)


>+addAttachmentButton=Add Attachmentâ¦
>+addAttachmentButtonAccessKey=A

should be addAttachmentButton.accessskey
(some l10n tools help with that, i'm told)

>+remindLaterButton=Remind Later
>+remindLaterButtonAccessKey=R

Same here.

> attachmentReminderTitle=Attachment Reminder
> attachmentReminderMsg=Did you forget to add an attachment?
>+attachmentReminderKeywordsMsg=This message contains the word:;This message contains the words:

How about "Found an attachment keyword: ;Found some attachment keywords: 

>+attachmentReminderOptionsMsg=Attachment reminder words can be configured in your preferences

I think we should drop this message altogether for now. (It's also has the win/linux opt vs prefs naming issue.)

Comment 27

[Magnus Melin [:mkmelin]]

(In reply to comment #25)
> > and make those |let|
> 
> I tried that initially, but whenever I change them, I get the error:
> Attachment Notification Worker error!!! missing ; before statement
> when I try to launch it as a worker.

That usually happens when the script isn't recognized as beeing js 1.5 (or later, can't recall). Probably some <script> element that misses the type attribute.

Comment 28

[Magnus Melin [:mkmelin]]

Oh, and perhaps "Remind Me Later" instead of "Remind Later"

Comment 29

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

Attachment: The previous patch, with mkmelin's suggestions.

(In reply to comment #26)
> >+ * @returns the (possibly-empty) list of attachment keywords in this message.
> AFAIK the correct tag is actually @return (at least in java). Though I know
> @returns is used too...

Fixed.

> >+  // empty keywords is still going to get split to an array of size 1.
> Capitalize empty

Done.  (Well, changed to a different sentence, but I capitalized that sentence.)

> >+onmessage = function(event)
> I'm not sure what this is, but shouldn't it just be
> function onmessage(event) {..... }

Hmmm...  Okay, fixed.

Just in case you were interested, it's the function that gets called when I call
attachmentWorker.postMessage([mailData, keywordsInCsv]);
on the main thread.

> >+attachmentWorker.onmessage = function (event)
> function(event)

Fixed.

> >+    msgText.textContent = PluralForm.get(keywordsFound.length, bundle.getString(
> The plurals isn't working, don't know why.

Weird.  I'll look into that.
Okay, I was just replacing the keywords, and not the message.
Fixed.

> >+  } else {
> Should be
> }
> else {

Fixed.

> >+function ShouldShowAttachmentNotification(async)
> Needs documentation. (Especially that it returns stuff.)

Done.  I also changed it to return a boolean, since that seemed simpler both to
use and to document.

> >+addAttachmentButtonAccessKey=A
> >+remindLaterButtonAccessKey=R
> should be addAttachmentButton.accessskey
> (some l10n tools help with that, i'm told)

Done.

> > attachmentReminderTitle=Attachment Reminder
> > attachmentReminderMsg=Did you forget to add an attachment?
> >+attachmentReminderKeywordsMsg=This message contains the word:;This message contains the words:
> How about "Found an attachment keyword: ;Found some attachment keywords: 

Sure.  Sounds good to me.

> >+attachmentReminderOptionsMsg=Attachment reminder words can be configured in your preferences
> I think we should drop this message altogether for now. (It's also has the
> win/linux opt vs prefs naming issue.)

But this is the text that you can click on to go to the pref pane to
configure the attachment words!  I would really rather not lose that
functionality, unless you insist.

(In reply to comment #27)
> > > and make those |let|
> > Attachment Notification Worker error!!! missing ; before statement
> > when I try to launch it as a worker.
> That usually happens when the script isn't recognized as beeing js 1.5 (or
> later, can't recall). Probably some <script> element that misses the type
> attribute.

Yeah, could be.  I'm creating the worker using:
var attachmentWorker = new Worker("resource://app/modules/attachmentChecker.js");
And as far as I can tell, I don't get to specify the type anywhere.

I've asked in #js, and apparently the version of js is "whatever javascript version the browser has".  MsgComposeCommands seems to be getting called with a type of "application/x-javascript".  Changing it to "application/x-javascript;version=1.7" had no effect.

(In reply to comment #28)
> Oh, and perhaps "Remind Me Later" instead of "Remind Later"

Fixed.

Thanks,
Blake.

Comment 30

[Magnus Melin [:mkmelin]]

Comment on attachment 387967 [details] [diff] [review]
The previous patch, with mkmelin's suggestions.

With this latest patch the notification is not shown in full, the lower part is hidden and some scrollbars appear. With full size compose window i don't get that, but with the earlier patches things were fine in a smaller window.

>
>+var EXPORTED_SYMBOLS = ["GetAttachmentKeywords"];

Should be |const|

>+/**
>+ * Get the (possibly-empty) list of attachment keywords in this message.
>+ *
>+ * @return the (possibly-empty) list of attachment keywords in this message.

Don't think non-full sentences in @return should have a period.

Re the attachmentPreferences: notifications usually don't have more than one line, so it looks out of place and should be removed IMO. Let the onclick handler be for the main notification text instead? 
If the user is someone who would configure things, i bet the first guess would be it can be configured from the preferences anyway.

Comment 31

[Magnus Melin [:mkmelin]]

Attachment: screenshot of the problem

Here's what the problem looked like

Comment 32

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

Attachment: The previous patch, with mkmelin's suggestions.

(In reply to comment #30)
> (From update of attachment 387967 [details] [diff] [review])
> With this latest patch the notification is not shown in full, the lower part
> is hidden and some scrollbars appear. With full size compose window i don't
> get that, but with the earlier patches things were fine in a smaller window.

Okay, it took me all weekend, but I think you will like the behaviour now.
It will still show the scroll bars if you make the window too small to
contain both the icons and the buttons, but that seems reasonable to me.
The text will get cropped with "…"s added.

> >+var EXPORTED_SYMBOLS = ["GetAttachmentKeywords"];
> Should be |const|

Fixed.

> >+ * @return the (possibly-empty) list of attachment keywords in this message.
> Don't think non-full sentences in @return should have a period.

Fixed.

> Re the attachmentPreferences: notifications usually don't have more than one
> line, so it looks out of place and should be removed IMO. Let the onclick
> handler be for the main notification text instead? 
> If the user is someone who would configure things, i bet the first guess would
> be it can be configured from the preferences anyway.

Okay, I've changed it.

(And we can polish it more for beta 4, if we decide that we really did want a multi-line notification.)

Thanks,
Blake.

Comment 33

[Magnus Melin [:mkmelin]]

Comment on attachment 388218 [details] [diff] [review]
The previous patch, with mkmelin's suggestions.

This is working really nicely now. Thanks for all the hard work you've put into this!

>+#attachmentNotificationBox notification .messageImage {

Looks like you should be able to make this selector more efficient, like
#attachmentNotificationBox > notification > .messageImage {

>+#attachmentPreferences {
>+  font-size: 90%;
>+  color: graytext;
>+  text-decoration: underline;
>+  -moz-margin-start: 1em;
>+}

attachmentPreferences is now unused

r=mkmelin with that

Comment 34

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

Attachment: The previous patch, with the last of mkmelin's suggestions.

(In reply to comment #33)
> (From update of attachment 388218 [details] [diff] [review])
> This is working really nicely now. Thanks for all the hard work you've put
> into this!

My pleasure!

> >+#attachmentNotificationBox notification .messageImage {
> Looks like you should be able to make this selector more efficient, like
> #attachmentNotificationBox > notification > .messageImage {

I can make it a little more efficient:
#attachmentNotificationBox > notification .messageImage {
But when I tried your example, the image didn't show up.

I think that it's because the notification expands into a bunch of stuff (http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/notification.xml#356 looks like it has the details.)

If that's true, then I could have written:
#attachmentNotificationBox > notification > hbox > hbox > .messageImage {
but that seems like it knows a little too much about the inards of the current implementation of notification, so I went with the slightly-less efficient version above.

> >+#attachmentPreferences {
> >+  font-size: 90%;
> >+  color: graytext;
> >+  text-decoration: underline;
> >+  -moz-margin-start: 1em;
> >+}
> attachmentPreferences is now unused

Removed.

> r=mkmelin with that

Thank you!

Later,
Blake.

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 388220 [details] [diff] [review]
The previous patch, with the last of mkmelin's suggestions.

My impression from skimming this bug is that this really wants another UI review, since the interaction has changed.  That said, if we decide to take this for b3, that can probably happen post facto.

Comment 36

[Mark Banner (:standard8)]

(In reply to comment #35)
> (From update of attachment 388220 [details] [diff] [review])
> My impression from skimming this bug is that this really wants another UI
> review, since the interaction has changed.  That said, if we decide to take
> this for b3, that can probably happen post facto.

Do you realise this patch affects l10n strings?

Comment 37

[Dan Mosedale (:dmosedale, :dmose)]

Yeah, that's why I added the [has l10n impact] whiteboard annotation.  We should indeed only take this if we think we can't stand the current interaction.  My current inclination is not to take it, but I wanted to get it on the list first and make that decision later today or tomorrow.

Comment 38

[Dan Mosedale (:dmosedale, :dmose)]

We've already got mercurial revision tags from a bunch of localizers for 3.0b3 assuming no more strings arrive.  Taking this would fix a problem that is, at worst, an annoyance.  Waving it off to b4.

Comment 39

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 388220 [details] [diff] [review]
The previous patch, with the last of mkmelin's suggestions.

checked this out on the mac and it looks good.

I'm still uncertain about opening the preferences page vs. just the attachment keyword list but the former does allow for turning the feature off.  Also, I think we need a better indicator that you can click on the msg text but I'm not sure what that is.  Perhaps even minimally just using the "help" cursor on hover?

Comment 40

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

(In reply to comment #39)
> I'm still uncertain about opening the preferences page vs. just the attachment
> keyword list

I haven't found a nice way to open the attachment keyword popup.  We can easily show the preference pane that has the button to configure the attachment keywords, but getting the popup to pop up was too hard for the time I had.

If someone has some pointers, I'ld be happy to revisit the code, and try to make it work.

> I think we need a better indicator that you can click on the msg text but
> I'm not sure what that is.  Perhaps even minimally just using the "help"
> cursor on hover?

Hmmm...  That might be good.  Or the hand-that-indicates-that-its-a-link?  Let me see what I can whip up, and I'll post another patch later today or tomorrow.

Thanks,
Blake.

Comment 41

[Ben Bucksch (:BenB)]

> indicator that you can click on the msg text

underline

Comment 42

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

Attachment: [checked in] The previous patch, with clarkbw's suggestions.

(In reply to comment #40)
> I haven't found a nice way to open the attachment keyword popup.  We can
> easily show the preference pane that has the button to configure the
> attachment keywords, but getting the popup to pop up was too hard for the
> time I had.
> 
> If someone has some pointers, I'ld be happy to revisit the code, and try to
> make it work.

I still haven't gotten this working.  The last thing I tried was:
      openOptionsDialog("paneCompose");
      var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
                         .getService(Components.interfaces.nsIWindowMediator);
      var win = wm.getMostRecentWindow("Mail:Preferences");
      win.gComposePane.attachmentReminderOptionsDialog();

But that failed if the preferences window wasn't already open.  I then tried putting it in a timer, but that failed with the error:
…MsgComposeCommands.js, line 1206: win.gComposePane is undefined

Since I haven't changed any code here, I'm assuming the r=mkmelin is carrying over.

> > Perhaps even minimally just using the "help" cursor on hover?
> Hmmm...  That might be good.  Or the hand-that-indicates-that-its-a-link?  Let
> me see what I can whip up, and I'll post another patch later today or
> tomorrow.

Okay, I used "cursor: pointer;" (a.k.a. hand-that-indicates-that-its-a-link ;), so that it really looks like a link.  Give it a try, and let me know what you think.

(In reply to comment #41)
> > indicator that you can click on the msg text
> underline

Nice!  It looked odd to have it under the text as well as the keywords, so I only put it under the keywords.

Thanks,
Blake.

Comment 43

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 391523 [details] [diff] [review]
[checked in] The previous patch, with clarkbw's suggestions.

ok, looks good!

Comment 44

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 391523 [details] [diff] [review]
[checked in] The previous patch, with clarkbw's suggestions.

minus is the new plus!

but only that one time

Comment 45

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/4d0d71a37a42

Comment 46

[Vlado Valastiak [:wladow]]

> +attachmentReminderKeywordsMsg=Found an attachment keyword: ;Found some attachment keywords: 
There's no reason to use pluralForm here unless you define keywords count in the string. And this needs a standard localization note about pluralForm usage. Like this:

# LOCALIZATION NOTE (attachmentReminderKeywordsMsg): Semi-colon list of plural forms.
# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals

Comment 47

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

Attachment: [checked in] A patch to fix the localization issues wladow brought up.

(In reply to comment #46)
> > +attachmentReminderKeywordsMsg
> There's no reason to use pluralForm here unless you define keywords count in
> the string.

Done…

> And this needs a standard localization note about pluralForm usage.

…and done.  :)

I also changed it to attachmentReminderKeywordsMsgs (with an "s" on the end) so that it'll show up as something new to be localized.

Thanks,
Blake.

Comment 48

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

Received an l10n-r+ from wladow in irc.

Comment 49

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

Attachment: RTL works too!

Comment 50

[Mark Banner (:standard8)]

Comment on attachment 391757 [details] [diff] [review]
[checked in] A patch to fix the localization issues wladow brought up.

Checked in: http://hg.mozilla.org/comm-central/rev/8b22cd7aae0d

Comment 51

[Mark Banner (:standard8)]

Comment on attachment 391523 [details] [diff] [review]
[checked in] The previous patch, with clarkbw's suggestions.

This was checked in in comment 45.

Comment 52

[Ludovic Hirlimann [:Usul]]

https://litmus.mozilla.org/show_test.cgi?id=7902
https://litmus.mozilla.org/show_test.cgi?id=7903
https://litmus.mozilla.org/show_test.cgi?id=7904

Comment 53

[Ludovic Hirlimann [:Usul]]

Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.3pre) Gecko/20090802 Shredder/3.0b4pre

Comment 54

[Markus Multrus]

An end-user comment on this: I needed 7 test runs to notice the "inline notification bar" on Windows Vista (TB 3.0B4), and I always wondered why I did not get any warning that I forgot an attachment. 

I have to say that as it is implemented right now, for me this notification bar is useless: I'm pretty much ignorant to the bottom of the composition window, when I focus on a) writing a text (then my eyes follow the cursor) or b) on pressing the send button (then my eyes follow the mouse pointer, which goes directly to the send button at the top of the window: this is exactly the different direction as the notification bar is). Another reason for my ignorance on this notification bar might also be that I'm conditioned to ignore the window bottom, except there is a strong visual stimulus: In MacOSX, you have these jumping logos in the dock, and on Windows items in the task bar start blinking, if they need your attention. Applications usually show "additional, but nor important information" at the bottom of the window: For example MS Word shows there document statistics, Firefox the page-load status, IDEs line numbers etc. Saying all this I have to admit that I'm really conditioned to ignore the very bottom of a window, except there is a strong stimulus.

I'm pretty much in favor of the pop-up dialog, since this would catch my eye immediately, and only when I need it (i.e. when it's already too late to add the attachment without an additional feedback). When it stays with this "inline notification bar", then I would need a stronger visual stimulus e.g. by a stronger color or blinking or... Nevertheless, I personally add attachments at the very end of my composition process, and this would be disturbing for me...

- Markus

Comment 55

[Ben Bucksch (:BenB)]

Yup, the bar should be at the top - near the Send button - not at the bottom.

Comment 56

[Mark Banner (:standard8)]

(In reply to comment #55)
> Yup, the bar should be at the top - near the Send button - not at the bottom.

As always, please file suggested changes in separate bugs and don't attach them to the bottom other fixed bugs.

Comment 57

[Markus Multrus]

Sorry. I filed bug 518215 for further discussion.