Closed Bug 648256 Opened 10 years ago Closed 8 years ago

No keyboard access for closing attachment reminder notification bar (proposal: ESC)

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: chazen, Assigned: sshagarwal)

References

()

Details

(Keywords: access, ux-efficiency, Whiteboard: [STR comment 14][gs][good first bug])

Attachments

(1 file, 10 obsolete files)

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20110310 Firefox/4.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.14) Gecko/20110223 Thunderbird/3.1.8

This is papercuts category.  When I use a keyword that attachment detection recognizes (attachment, cv, etc) as an attachment word, I get a banner across the bottom of the compose window, "Found an attachment keyword: ...", with the options "Add attachment" and "Remind me later".

But a substantial portion of the time, I don't want an attachment.  And that leaves me bothered -- "stupid Thunderbird... I don't _want_ an attachment, and the only thing that will make this glowing banner go away is to say 'Remind me later'.  I don't WANT it to remind me later, I don't want an attachment!"  

It would be great to have an option to dismiss attachment detection, and not to just defer the prompt to later.

Reproducible: Always

Steps to Reproduce:
1. Enter compose window.  Do not intend to add an attachment.
2. Use an attachment keyword in the body.
3. Notice that Compose window gives you no option to disable attachment detection for this message; and only options to add more clicks by "Reminding me later".


Expected Results:  
When attachment detection kicks in, give the user a way to disable it for a message.
There is an [x] in the lower-right corner to close the bar for a single occurrence, but indeed it doesn't seem possible to disable the feature temporarily for the current message which is composed.

Confirming as a valid RFE, nothing similar seems to have been filed yet.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Summary: Attachment detection in compose window should give me "No attachment" option. → Attachment detection in compose window should give me "No attachment" option for the current message
Version: unspecified → Trunk
Summary: Attachment detection in compose window should give me "No attachment" option for the current message → Cannot dismiss attachment reminder for the current message
Duplicate of this bug: 670273
Desired behavior: <esc> should dismiss the reminder, consistent with Apple interface standards.  [this comment lifted from the duplicate Bug 670273]

From above: "It would be great to have an option to dismiss attachment detection."
Comment: this can be done by the little X box in the lower right corner, but requires a precise mouse movement (time-consuming). Attachment detection can be turned off entirely in Preferences, but that's defeating its purpose (and occasional usefulness).
Attached patch 648256.patch (obsolete) — Splinter Review
Hi, I have a patch for it but I am not sure who should I ask for a review?. Also, I am not sure if I am implementing it correctly. For every keystroke it is checking if the keypressed is ESC. Please let me know if it is fine.
Assignee: nobody → jasonyeo88
Status: NEW → ASSIGNED
Comment on attachment 592592 [details] [diff] [review]
648256.patch

Magnus is probably a good candidate. Sorry for the delay.
Attachment #592592 - Flags: review?(mkmelin+mozilla)
Comment on attachment 592592 [details] [diff] [review]
648256.patch

>     <editor type="content-primary" id="content-frame" src="about:blank" name="browser.message.body" flex="1"
>-            context="msgComposeContext"/>
>+            context="msgComposeContext" onkeypress="if (event.keyCode == 27) document.getElementById('attachmentNotificationBox').currentNotification.close();"/>
>   </vbox>

Shouldnn't inline this, make it a <key> instead. And use VK_ESCAPE not 27. 

However, i don't think esc should dismiss notification bars. Is there any precedence for that? Seems odd to me.
Attachment #592592 - Flags: review?(mkmelin+mozilla) → review-
I think this bug is a bit misunderstood. The attachment reminder doesn't remind again if the [x] is clicked. Or it does, but only if it finds *another* attachment keyword too. Is that really very likely? 
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1424
no longer working on this.
Status: ASSIGNED → NEW
Assignee: yshun → nobody
Bug 521158 adds more manual control to attachment reminder, but not yet what this bug is seeking for, I think.

(In reply to Magnus Melin from comment #7)

> I think this bug is a bit misunderstood. The attachment reminder doesn't
> remind again if the [x] is clicked. Or it does, but only if it finds
> *another* attachment keyword too. Is that really very likely? 
> http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/
> MsgComposeCommands.js#1424

I'm not so sure about the current behaviour, comment in code suggests otherwise, also involves "aggressive" pref... - but whatever it is, it's not very obvious from the current [x] button on reminder notification bar what is (not) going to happen when you click it.
See Also: → 521158
The [x] at the lower right sort of turns off the attachment reminder bar for the particular word, and if a new word is entered (that is in the category of attachment keywords), the bar re-appears with all the words (including the previous dismissed word) in the found words list.
Discussion of this bug has wandered away from the original filing and my comment 2 years ago (see Comment #3 above). To repeat: It is hard for the user to dismiss an unwanted attachment warning, because the only way of doing so is to interrupt keyboard activity (which is what triggers it) and make a precise mouse move to the small target X in the right corner. Doing this takes time and is annoying. I guess this is hard for programmers to understand because it is something you do constantly, but put yourself in the place of the average user. It would be FAR easier to just hit the <esc> key and keep going. In the Mac Finder and in much Mac software <esc> is a keyboard equivalent of <Cancel>. Which is exactly what is desired here.

And in belated reply to Comment #7 (and #9), "Is that really very likely?" -- actually, yes, it happens to me a lot since quite a few key words trigger the attachment reminder, and it is not uncommon for more than one to turn up in an e-mail message I'm typing. So there really IS a need for Thunderbird to have an easier way of dismissing an unwanted attachment warning if this feature is to actually be useful.
(In reply to godfreye@bigw.org from comment #11)
> Discussion of this bug has wandered away from the original filing and my
> comment 2 years ago (see Comment #3 above). To repeat: It is hard for the
> user to dismiss an unwanted attachment warning, because the only way of
> doing so is to interrupt keyboard activity (which is what triggers it) and
> make a precise mouse move to the small target X in the right corner. Doing
> this takes time and is annoying.

+1. Worse, it's an accessability problem because mouse-handicapped people have no way of dismissing the reminder at all.

> I guess this is hard for programmers to
> understand because it is something you do constantly, but put yourself in
> the place of the average user. It would be FAR easier to just hit the <esc>
> key and keep going. In the Mac Finder and in much Mac software <esc> is a
> keyboard equivalent of <Cancel>. Which is exactly what is desired here.

+1. I agree that just hitting ESC would feel perfectly right and intuitive to get rid of something that just crawled into my screen. When typing, there is no other reason to use escape, so there can't be any doubts about the target of pressing ESC in that situation. Of course if there's anything else which should consume ESC first because it's closer to user's current focus of activity (e.g. an open context menu in msg body), as usual we'll consume ESC on those other elements first before sending subsequent ESC presses to the reminder notification bar (and I think that sequence of consumption will actually occur automatically without extra coding).

> And in belated reply to Comment #7 (and #9), "Is that really very likely?"
> -- actually, yes, it happens to me a lot since quite a few key words trigger
> the attachment reminder, and it is not uncommon for more than one to turn up
> in an e-mail message I'm typing. So there really IS a need for Thunderbird
> to have an easier way of dismissing an unwanted attachment warning if this
> feature is to actually be useful.

+1

(In reply to Magnus Melin from comment #6)
> However, i don't think esc should dismiss notification bars. Is there any
> precedence for that? Seems odd to me.

Well, we have very similar behaviour for Quick Filter Bar, which is not a notification bar as such, but suggested use pattern of consuming ESC would be very similar:

STR and behaviour with quickfilter bar:
1 in 3pane, press Ctrl+Shift+K to bring up quickfilter bar
2 in msg list, select single msg
3 press context menu key -> msg context menu pops up
4 press ESC -> context menu goes away
5 press ESC again -> quick filter bar goes away

Having said which, we might have to disentangle this bug.
ESC could be dealt with in bug 670273.

I understand that comment 0 of this bug seeks a new feature, probably a button or dropdown choice from the notification bar, which will dismiss not only the current notification, but all subsequent notifications for the current message, as well as prevent the actual reminder alert when sending for the current message.
Keywords: access
Summary: Cannot dismiss attachment reminder for the current message → Cannot dismiss attachment reminder for the current message; no keyboard access for closing notification bar (proposal: ESC)
I don't think clicking the small close button or hitting ESC should turn off the reminder permanently. It can hide the reminder, but it should appear again at sending.
If you want to kill the reminder permanently, there should be a real "do not remind me again" button that the user needs to click explicitly. If he does not want these reminders ever, it can be turned off in the Options.
(In reply to :aceman from comment #13)
> I don't think clicking the small close button or hitting ESC should turn off
> the reminder permanently. It can hide the reminder, but it should appear
> again at sending.

Per comment 10, clickin [x] button will currently hide notification bar only temporarily; if user types another attachment keyword not yet ignored by user for this msg, notification will return.
But if user has closed the notification and it's still hidden at the time of sending, there'll be no reminder alert even in aggressive mode. Aggressive reminder only kicks in when a notification is ignored, i.e. left untouched and still visible on screen when sending. That's all by design and I think it's good design, because we play safe if you do nothing, but if you just click away the reminder without opting in to be reminded, we should not force the modal reminder on uninterested user. I think offering to be reminded for every new keyword which occurs is more than enough, if that offer isn't accepted, it's user's free choice and we need to respect that. The effect of closing the notification might not be very clear from the [x] tooltip, but certainly user cannot expect to get a full-scale alert when he deliberately ignores "remind me later" option.

> If you want to kill the reminder permanently, there should be a real "do not
> remind me again" button that the user needs to click explicitly. If he does
> not want these reminders ever, it can be turned off in the Options.

+1. Aceman, pls note that this bug covers two different features which are pretty much independent of each other:

1) "Do not remind me for this message": provide way of turning off further notifications (i.e. also reminder alert) just *for the current message*
The easiest solution would probably be a dropdown on the button which is on notification:

Remind me later | v
Do not remind me for this message

2) ESC: Provide keyboard access of closing the current notification
Blocks: 942436
This bug used to cover two independent features which need disentangling (see end of comment 14).
Starting from comment 3, this bug has been morphed into what was bug 670273 about ESC to hide/ignore the notification bar via keyboard (bug 670273 was wrongly duped to here, but it's too late now to reopen, given extensive discussion here). So I've refiled this bug's comment 0 as bug 942436, so we can just continue with ESC here...

Steps to reproduce (based on bug 670273, comment 0):

1) Verify default setting where "Check for missing attachments" is enabled in <Preferences-Composition-General>.
2) Compose new msg
3) Type attachment keyword into body
-> attachment reminder notification bar appears
4) Try to dismiss/hide the notification via keyboard
5) Realize that 4) is not possible and use mouse to hide notification bar


Actual results:

3) with default settings, notification bar appears automagically (keyword-triggered)
4) There is no way to dismiss/hide notfication via keyboard (violating accessibility principles)
5) To dismiss/hide notification with mouse, it's quite a delicate maneuver to exactly hit the teeny-weeny "x" button in the far right bottom corner, which is very aggravating and time-consuming.


Expected results:

4) TB must provide keyboard access for dismissing the automagical notification bar; keyboard shortcut should be intuitive, memorable, and ux-consistent.

Proposed solution/UI:

Add ESC keyboard shortcut for hiding attachment reminder notification bar.

Benefits:
- intuitive: ESC is very intuitive for users, because it's ux-consistent with structurally similar scenarios in TB and other apps (see below)
- memorable: ESC is the easiest possible shortcut to well, escape from unwanted interactions
- ux-consistent: often used in similar scenarios in TB and other apps to get rid of things which have just crawled into your screen. Compare e.g. the existing behaviours of
  - hiding TB's Quick Filter Bar via ESC
  - ignoring TB's attachment reminder alert via ESC (aggressive or manual reminder)
  - hiding/ignoring FF's automagical "Save password" bubble prompt via ESC

Anecdotal user evidence from bug 670273, comment 0:
> [So users can just] hit the <Escape> key to dismiss the reminder when it appears. ...
> Using escape would be fast, and fits Apple programming norms. I tried
> submitting this as a suggestion (not a bug) over a year ago, and was
> disappointed when TBird 5.0 appeared without this behavior; it obviously got
> lost in the shuffle. This change isn't glitzy, but is a major irritant for
> me in using Thunderbird. The attachments reminder is convenient and I like
> to use it even though it rarely applies, but without being able to dismiss
> it easily the only alternative is to disable the reminder option, which is
> too bad. Hope someone can do the small amount of programming to fix this
> problem. Thanks!
No longer blocks: 942436
Severity: enhancement → normal
Summary: Cannot dismiss attachment reminder for the current message; no keyboard access for closing notification bar (proposal: ESC) → No keyboard access for closing attachment reminder notification bar (proposal: ESC)
Whiteboard: [gs] → [Morphed! New STR comment 14][gs]
Blocks: 942436
Also note, per comment 11, that the notification gets triggered by *keyboard* input, lending further support to the binding accessibility argument that we need to provide a *keyboard* way for users to hide the bar, instead of forcing them to interrupt for a mouse-only activity.
Whiteboard: [Morphed! New STR comment 14][gs] → [Morphed! New STR comment 14][gs][good first bug][has draft patch
Attached patch Patch v1 (obsolete) — Splinter Review
Okay, so let's try to end this now!
Attachment #592592 - Attachment is obsolete: true
Attachment #8337393 - Flags: feedback?(bugzilla2007)
Attachment #8337393 - Flags: feedback?(acelists)
Comment on attachment 8337393 [details] [diff] [review]
Patch v1

Great! So simple, so good.

For polishing, I think we might want to restrict ESC for closing notification so that it only works while focus is in body or on the notification itself.
I think at least I wouldn't want it to trigger when focus is in recipients fields.

Although technically there should be no problem to make all of composition ESC-sensitive for closing notification (assuming that all other ESC events like closing dropdowns etc will always be consumed first), it might be both unexpected and slightly error-prone if one ESC too much in recipient autocomplete will remove the distant notification. Put positively, notification occurs when typing keywords in body, so that's the most likely scenario where users will want to hide the notification immediately after it has appeared.
Attachment #8337393 - Flags: feedback?(bugzilla2007) → feedback+
Comment on attachment 8337393 [details] [diff] [review]
Patch v1

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

I do not like that all the code is put directly into the oncommand attribute. Can you make a function for it and make the same function be called also when clicking the X on the notification? Or is this not the point of this bug? Isn't the behaviour intended to be the same? Is ESC to only close the notification temporarily (until a new keyword is input) and the X is intended to be permanent? Even in that case I'd like a common function taking an argument saying if the hiding should be permanent.
Attachment #8337393 - Flags: feedback?(acelists)
(In reply to :aceman from comment #19)
> Comment on attachment 8337393 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8337393 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I do not like that all the code is put directly into the oncommand
> attribute. Can you make a function for it and make the same function be
> called also when clicking the X on the notification? Or is this not the
> point of this bug? Isn't the behaviour intended to be the same?

Yes, it should be the same.

> Is ESC to
> only close the notification temporarily (until a new keyword is input) and
> the X is intended to be permanent?

No, we can't create a difference of behaviour between ESC and [x], and also current behaviour of [x] looks good behaviour to me. [x] == ESCape == lightweight closure, more like "ignore for now".
Semi-permanent dismissal of notification so that it does not come back for the current message is planned for bug 942436, with an explicit option of "Do not remind me for this message" in the dropdown of [Remind me later] button.

> Even in that case I'd like a common
> function taking an argument saying if the hiding should be permanent.

Common function sounds good, perhaps even a command like cmd_closeAttachmentReminderNotification?
Things like command disabling might help to ensure that ESC does not trigger errors when notification is not around but ESC is pressed.
Aceman, could you comment on the proposed behaviour of comment 18?

By analogy, note that even for QuickFilter bar, ESC will not work when focus is in Global Search Box (because that's unrelated and would be confusing), so practically it works mainly inside the two elements which are most affected by the filter, namely folders list and message list.

(In reply to Thomas D. from comment #18)
> Comment on attachment 8337393 [details] [diff] [review]
> Patch v1
> 
> For polishing, I think we might want to restrict ESC for closing
> notification so that it only works while focus is in body or on the
> notification itself.
> I think at least I wouldn't want it to trigger when focus is in recipients
> fields.
Likewise imo there's not much point for ESC from contacts side bar to hide notification, but that would be more acceptable and less risky than from recipients.
> [snip]
^^
Flags: needinfo?(acelists)
Yes, I'd agree with the ESC only works when focus is in the body. If needed, it should be possible to check which element is focused (I remember in one of my bugs I needed that).
Flags: needinfo?(acelists)
Attached patch Patch v2 (obsolete) — Splinter Review
Is this acceptable?
Attachment #8337393 - Attachment is obsolete: true
Attachment #8338575 - Flags: feedback?(acelists)
Comment on attachment 8338575 [details] [diff] [review]
Patch v2

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

Yes, this works great. If calling the command of the notification button didn't work, then this is acceptable.
Attachment #8338575 - Flags: feedback?(acelists) → feedback+
Comment on attachment 8338575 [details] [diff] [review]
Patch v2

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

I'm happy with Suyash rapid resolve for problems :)

But I have some nits...

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1779,5 @@
>    dump("Attachment Notification Worker error!!! " + error.message + "\n");
>    throw error;
>  };
>  
> +function hideNotification()

This function is in an odd place:
I'd think we don't want to separate attachmentWorker.onerror and attachmentWorker.onmessage with a non-attachmentWorker function.

This function name is very misleading:
- it's not hiding *any* notification (as the name suggests), but only *attachment* notification.
- it's not even reliably hiding attachment notification, but only IF the focus is on content-frame. Which means it can't be used from anywhere else, and it's a special-case function designed for ESC. that matters because this will create confusion if coders try to reuse this for just hiding notification, which won't work as expected from function name.

So to adequately describe the current content of the function, the name would have to be hideAttachmentNotificationOnESC()

Odd? Perhaps yes, but maybe it's the function which isn't as good as it could be yet. As aceman said, it's desirable to call the same function from [x] button on notification (and possibly other spots), but that won't work with the current design of this function.

Furthermore, with this patch, can I press ESC to hide the bar when focus is *on* the notification bar? Probably not, but that should work, too. (Use shift+tab to put focus on notification bar buttons without hiding bar).

I'd imagine something like the following structure (quick draft, not tested, not yet functional, pls adjust):

<key keycode="VK_ESCAPE" oncommand="handleESC();"/>

function handleESC() {
  if (document.activeElement == document.getElementById("content-frame") || document.activeElement == ##doc.getEl...notification##) {
    // if there's a current attachment reminder notification, hide it
    hideAttachmentNotification();
  }  
}

That's also better bc it probably allows addons to customize how ESC is handled.

function hideAttachmentNotification() {

  let notification = document.getElementById("attachmentNotificationBox").currentNotification;
  if (notification) {
    notification.close();
  }
}

Plus code to call hideAttachmentNotification from the notification bar's [x] button, and potentially other spots which hide that notification.

ot: btw [Add attachment] button on notification also has a bug because bar is hidden even if I cancel adding first attachment.

::: mail/components/compose/content/messengercompose.xul
@@ +276,5 @@
>    <!-- Mac Window Menu -->
>    <key id="key_minimizeWindow" command="minimizeWindow" key="&minimizeWindow.key;" modifiers="accel"/>
>  #endif
>  
> +  <key keycode="VK_ESCAPE" oncommand="hideNotification();"/>

We're not (yet) hiding any potential notification which might be there, so the function name looks wrong. more below

I assume this won't hinder/consume ESC on other items like dropdown buttons or recipients area, right?
comment 26
> We're not (yet) hiding any potential notification which might be there, so the function name looks
> wrong. more below

typo: more below = see above in comment 26
(In reply to Thomas D. from comment #26)
> Furthermore, with this patch, can I press ESC to hide the bar when focus is
> *on* the notification bar? Probably not, but that should work, too. (Use
> shift+tab to put focus on notification bar buttons without hiding bar).
Not with the current design, I think.
Reason being, the buttons don't have an id, so if you want, this can be done, but
I don't think there are any usual cases when a user will focus on the notification bar buttons.
Instead, there is more probability that he/she will actually click on them, so focus shouldn't be a concern there.

> ot: btw [Add attachment] button on notification also has a bug because bar
> is hidden even if I cancel adding first attachment.
Ya, I can look at that.

> I assume this won't hinder/consume ESC on other items like dropdown buttons
> or recipients area, right?
The present patch doesn't consume any escape key presses that are in the front context (I don't know what to call that thing, where you have a layered like feel, and the menus/dropdowns are focused firstly and notification bar is hidden behind them, and when these menus/dropdowns go away, notification is exposed to the control).
(In reply to Suyash Agarwal (:sshagarwal) (unfortunately away till 1st Dec) from comment #28)
> (In reply to Thomas D. from comment #26)
> > Furthermore, with this patch, can I press ESC to hide the bar when focus is
> > *on* the notification bar? Probably not, but that should work, too. (Use
> > shift+tab to put focus on notification bar buttons without hiding bar).
> Not with the current design, I think.
> Reason being, the buttons don't have an id, so if you want, this can be
> done,

I definitely want this to be done :)

> I don't think there are any usual cases when a user will focus on the
> notification bar buttons.

Usual or unusual (aka edge case) doesn't matter much. Such bugs are known to haunt TB, they will always come back to you one day. Moreover, do you know how a blind person operates TB? This is an accessibility bug, so it's actually also about users who are handicapped and *cannot* use their mouse. Perhaps when the notification pops up, first thing what blind users do is to shift+tab into it to "see" the choice of buttons (via screen reader), then decide they don't want, then press ESC with focus on the bar - we should allow that. Plus, it simply violates ux-consistency that ESC wouldn't work from the very bar it applies to. Plus, very soon it will also affect mouse users, see next point.

> Instead, there is more probability that he/she will actually click on them,
> so focus shouldn't be a concern there.
> > ot: btw [Add attachment] button on notification also has a bug because bar
> > is hidden even if I cancel adding first attachment.
> Ya, I can look at that.

When that bug is fixed, it's easy for mouse users to end up with focus on notification bar:
- click [Add attachment...] on notification
- click [Cancel] on Attach File(s) dialogue
-> notification should remain visible, as no attachments have been added.
-> focus will be still on [Add attachment...] button!
- change your mind for whatever reason (we don't know, but it's possible), and hit ESC while focus is still on the notification
-> notification should definitely go away... (think of the notification as a miniature dialogue, so ESC while on the dialogue is like clicking Cancel, which is represented by [x], and Cancel should always work inside any dialogue...)
(In reply to :aceman from comment #25)
> Comment on attachment 8338575 [details] [diff] [review]
> Patch v2
> Yes, this works great. If calling the command of the notification button
> didn't work, then this is acceptable.
Ya, I wasn't able to put it in a common function. I'll try once more.

(In reply to Thomas D. from comment #26)
> Comment on attachment 8338575 [details] [diff] [review]
> Patch v2
> ::: mail/components/compose/content/MsgComposeCommands.js
> > +function hideNotification()
> 
> This function is in an odd place:
> This function name is very misleading:
> So to adequately describe the current content of the function, the name
> would have to be hideAttachmentNotificationOnESC()
> I'd imagine something like the following structure (quick draft, not tested,
> not yet functional, pls adjust):
> 
> <key keycode="VK_ESCAPE" oncommand="handleESC();"/>
> 
> function handleESC() {
>   if (document.activeElement == document.getElementById("content-frame") ||
> document.activeElement == ##doc.getEl...notification##) {
>     // if there's a current attachment reminder notification, hide it
>     hideAttachmentNotification();
>   }  
> }
Sure, I'll get this behavior in the code.

> ot: btw [Add attachment] button on notification also has a bug because bar
> is hidden even if I cancel adding first attachment.
Ya, I'll look at that, but probably that will be another bug.
Please file it and let me know.

> I assume this won't hinder/consume ESC on other items like dropdown buttons
> or recipients area, right?
Not, in the present design.

(In reply to Thomas D. from comment #29)
> (In reply to Suyash Agarwal (:sshagarwal) (unfortunately away till 1st Dec)
> from comment #28)
> > (In reply to Thomas D. from comment #26)
> > > Furthermore, with this patch, can I press ESC to hide the bar when focus is
> > > *on* the notification bar? Probably not, but that should work, too. (Use
> > so if you want, this can be
> > done,
> 
> I definitely want this to be done :)
Okay, it will be done.

Thanks.
(In reply to Suyash Agarwal (:sshagarwal) (unfortunately away till 1st Dec) from comment #30)
> (In reply to :aceman from comment #25)
> > Comment on attachment 8338575 [details] [diff] [review]
> > Patch v2
> > Yes, this works great. If calling the command of the notification button
> > didn't work, then this is acceptable.
> Ya, I wasn't able to put it in a common function. I'll try once more.
> 
I mean:

function handleESC() {
let notification =  document.getElementById("attachmentNotificationBox").currentNotification;
  if (notification && document.activeElement == document.getElementById("content-frame") || (check from Thomas))
    notification.querySelector(".closeButton").doCommand();
  }  
}
Attached patch Patch v3 (obsolete) — Splinter Review
I wasn't able to call the internal method of close that is called when close button on the notification bar is clicked.

This works even when the focus is on the "add attachment" or "remind me later" buttons, but not when the focus is on the "x" button.
Attachment #8338575 - Attachment is obsolete: true
Attachment #8340497 - Flags: feedback?(acelists)
Attached patch Toolkit changes required (obsolete) — Splinter Review
I had to make a few changes in the toolkit for using the ids with the buttons.
Attachment #8340499 - Flags: feedback?(acelists)
Comment on attachment 8340497 [details] [diff] [review]
Patch v3

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1773,5 @@
> +function HandleEsc()
> +{
> +  if (document.activeElement == document.getElementById("content-frame") ||
> +      document.activeElement == document.getElementById("attachmentAdd") ||
> +      document.activeElement == document.getElementById("attachmentRemind")) {

This could be sped up like:
let activeElementId = document.activeElement.id;
if ((activeElementId = "content-frame") || (activeElementId = "attachmentAdd")

(hopefully it works also for elements without ID.

However I do not like the adding of the IDs to the buttons of the notification. Can you find a DOM way to check if the active element is a child of body or of the notification? Without testing a specific ID, but using the node hierarchy (the DOM).

@@ +1783,5 @@
> +function hideAttachmentNotification()
> +{
> +  let notification = document.getElementById("attachmentNotificationBox").currentNotification;
> +  if (notification)
> +    notification.close();

I'm OK with causing the .close() method now, after some tests.
But why is this a separate function from handleESC?
Will you use this function from somewhere else (to merge duplicated code) as I wanted? :)
Attachment #8340497 - Flags: feedback?(acelists) → feedback-
Attached patch Patch v4 (obsolete) — Splinter Review
Okay so, probably this?

(In reply to :aceman from comment #34)
> Comment on attachment 8340497 [details] [diff] [review]
> Patch v3
document.getElementById("attachmentNotificationBox").currentNotification;
> > +  if (notification)
> > +    notification.close();
> 
> I'm OK with causing the .close() method now, after some tests.
> But why is this a separate function from handleESC?
> Will you use this function from somewhere else (to merge duplicated code) as
> I wanted? :)
I am not sure, but this design is from Thomas D's comment 26.
Attachment #8340497 - Attachment is obsolete: true
Attachment #8340499 - Attachment is obsolete: true
Attachment #8340499 - Flags: feedback?(acelists)
Attachment #8340528 - Flags: feedback?(acelists)
Comment on attachment 8340528 [details] [diff] [review]
Patch v4

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1772,5 @@
>  
> +function HandleEsc()
> +{
> +  let activeElement = document.activeElement;
> +  if (activeElement == document.getElementById("content-frame") ||

So didn't the activeElement.id = "content-frame" way work?

@@ +1774,5 @@
> +{
> +  let activeElement = document.activeElement;
> +  if (activeElement == document.getElementById("content-frame") ||
> +      activeElement.classList.contains("notification-button") ||
> +      activeElement.classList.contains("messageCloseButton")) {

What about notification.contains(activeElement) as I proposed?
Attached patch Patch v5 (obsolete) — Splinter Review
Sorry, I missed that.
This works well, but it doesn't work when the focus is on the  "x" button.
Attachment #8340528 - Attachment is obsolete: true
Attachment #8340528 - Flags: feedback?(acelists)
Attachment #8340547 - Flags: feedback?(acelists)
Comment on attachment 8340547 [details] [diff] [review]
Patch v5

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

As long as it works anywhere on the notification bar (including all buttons of it), I like it :)
If "x" doesn't work, it is not worse than what you had, but saves the adding of IDs and changing toolkit. Not sure why "x" doesn't work, maybe again because it is an anonymous element and is not evaluated as a descendant of notification. Neil would need to comment on this.
Attachment #8340547 - Flags: feedback?(acelists) → feedback+
Attached patch Patch v6 (obsolete) — Splinter Review
Patch v5 didn't work when the focus was on the "x" button on the notification bar. Fixed it in this patch.
Attachment #8340547 - Attachment is obsolete: true
Attachment #8340603 - Flags: feedback?(acelists)
Comment on attachment 8340603 [details] [diff] [review]
Patch v6

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

Works great now.
Attachment #8340603 - Flags: feedback?(acelists) → feedback+
Will this have a test? :)
Assignee: nobody → syshagarwal
I plan to add the test for this after the tests in bug 939700 land.
Depends on: 939700
Comment on attachment 8340603 [details] [diff] [review]
Patch v6

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

Let's tweak comments a little, for the benefit of posterity

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1769,5 @@
>  
>    return null;
>  }
>  
> +function HandleEsc()

Let's get into the habit and comment all functions that we add pls...

/**
 * Handle ESC keypress from composition window (to close attachment reminder notification bar)
 */

function HandleEsc()

@@ +1771,5 @@
>  }
>  
> +function HandleEsc()
> +{
> +  let activeElement = document.activeElement;

Above this line, please add this comment:

// if there is an attachment reminder notification AND focus is in message body
// or on the notification, hide it

@@ +1776,5 @@
> +  let notification = document.getElementById("attachmentNotificationBox").currentNotification;
> +  if (notification && activeElement.id == "content-frame" ||
> +      notification.contains(activeElement) ||
> +      activeElement.classList.contains("messageCloseButton")) {
> +    // if there is an attachment reminder notification, hide it

this comment is in the wrong place, remove here (see rephrasing above)
Status: NEW → ASSIGNED
Comment on attachment 8340603 [details] [diff] [review]
Patch v6

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1769,5 @@
>  
>    return null;
>  }
>  
> +function HandleEsc()

function names should be camelCase. (Yes, this file does that wrong in many places, it's old.)
Attached patch Patch v6.1 (obsolete) — Splinter Review
Shall we consider this complete? :)
Attachment #8340603 - Attachment is obsolete: true
Attachment #8347710 - Flags: ui-review?(josiah)
Attachment #8347710 - Flags: review?(mkmelin+mozilla)
Attachment #8347710 - Flags: review?(acelists)
Attachment #8347710 - Flags: feedback?(bugzilla2007)
Whiteboard: [Morphed! New STR comment 14][gs][good first bug][has draft patch → [Morphed! New STR comment 14][gs][good first bug]
Comment on attachment 8347710 [details] [diff] [review]
Patch v6.1

Yes, this looks good now! :)
Attachment #8347710 - Flags: feedback?(bugzilla2007) → feedback+
Comment on attachment 8347710 [details] [diff] [review]
Patch v6.1

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1769,5 @@
>  
>    return null;
>  }
>  
> +/*

/**

@@ +1775,5 @@
> + */
> +function handleEsc()
> +{
> +  // if there is an attachment reminder notification AND focus is in message body
> +  // or on the notification, hide it.

Capital "If" as it is a sentence.

@@ +1780,5 @@
> +  let activeElement = document.activeElement;
> +  let notification = document.getElementById("attachmentNotificationBox").currentNotification;
> +  if (notification && activeElement.id == "content-frame" ||
> +      notification.contains(activeElement) ||
> +      activeElement.classList.contains("messageCloseButton")) {

Shouldn't this be "if (notification && (...all the other conditions...))" ? Notification can be null I think.
Attachment #8347710 - Flags: review?(acelists)
Attached patch Patch 6.5 (obsolete) — Splinter Review
Sorry, missed that.

Thanks.
Attachment #8347710 - Attachment is obsolete: true
Attachment #8347710 - Flags: ui-review?(josiah)
Attachment #8347710 - Flags: review?(mkmelin+mozilla)
Attachment #8347894 - Flags: ui-review?(josiah)
Attachment #8347894 - Flags: review?(mkmelin+mozilla)
Attachment #8347894 - Flags: feedback?(acelists)
Comment on attachment 8347894 [details] [diff] [review]
Patch 6.5

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1769,5 @@
>  
>    return null;
>  }
>  
> +/*

This should be /** as I already said.
Attachment #8347894 - Flags: feedback?(acelists) → feedback+
Comment on attachment 8347894 [details] [diff] [review]
Patch 6.5

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

Looks fine to me. I'm not sure if I like that the reminder pops back up if you use a different keyword, but that's not specific to this bug. Using the (x) does the same thing. So ui-r+
Attachment #8347894 - Flags: ui-review?(josiah)
Attachment #8347894 - Flags: ui-review+
Attachment #8347894 - Flags: review?(mkmelin+mozilla)
Attachment #8347894 - Flags: review+
(In reply to :aceman from comment #49)
> Comment on attachment 8347894 [details] [diff] [review]
> > +/*
> This should be /** as I already said.

Suyash, do you want somebody to add that missing * in the comment for you or you want to add it yourself?
You could perhaps just add it in the patch file itself without going back to the drawing board ;)
Flags: needinfo?(syshagarwal)
Attached patch Patch v7Splinter Review
Sorry for taking so long on such a thing, but needinfo wasn't required :P

I don't know if JosiahOne's r+ was intentional or not, I am re-requesting review from mkmelin just for being sure.

Thanks.
Attachment #8347894 - Attachment is obsolete: true
Attachment #8349191 - Flags: ui-review+
Attachment #8349191 - Flags: review?(mkmelin+mozilla)
Attachment #8349191 - Flags: feedback+
Flags: needinfo?(syshagarwal)
Whiteboard: [Morphed! New STR comment 14][gs][good first bug] → [STR comment 14][gs][good first bug]
(In reply to Suyash Agarwal (:sshagarwal) from comment #52)
> I don't know if JosiahOne's r+ was intentional or not, 

It was not. Sorry about that.
Attachment #8349191 - Flags: review?(mkmelin+mozilla) → review+
Ready for landing :)
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/0c99b0c0c9e7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
You need to log in before you can comment on or make changes to this bug.