Closed
Bug 562048
Opened 14 years ago
Closed 11 years ago
Notification bar needs to look slicker (Remote Content, Junk, Scam, Send Return Receipt, Edit Draft)
Categories
(Thunderbird :: Message Reader UI, defect)
Thunderbird
Message Reader UI
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 28.0
People
(Reporter: andreasn, Assigned: mkmelin)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: ux-consistency)
Attachments
(12 files, 42 obsolete files)
68.13 KB,
image/png
|
Details | |
28.08 KB,
image/png
|
Details | |
8.42 KB,
patch
|
Details | Diff | Splinter Review | |
2.71 KB,
text/plain
|
Details | |
84.02 KB,
image/jpeg
|
Details | |
22.29 KB,
image/png
|
Details | |
39.34 KB,
image/png
|
bwinton
:
ui-review+
|
Details |
50.13 KB,
image/png
|
bwinton
:
ui-review-
|
Details |
33.78 KB,
image/png
|
bwinton
:
ui-review+
|
Details |
142.74 KB,
image/png
|
Details | |
216.98 KB,
image/png
|
Details | |
109.03 KB,
patch
|
bwinton
:
review+
mkmelin
:
ui-review+
|
Details | Diff | Splinter Review |
The Firefox notification bar on the Mac looks really slick. It would be nice to use a similar style for Thunderbird as well.
Reporter | ||
Updated•14 years ago
|
Keywords: ux-consistency
Comment 1•14 years ago
|
||
So I was actually going to file a bug sometime (I thought I had, but I can't find it) on the fact that we should replace our current notification bar in messages with the toolkit one - the one we currently has is made up "manually", and replacing it with the current toolkit notification bar would be a bonus and allow us to get the same styling.
Updated•14 years ago
|
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Comment 2•14 years ago
|
||
Hints for implementation: The existing xul "notification" bar for messages is defined here: http://hg.mozilla.org/comm-central/annotate/4a6a1e8f4389/mail/base/content/mailWindowOverlay.xul#l1852 The main code that drives the xul is here: http://hg.mozilla.org/comm-central/annotate/4a6a1e8f4389/mail/base/content/mailWindowOverlay.js#l2539 If you look for notificationbar in existing xul code, you'll see various - Thunderbird has them in content tabs and on its main window. Generally, you're looking at completely replacing the xul notification bar in mailWindowOverlay.xul (id="msgNotificationBar") with a <notificationbar> that wraps the messagepane browser element. I would suggest keeping some of the gMessageNotificationBar structure for easy of use. Note that the notificationbar element can do its own prioritisation (iirc) if you give it the right data. I'd suggest initially doing: - basic replacement of the msgNotificationBar with the notificationbar element - get some notifications being displayed at appropriate times. - Just hard-code strings until you've got the basics working. At that point, you could try for an initial ui-review, and I'm sure Andreas will help with some styling.
OS: Mac OS X → All
Hardware: x86 → All
Reporter | ||
Comment 3•14 years ago
|
||
(In reply to comment #2) > At that point, you could try for an initial ui-review, and I'm sure Andreas > will help with some styling. Yup, just let me know if/when there is anything I can help out with.
Reporter | ||
Comment 4•14 years ago
|
||
This patch really sucks right now, but it kind of works. Makes use of <notificationbox> and <notification> for all the notifications. This also makes the notifications smaller, bug 540106 Known issues: If a notification is closed, the next in turn will show up. Icons are wrong.
Reporter | ||
Comment 5•14 years ago
|
||
Sets warning on all the notifications to make them look that Firefox-slick on Mac OS X.
Attachment #468689 -
Attachment is obsolete: true
Reporter | ||
Comment 6•14 years ago
|
||
Comment 7•13 years ago
|
||
Attachment #511007 -
Flags: feedback?(bugzilla)
Comment 8•13 years ago
|
||
This is closer to what I was thinking of. It removes all of the existing xul bars and replaces them by a single notificationbar element. The notificationbar is then fed individual displays by the appropraite function. This example works reasonably well for junk emails, I've not updated anything else. There's a bunch of work still to do: - removing all the items associated with the old set-up (strings, css etc) - revising the existing code to work with the new bars - localisation of the javascript strings - making sure we've got a chrome icon at the same place in the theme on all platforms and probably a bit more. However, that's where I would be starting from.
Comment 9•13 years ago
|
||
Comment on attachment 511007 [details] [diff] [review] Made the notification bar look slicker. (v1) I think if we're going to make a change here, we may as well try going the whole way and reimplementing this with proper use of notificationbar, which will be better for extensions as well.
Attachment #511007 -
Flags: feedback?(bugzilla) → feedback-
Comment 10•13 years ago
|
||
Comment on attachment 511015 [details] [diff] [review] Example patch Notes for whom it may concern: >+ var button = [{ [Since it's an array, it's normally named buttons, even if there's only one.] >+ this.mMsgNotificationBar.appendNotification("This message is junk", 1, "chrome://messenger/skin/icons/junk.png", this.mMsgNotificationBar.PRIORITY_INFO_MEDIUM, button); Text will need to be localised of course. >+// this.updateMsgNotificationBar(kMsgNotificationJunkBar, isJunk); Need to remove the notification if it's not junk. >+ this.mMsgNotificationBar.removeAllNotifications(true); >+ // XXX fixme >+// this.mBarStatus = 0; >+// this.mMsgNotificationBar.selectedIndex = 0; >+// this.mMsgNotificationBar.collapsed = true; Well, that's the equivalent, although it will remove extension notifications, which may or may not be desired. There's another call that only removes notifications due for removal. >+ <!-- XXX May want to actually wrap this around messagepanewrapper. XXX Does this want to be within the messagepanewrapper hbox? --> >+ <notificationbox id="msgNotificationBar"/> > <!-- The messagepanewrapper hbox exists to allow > extensions to add sidebars to the message. --> > <hbox id="messagepanewrapper" flex="1"> By comparison with other <notificationbox>es, the ideal location is inside the messagepanewrapper wrapped around the messagepane <browser>.
Comment 11•13 years ago
|
||
(In reply to comment #10) > Text will need to be localised of course. That's what "- localisation of the javascript strings" meant ;-) > >+// this.updateMsgNotificationBar(kMsgNotificationJunkBar, isJunk); > Need to remove the notification if it's not junk. If clear notifications does a removeAllNotifications, then it isn't necessary to remove the notification. > >+ this.mMsgNotificationBar.removeAllNotifications(true); > >+ // XXX fixme > >+// this.mBarStatus = 0; > >+// this.mMsgNotificationBar.selectedIndex = 0; > >+// this.mMsgNotificationBar.collapsed = true; > Well, that's the equivalent, although it will remove extension notifications, > which may or may not be desired. There's another call that only removes > notifications due for removal. Yes, we need to check the callers to that function. I was assuming it was generally just called on change of message/display, and therefore I think we should always clear all notifications (e.g. think of going from a message display to the start page).
Comment 12•13 years ago
|
||
Attachment #511007 -
Attachment is obsolete: true
Attachment #513639 -
Flags: ui-review?
Updated•13 years ago
|
Attachment #513639 -
Flags: ui-review? → ui-review?(nisses.mail)
Reporter | ||
Comment 13•13 years ago
|
||
(In reply to comment #12) > Created attachment 513639 [details] [diff] [review] > Made the notification bar look slicker. Thanks for the patch Edmund! I seem to have some issues getting it to function correctly on my system. When running this patch, Thunderbird seemed to become very confused about what messages should pop up a "remote content" bar, a "return recipient" bar etc. It also spits out some errors about linkLabel not being defined in the error console. I think this might be related. Do you get the same behavior on your system running the patch? Visually, it seem that making the notification type=info makes the bar all gray on Linux as it sets the background-color to -moz-dialog. Removing that in DOM inspector seems to make it have it's regular color (infobackground) again. Haven't tried on other systems yet.
Comment 14•13 years ago
|
||
Just changed the names of the properties. Also moved the notification bar before the message header. The 'meat' of the patch hasn't been changed.
Attachment #513639 -
Attachment is obsolete: true
Attachment #513862 -
Flags: ui-review?(nisses.mail)
Attachment #513639 -
Flags: ui-review?(nisses.mail)
Comment 15•13 years ago
|
||
(In reply to comment #14) > Just changed the names of the properties. Also moved the notification bar > before the message header. The 'meat' of the patch hasn't been changed. I'm not convinced by this change. It will mean that if a notification bar is displayed the header will be displayed lower down. This will mean that the position of the buttons in the header will change. For example, if I'm busy archiving messages in a folder, and I hit a message with a notification bar, then suddenly the archive button isn't going to be in the same place, and I'm going to potentially hit a notification bar icon instead. (In reply to comment #13) > Visually, it seem that making the notification type=info makes the bar all gray > on Linux as it sets the background-color to -moz-dialog. Removing that in DOM > inspector seems to make it have it's regular color (infobackground) again. > Haven't tried on other systems yet. This is probably because we're using PRIORITY_INFO_MEDIUM everywhere and grey is the information option. There's probably two parts to this. Firstly, change the respective priorities to match the notification type, so I would probably suggest: - Phishing: PRIORITY_CRITICAL_MEDIUM - Junk: PRIORITY_WARNING_HIGH - Remote Content: PRIORITY_WARNING_MEDIUM - MDN (Return Receipt): PRIORITY_INFO_MEDIUM I suspect Critical is red, and Warning is yellow. However, if we're not happy with the default colours of those, then we can add some styling just to the notification bar in the message header to alter those colours.
Comment 16•13 years ago
|
||
Comment on attachment 513862 [details] [diff] [review] Made the notification bar look slicker. (v2) Just a couple of fly-by comments. >@@ -2556,16 +2556,44 @@ var gMessageNotificationBar = Once this is a bit nearer completion, you should be able to remove mBarStatus and mBarFlagValues from gMessageNotificationBar. >+ var junkBarMsg = stringBundle.getString('junkBarMessage') + " "; >+ var junkBarNotJunk = stringBundle.getString('junkBarButton') + " "; >+ var junkBarInfo = stringBundle.getString('junkBarInfoButton') + " "; Not sure why you're adding spaces to these, it shouldn't be necessary. >+ if (isJunk) { >+ var buttons = [{ >+ label: junkBarInfo, >+ accessKey: junkBarInfoKey, I think you could skip the intermediate variables for these. >+ this.mMsgNotificationBar.appendNotification(junkBarMsg, 1, >+ "chrome://messenger/skin/icons/folder-junk.gif", >+ this.mMsgNotificationBar.PRIORITY_INFO_MEDIUM, >+ buttons); >+ >+ this.updateMsgNotificationBar(kMsgNotificationJunkBar, isJunk); updateMsgNotificationBar should just go away because the appendNotification is doing the equivalent. >+ var stringBundle = document.getElementById('bundle_messenger'); >+ var brandBundle = document.getElementById('brand_bundle'); Might be worth just making these member variables of gMessageNotificationBar in the same way that mMsgNotificationBar is.
Comment 17•13 years ago
|
||
Attachment #513862 -
Attachment is obsolete: true
Attachment #514113 -
Flags: ui-review?(nisses.mail)
Attachment #513862 -
Flags: ui-review?(nisses.mail)
Comment 18•13 years ago
|
||
I would really like to see this get done. :)
Reporter | ||
Comment 19•13 years ago
|
||
Yay! This is making great progress! Junk bar: * Missing junk icon to the left * the big [ ? ]-button looks a bit heavy. I think we want to make this just an icon. Probably something similar to the (?) icon in the address book [1]. Remote content bar: * I was unable to trigger this at all. Draft message: * Put the edit button on the right * (This is kind of out of the scope of this bug, but...) it would be neat to fix the bold+italic text to just regular text here.
Reporter | ||
Comment 20•13 years ago
|
||
Here is a patch with new info icons for all platforms (linux, mac, windows xp/aero) to use for the info icon.
Comment 21•13 years ago
|
||
I think it would be possible to continue using CSS for the images. https://developer.mozilla.org/en/XUL/notificationbox#m-appendNotification appendNotification( label , value , image , priority , buttons ) This creates a notification inside the notiifcation box with a "value" attribute set to the second parameter of appendNotification() So: this.mMsgNotificationBar.appendNotification(junkBarMsg, "junk-message", null, this.mMsgNotificationBar.PRIORITY_WARNING_HIGH, buttons); Then you could use some CSS like: { #msgNotificationBar > notification[value="junk-message"] { list-style-image: url("chrome://messenger/skin/icons/junk.png"); } This makes things easier for third party themes.
Comment 22•13 years ago
|
||
Attachment #514113 -
Attachment is obsolete: true
Attachment #531310 -
Flags: ui-review?(nisses.mail)
Attachment #514113 -
Flags: ui-review?(nisses.mail)
Comment 23•13 years ago
|
||
Ewong, where is the bit where you set the class attribute to "infoButton"??
Reporter | ||
Comment 24•13 years ago
|
||
Comment on attachment 531310 [details] [diff] [review] Made the notification bar look slicker. (v4) In general this looks good. Some small issues I ran into: * "To protect your privacy, &brandShortName; has blocked..." in the remote content bar. It should probably say Thunderbird and/or Shredder. * If viewing a message with remote content right after another and then closing the bar in the second message, another one (from the previous message?) is still around. * Icon scaling issues for remote content, at least under Linux. I'll come up with some updated graphics for this and attach to the bug.
Attachment #531310 -
Flags: ui-review?(nisses.mail) → ui-review-
Reporter | ||
Comment 25•13 years ago
|
||
It also seems that the phishing bar does no longer work.
Comment 26•13 years ago
|
||
Attachment #531310 -
Attachment is obsolete: true
Attachment #537346 -
Flags: ui-review?(nisses.mail)
Comment 27•13 years ago
|
||
Attachment #537346 -
Attachment is obsolete: true
Attachment #537452 -
Flags: ui-review?(nisses.mail)
Attachment #537346 -
Flags: ui-review?(nisses.mail)
Reporter | ||
Comment 29•13 years ago
|
||
Comment on attachment 537452 [details] [diff] [review] Made the notification bar look slicker. (v6) Edmund told me there were still some issues with this patch (specifically regards triggering phishing), so setting the Flag to feedback instead of ui-review and trying out this as soon as I'm done with my build.
Attachment #537452 -
Flags: ui-review?(nisses.mail) → feedback?(nisses.mail)
Reporter | ||
Comment 30•13 years ago
|
||
It seems this patch no longer apply. Bitrotted?
Comment 31•13 years ago
|
||
unbitrotted patch
Attachment #537452 -
Attachment is obsolete: true
Attachment #546423 -
Flags: feedback?(nisses.mail)
Attachment #537452 -
Flags: feedback?(nisses.mail)
Comment 32•13 years ago
|
||
Comment on attachment 546423 [details] [diff] [review] Made the notification bar look slicker. (v7) The phishing part isn't done. isPhishingMsg could require a new parameter aUrl.
Reporter | ||
Comment 33•13 years ago
|
||
Comment on attachment 546423 [details] [diff] [review] Made the notification bar look slicker. (v7) In general, nice progress! One small thing I noticed is that the close button in the remote content bar triggers the animation, but don't make the bar go away. Apart from that, things are looking sweet!
Attachment #546423 -
Flags: feedback?(nisses.mail) → feedback+
Comment 34•13 years ago
|
||
I started working on bug 631688, but I think the best solution to that bug is included in this patch. Edmund, let me know if you're still working on this or if I can take over.
Comment 36•13 years ago
|
||
(In reply to comment #34) > I started working on bug 631688, but I think the best solution to that bug > is included in this patch. Edmund, let me know if you're still working on > this or if I can take over. Yup. Still working on it.
Comment 37•13 years ago
|
||
With phishing added, but need feedback. Remote Content notification bar is faulty. From the DOM, it seems to be generated more than once.
Attachment #546423 -
Attachment is obsolete: true
Attachment #551028 -
Flags: feedback?(nisses.mail)
Comment 38•13 years ago
|
||
the OnMsgHasRemoteContent callback can be called multiple times per message; I'm not completely sure, but it may be once per remote URL in the message. There's an example in the MDN docs for notificationbox of either getting the existing notification for a particular key, or creating a new one; see https://developer.mozilla.org/en/Code_snippets/Alerts_and_Notifications#Using_notification_box You'll need to do this for remote content, and it might be a good idea to do it for all the other notifications too. I'm also getting an JavaScript exception because your patch doesn't include a declaration for checkMsgHdrPropertyIsNot which you're using in setPhishingMsg()
Comment 39•13 years ago
|
||
Attachment #551028 -
Attachment is obsolete: true
Attachment #552067 -
Flags: feedback?(irving)
Attachment #551028 -
Flags: feedback?(nisses.mail)
Updated•13 years ago
|
Attachment #552067 -
Flags: feedback?(irving) → feedback?(nisses.mail)
Comment 40•13 years ago
|
||
Sorry for the massive delay; aside from vacation, I got hung up trying to figure out why I couldn't add non-button elements to the notificationbox XUL. Next step is to ask on IRC #xul... I did come up with a fix for multiple copies of the notification, I've attached a patch to go on top of ewong's v9 patch. Most of the change is just indentation, *except* the appendNotification() call where I replaced a null parameter with a string tag for the type of notification. I also tried a version with all the non-button elements turned into buttons; the notificationbox displayed them correctly but bwinton vetoed the UI because it looked really ugly.
Reporter | ||
Comment 41•13 years ago
|
||
Comment on attachment 552067 [details] [diff] [review] Made the notification bar look slicker. (v9) Since the patch by Irving works slightly better, I'm setting this particular patch to feedback minus.
Attachment #552067 -
Flags: feedback?(nisses.mail) → feedback-
Reporter | ||
Comment 42•13 years ago
|
||
Comment on attachment 552067 [details] [diff] [review] Made the notification bar look slicker. (v9) Realized that Irving's patch of course need this patch applied first (I had them mixed up in my queue). So yeah, giving feedback+ to this. Having a combined patch would be great, and having phishing working would be even better! :)
Attachment #552067 -
Flags: feedback- → feedback+
Comment 43•13 years ago
|
||
Included irving's patch changes. Phishing is still MIA.
Attachment #552067 -
Attachment is obsolete: true
Attachment #563271 -
Flags: feedback?(nisses.mail)
Reporter | ||
Updated•13 years ago
|
Attachment #563271 -
Flags: feedback?(nisses.mail) → feedback+
Comment 44•13 years ago
|
||
Unbitrotted patch
Attachment #563271 -
Attachment is obsolete: true
Attachment #575077 -
Flags: feedback?(nisses.mail)
Comment 45•13 years ago
|
||
Comment on attachment 575077 [details] [diff] [review] Made the notification bar look slicker. (v11) Phishing is still MIA.
Comment 46•13 years ago
|
||
With this patch, my debug build of Daily dumps an exception message to standard error when I display a phishing message: ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'[JavaScript Error: "checkMsgHdrPropertyIs is not defined" {file: "chrome://messenger/content/mailWindowOverlay.js" line: 2762}]' when calling method: [nsIUrlListManagerCallback::handleEvent]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: file:///Users/ireid/tbird/objdir-comm-central-default/mozilla/dist/DailyDebug.app/Contents/MacOS/components/nsUrlClassifierListManager.js :: <TOP_LEVEL> :: line 25" data: yes] ************************************************************ It looks like you copied the phishing message classification code from the comm-central/suite source tree into the Thunderbird tree. You didn't need to do this; Thunderbird already has similar code in comm-central/source/mail/base/content/phishingDetector.js. This code runs before (and calls) setPhishingMsg(), so we already know the message is a scam; we don't need to do extra tests in setPhishingMsg(). When I took out the extra scam checking code and made setPhishingMsg() just display the notification, it works for me. I've attached a patch, again to apply over the v11 patch.
Comment 47•13 years ago
|
||
Apply ewong's v11 patch first, and then this patch. The "this message may be a scam" message should show in the notification box. There are still some issues to clean up after this: - icons don't appear in the notification box in my build - "allow remote content for all messages from <address>" link doesn't show up in remote content notification
Attachment #557555 -
Attachment is obsolete: true
Comment 48•13 years ago
|
||
incl irving's changes.
Attachment #575077 -
Attachment is obsolete: true
Attachment #577261 -
Flags: feedback?(irving)
Attachment #575077 -
Flags: feedback?(nisses.mail)
Comment 49•13 years ago
|
||
Comment on attachment 577261 [details] [diff] [review] Made the notification bar look slicker. (v12) Review of attachment 577261 [details] [diff] [review]: ----------------------------------------------------------------- You didn't remove all the code you had previously pulled in from SeaMonkey; everything from the "+" sign on a line by itself (mailWindowOverlay.js around line 3378, just before "function checkMsgHdrPropertyIs(aProperty, aValue)") through to the end of the file should be removed. That's checkMsgHdrPropertyIs, msgHdrForCurrentMessage, GetLoadedMessage, and isMsgEmailScam. Similarly, you modified phishingDetector.js to pass the aUrl parameter to setPhishingMsg(); this is no longer needed, so you can revert that change. I missed one thing in my update to your v11 patch - in mailWindowOverlay.js, the button callback in setPhishingMsg() (around line 2765) should call IgnorePhishingWarning(), not MsgIsNotAScam() - the latter is only implemented in SeaMonkey, not TB. Once these are cleaned up, we still need to get the icons to show up properly in the notification windows, and re-implement the other features currently available in Thunderbird: For the Phishing / Scam Detection window, the "Disable scam detection for all messages" link For the Remote Content window, the "Always load remote content from <address>" link
Attachment #577261 -
Flags: feedback?(irving) → feedback+
Comment 50•13 years ago
|
||
Attachment #577261 -
Attachment is obsolete: true
Attachment #577915 -
Flags: feedback?(irving)
Comment 51•12 years ago
|
||
Comment 52•12 years ago
|
||
Comment on attachment 577915 [details] [diff] [review] Made the notification bar look slicker. (v13) Minor bit-rot on the latest attachment, one of the themes didn't patch cleanly: patching file mail/themes/qute/jar.mn Hunk #1 FAILED at 221 Hunk #2 FAILED at 437 2 out of 2 hunks FAILED -- saving rejects to file mail/themes/qute/jar.mn.rej Aside from that... In 3 pane view, when I switch focus to a message with both remote images and a fake URL to trigger the scam warning(https://bugzilla.mozilla.org/attachment.cgi?id=592136), I see the following on standard error: JavaScript error: chrome://messenger/content/mailWindowOverlay.js, line 2791: mdnBarMsg is null Chrome file doesn't exist: /Users/ireid/tbird/objdir-comm-central-ewong/mozilla/dist/DailyDebug.app/Contents/MacOS/chrome/classic/skin/classic/messenger/icons/remote-blocked.png WARNING: We should have hit the document element...: file /Users/ireid/tbird/comm-central/mozilla/layout/xul/base/src/nsBoxObject.cpp, line 213 Chrome file doesn't exist: /Users/ireid/tbird/objdir-comm-central-ewong/mozilla/dist/DailyDebug.app/Contents/MacOS/chrome/classic/skin/classic/messenger/icons/remote-blocked.png Chrome file doesn't exist: /Users/ireid/tbird/objdir-comm-central-ewong/mozilla/dist/DailyDebug.app/Contents/MacOS/chrome/classic/skin/classic/messenger/icons/question.png WARNING: We should have hit the document element...: file /Users/ireid/tbird/comm-central/mozilla/layout/xul/base/src/nsBoxObject.cpp, line 213 Chrome file doesn't exist: /Users/ireid/tbird/objdir-comm-central-ewong/mozilla/dist/DailyDebug.app/Contents/MacOS/chrome/classic/skin/classic/messenger/icons/question.png Looking at the patch: In phishingDetector.js > 213 if (aLocalListStatus == 0 /* PROT_ListWarden.IN_BLACKLIST */ || > 214 (aLocalListStatus == 2 /* PROT_ListWarden.PROT_ListWarden.NOT_FOUND */ && aFailsStaticTests)) > 215 gMessageNotificationBar.setPhishingMsg(aUrl); setPhishingMsg() doesn't take any parameters, so remove aUrl In messenger.properties > 519 remoteContentBarMessage=To protect your privacy, %S has blocked remote content in this message. Should we have a localization note on this line saying that %S is the product name? We're still missing some of the extra actions from the existing notifications, such as the "always accept remote content from <address>" link. I think we're going to need some help from the XUL team to figure out how to implement those.
Attachment #577915 -
Flags: feedback?(irving) → feedback+
Comment 53•12 years ago
|
||
Unbitrotted.
Attachment #577915 -
Attachment is obsolete: true
Attachment #600611 -
Flags: ui-review?(nisses.mail)
Comment 54•12 years ago
|
||
Attachment #600611 -
Attachment is obsolete: true
Attachment #600614 -
Flags: ui-review?(nisses.mail)
Attachment #600611 -
Flags: ui-review?(nisses.mail)
Reporter | ||
Comment 55•12 years ago
|
||
Comment on attachment 600614 [details] [diff] [review] Made the notification bar look slicker. (v15) I tested v15 of the patch on Mac, and these are the issues I identified: * Junk mail bar lacks an icon * Remote content bar don't show * Phishing bar don't show * Draft message bar is white (should be yellow) and the button is different from the other notification bars
Attachment #600614 -
Flags: ui-review?(nisses.mail) → ui-review-
Comment 56•12 years ago
|
||
Attachment #600614 -
Attachment is obsolete: true
Attachment #610118 -
Flags: feedback?(irving)
Comment 57•12 years ago
|
||
(In reply to Andreas Nilsson (:andreasn) from comment #55) > Comment on attachment 600614 [details] [diff] [review] > Made the notification bar look slicker. (v15) > > I tested v15 of the patch on Mac, and these are the issues I identified: > * Junk mail bar lacks an icon > * Remote content bar don't show > * Phishing bar don't show Current progress on this bug: - icon added to Junk mail bar - Phishing bar is now showing. * Having major difficulties with getting the Remote Content Bar to show (with and without my patch). irving's phishing example does give me the phishing bar, but no Remote Content bar (again, with and without my patch applied). > * Draft message bar is white (should be yellow) and the button is different > from the other notification bars Not exactly sure where this draft message bar is.
Comment 58•12 years ago
|
||
(In reply to Edmund Wong (:ewong) from comment #57) > > * Draft message bar is white (should be yellow) and the button is different > > from the other notification bars > > Not exactly sure where this draft message bar is. Never mind. Found it.
Comment 59•12 years ago
|
||
Attachment #610499 -
Flags: review?(nisses.mail)
Updated•12 years ago
|
Attachment #610118 -
Attachment is obsolete: true
Attachment #610118 -
Flags: feedback?(irving)
Comment 60•12 years ago
|
||
(In reply to Edmund Wong (:ewong) from comment #59) > Created attachment 610499 [details] [diff] [review] > Made the notification bar look slicker. (v17) This version puts up multiple copies of the "remote content" notification. These lines: + var oldNotif = this.mMsgNotificationBar.getNotificationWithValue("remoteContent"); + if (oldNotif == null) { + var bar = + this.mMsgNotificationBar.appendNotification(remoteContentMsg, 1, + "chrome://messenger/skin/icons/remote-blocked.png", + this.mMsgNotificationBar.PRIORITY_WARNING_MEDIUM, + buttons); You need to pass the same identifier string "remoteContent" as the second parameter to mMsgNotificationBar.appendNotification(), as you use in the getNotificationWithValue() call, like so: + var oldNotif = this.mMsgNotificationBar.getNotificationWithValue("remoteContent"); + if (oldNotif == null) { + var bar = *** change 1 to "remoteContent" + this.mMsgNotificationBar.appendNotification(remoteContentMsg, "remoteContent", *** + "chrome://messenger/skin/icons/remote-blocked.png", + this.mMsgNotificationBar.PRIORITY_WARNING_MEDIUM, + buttons); Other than that, looking good.
Reporter | ||
Comment 61•12 years ago
|
||
I think this patch has too much javascript in it for me to do code review, but I'm happy to do a ui-review for you instead. He're some feedback in the meantime: * Draft - OK! * Remote content - Needs icon (I will fix this) * Junk mail - Needs icon (I will fix this) * Scam - when this pops up I get a bunch of other bars under it that I also have to close. This sounds similar to what Irving experienced above. I'm really happy with the progress on this bug. It's starting to look really slick on the mac now!
Comment 62•12 years ago
|
||
the Draft Message should use (atm) info.png, but it's not showing. Need some help with understanding why it's failing to find chrome://messenger/skin/icons/info.png.
Attachment #610499 -
Attachment is obsolete: true
Attachment #611356 -
Flags: ui-review?(nisses.mail)
Attachment #610499 -
Flags: review?(nisses.mail)
Reporter | ||
Comment 63•12 years ago
|
||
This patch (to be applied on top of Edmunds v18 patch) should take care of the missing icons on OSX and the bad scaling of icons on Windows and Linux.
Reporter | ||
Comment 64•12 years ago
|
||
Comment on attachment 611356 [details] [diff] [review] Made the notification bar look slicker. (v18) canceling review as we'll have a new patch with the new graphics up soon.
Attachment #611356 -
Flags: ui-review?(nisses.mail)
Reporter | ||
Comment 65•12 years ago
|
||
This is really close now! Two issues I've identified that's a regression compared to what we have now: * If a scam message have several suspicious links, this patch will trigger one scam notification bar for each. * Closing the junk message works fine, but pressing the "Not junk" button sometimes trigger a new junk mail notification. Apart from that, all looks good!
Reporter | ||
Comment 66•12 years ago
|
||
If qfold worked as it should this should be a combined patch with Edmunds v18 from attachment 611356 [details] [diff] [review] and my new icons from attachment 613622 [details] [diff] [review]
Attachment #611356 -
Attachment is obsolete: true
Attachment #613622 -
Attachment is obsolete: true
Comment 67•12 years ago
|
||
We can add XUL elements directly as children of the Notification object, as in the example below (this is on top of andreasn's most recent version of the patch). The appendNotification call (just above this diff) returns a XUL Notification object, and adds the buttons as the last direct children of the Notification. We can add additional children to appear before or after the buttons; the example below puts the child after, but we want to change that so it comes before. Using just this change, the button moves over to make room for the label but the label text isn't actually visible. I haven't figured out what the problem with that is yet. --- a/mail/base/content/mailWindowOverlay.js +++ b/mail/base/content/mailWindowOverlay.js @@ -2750,19 +2750,17 @@ var gMessageNotificationBar = var linkLabel = bar.ownerDocument.createElementNS(XULNS, "label"); var addedLink = this.stringBundle.getFormattedString('alwaysLoadRemoteContentForSender2', [emailAddress ? emailAddress : aMsgHdr.author]); linkLabel.className = "text-link"; linkLabel.setAttribute("value", addedLink); linkLabel.onclick = allowRemoteContentForSender; - var nContainer = bar.ownerDocument.getAnonymousElementByAttribute(bar, - "anonid", "messageText"); - nContainer.appendChild(linkLabel); + bar.appendChild(linkLabel); } }, setPhishingMsg: function() { var phishingMsgNote = this.stringBundle.getString('phishingBarMessage'); var buttons = [{
Comment 68•12 years ago
|
||
Here's a patch that gets "Always allow remote content from..." working - there were a number of things that needed fixing, including inserting our XUL in the right place and fixing a typo in the XUL namespace string. Also cleaned up the onClick callback to pass parameters through rather than re-calculating the sender name/address. Still seeing multiple copies of the "scam" notice when there is more than one bad URL in the message. There is another approach, which I haven't tried yet, which is to attach a popup menu to the button; the only example I could find in the browser source tree is at http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#495 / http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.xul#261
Attachment #614572 -
Flags: feedback?(ewong)
Updated•12 years ago
|
Attachment #614572 -
Flags: feedback?(ewong) → feedback+
Comment 69•12 years ago
|
||
Patch that includes irving's diff.
Attachment #619028 -
Flags: ui-review?(nisses.mail)
Reporter | ||
Comment 70•12 years ago
|
||
It looks like I get a [object MouseEvent] filled out as the e-mail address if I press the "always load remote content..." with patch v19. Apart from that everything looks good. ui-r+ on patch v19 with that fixed.
Comment 71•12 years ago
|
||
Attachment #619028 -
Attachment is obsolete: true
Attachment #619028 -
Flags: ui-review?(nisses.mail)
Attachment #629568 -
Flags: ui-review?(nisses.mail)
Comment 72•12 years ago
|
||
Wow! As I could find out by chance today in a real-world experience, the phishing warning works nice with v20 and is very eye-catching. :-)
Reporter | ||
Comment 73•12 years ago
|
||
Seems there have been some bitrot in a couple of files.
Comment 74•12 years ago
|
||
Attachment #629568 -
Attachment is obsolete: true
Attachment #629568 -
Flags: ui-review?(nisses.mail)
Attachment #632239 -
Flags: ui-review?(nisses.mail)
Comment 75•12 years ago
|
||
Fixed the v21 unbitrotted patch.
Attachment #632239 -
Attachment is obsolete: true
Attachment #632239 -
Flags: ui-review?(nisses.mail)
Attachment #632518 -
Flags: ui-review?(nisses.mail)
Reporter | ||
Comment 76•12 years ago
|
||
This is a similar visual glitch that happens in bug #759744 and not directly related to this bug, but I thought I should still mention it.
Reporter | ||
Comment 77•12 years ago
|
||
There is an issue that the above screenshot has &brandShortName; in it though.
Comment 78•12 years ago
|
||
v22 patch somehow lost the phishing part changes.
Attachment #632518 -
Attachment is obsolete: true
Attachment #632518 -
Flags: ui-review?(nisses.mail)
Attachment #632534 -
Flags: ui-review?
Updated•12 years ago
|
Attachment #632534 -
Flags: ui-review?
Comment 79•12 years ago
|
||
Fixed oddity and phishing from v21.
Attachment #632534 -
Attachment is obsolete: true
Attachment #632538 -
Flags: ui-review?
Updated•12 years ago
|
Attachment #632538 -
Flags: ui-review? → ui-review?(nisses.mail)
Comment 80•12 years ago
|
||
Edmund, I hate to throw another wrinkle onto this patch, but can you make sure it doesn't have the same problem as bug 761654?
Reporter | ||
Comment 81•12 years ago
|
||
All seems to work fine (tested on Linux and Mac so far). There is a scrollbar appearing sometimes, but that's a bug about notification bars in TB overall, so that's being taken care of in bug #759744. Will test on Windows before I give ui-r+
Reporter | ||
Comment 82•12 years ago
|
||
Comment on attachment 632538 [details] [diff] [review] Make the notification bar look slicker. (v24) Looks good on Windows too. ui-r+!
Attachment #632538 -
Flags: ui-review?(nisses.mail) → ui-review+
Updated•12 years ago
|
Attachment #632538 -
Flags: review?(bwinton)
Comment 83•12 years ago
|
||
Comment on attachment 632538 [details] [diff] [review] Make the notification bar look slicker. (v24) So, first off, the Remote Content notification still looks a little weird for me, as shown in http://dl.dropbox.com/u/2301433/Screenshots/remoteContent.png >+++ b/mail/base/content/mailWindowOverlay.js >@@ -2629,52 +2629,129 @@ var gMessageNotificationBar = > get mMsgNotificationBar() { > delete this.mMsgNotificationBar; >+ delete this.stringBundle; >+ delete this.brandBundle; >+ >+ this.stringBundle = document.getElementById('bundle_messenger'); >+ this.brandBundle = document.getElementById('brand_bundle'); Why do we need to do this every time we get the notification bar? > setJunkMsg: function(aMsgHdr) > { > var isJunk = false; >- >- if (aMsgHdr) >- { >+ var junkBarMsg = this.stringBundle.getString('junkBarMessage'); >+ >+ if (aMsgHdr) { > var junkScore = aMsgHdr.getStringProperty("junkscore"); > isJunk = ((junkScore != "") && (junkScore != "0")); > } > >- this.updateMsgNotificationBar(kMsgNotificationJunkBar, isJunk); >- >- goUpdateCommand('button_junk'); Given that mconley has a bug open to move more things to commands (so that we can track them in Test Pilot), this seems like an odd change to make. >@@ -2683,38 +2760,71 @@ var gMessageNotificationBar = >- let mdnBarMsg = document.getElementById("mdnBarMessage"); [snip…] >+ var mdnBarMsg2 = this.stringBundle.getString('mdnBarMessage'); Why not just call this mdnBarMsg? >+ this.mMsgNotificationBar.appendNotification(mdnBarMsg2, 1, >+ "chrome://messenger/skin/icons/info.png", >+ this.mMsgNotificationBar.PRIORITY_INFO_MEDIUM, >+ buttons); The other functions check stuff like: >+ var oldNotif = this.mMsgNotificationBar.getNotificationWithValue("phishingContent"); >+ if (oldNotif == null) { Why aren't we doing that here? >+ setDraftEditMessage: function() { [snip…] >+ this.mMsgNotificationBar.appendNotification(draftMsgNote, "draftMsgContent", >+ "chrome://messenger/skin/icons/info.png", >+ this.mMsgNotificationBar.PRIORITY_WARNING_HIGH, >+ buttons); Or here? >@@ -2750,35 +2860,18 @@ function LoadMsgWithRemoteContent() >-function allowRemoteContentForSender() >+function allowRemoteContentForSender(authorEmailAddress, authorDisplayName) > { >- // get the sender of the msg hdr >- var msgHdr = gMessageDisplay.displayedMessage; Hurray for less reliance on globals! :) >+++ b/mail/base/content/mailWindowOverlay.xul >@@ -2002,65 +2003,16 @@ >-<!-- The msgNotificationBar appears on top of the message and displays >- information like: junk, contains remote images, or is a suspected phishing >- URL. >---> I think we might want to keep this comment, but… >+++ b/mail/base/content/messageWindow.xul >@@ -142,18 +142,17 @@ >- <hbox id="editMessageBox"/> >- <deck id="msgNotificationBar"/> >+ <notificationbox id="msgNotificationBar"/> put it here instead. >+++ b/mail/base/content/messenger.xul >@@ -492,18 +492,17 @@ >- <hbox id="editMessageBox"/> >- <deck id="msgNotificationBar"/> >+ <notificationbox id="msgNotificationBar"/> and maybe here, too. >+++ b/mail/locales/en-US/chrome/messenger/messenger.properties >@@ -289,16 +289,36 @@ feedsAcctType=Feeds >+junkBarMessage=This message is Junk mail. Huh, that's not the same as the previous message ("&brandShortName; thinks this message is junk mail."). And seems to posses a certainty I don't think is warranted. >+remoteContentBarMessage=To protect your privacy, %S has blocked remote content in this message. This is going to need a localization note (https://developer.mozilla.org/en/Localization_notes) explaining what %S is. >+phishingBarMessage=This message may be a scam. >+phishingBarIgnoreButton=Ignore Warning >+phishingBarIgnoreButtonKey=I >+phishingBarErrorMessage=This message doesn't appear to be a scam What happened to the "Disable scam detection for all messages" message? Do we not offer that option anymore, and if not, why not? Aside from that, it seems reasonable, but I think I'm going to say r- so that I can double-check the fixes, and also because I'm some crazy masochist who likes reviewing 1150-line, 57.71 KB patches. :P Thanks, Blake.
Attachment #632538 -
Flags: review?(bwinton) → review-
Reporter | ||
Comment 84•12 years ago
|
||
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #83) > Comment on attachment 632538 [details] [diff] [review] > Make the notification bar look slicker. (v24) > > So, first off, the Remote Content notification still looks a little weird > for me, as shown in > http://dl.dropbox.com/u/2301433/Screenshots/remoteContent.png This is probably because of bug #759744 not been checked in yet.
Comment 85•12 years ago
|
||
I need a bit of a feedback on this patch. After unbitrotting it after updating my tree, now for RemoteContent notification bar, when I click on a message that has it, the "New Contact" dialog appears. I click cancel and it appears again, ad infinitum until I close the main TB window (which requires precision clicking).
Attachment #647473 -
Flags: feedback?(irving)
Comment 86•12 years ago
|
||
Comment on attachment 647473 [details] [diff] [review] Make the notification bar look slicker. (v25) Review of attachment 647473 [details] [diff] [review]: ----------------------------------------------------------------- Some of the graphics don't seem to be there on the Mac version yet: Chrome file doesn't exist: /Users/ireid/tbird/objdir-comm-central-default/mozilla/dist/DailyDebug.app/Contents/MacOS/chrome/classic/skin/classic/messenger/icons/remote-blocked.png Chrome file doesn't exist: /Users/ireid/tbird/objdir-comm-central-default/mozilla/dist/DailyDebug.app/Contents/MacOS/chrome/classic/skin/classic/messenger/icons/phishing.png ::: mail/base/content/mailWindowOverlay.js @@ +2719,5 @@ > + this.mMsgNotificationBar.PRIORITY_WARNING_MEDIUM, > + buttons); > + > + if (emailAddress) { > + var XULNS = "http://www.mozilla.org/keymaster.gatekeeper/there.is.only.xul"; In v.24 of the patch, where this worked, this line was var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; (slash, rather than dot, between keymaster and gatekeeper). I think this error was in some earlier work in progress on this patch, seems to have regressed. @@ +2727,5 @@ > + > + linkLabel.className = "text-link"; > + linkLabel.setAttribute("value", addedLink); > + linkLabel.onclick = allowRemoteContentForSender(emailAddress, > + displayName); In v.24 this was linkLabel.onclick = function(){allowRemoteContentForSender(emailAddress, displayName); }; which was correct. @@ +2755,5 @@ > + "chrome://messenger/skin/icons/phishing.png", > + this.mMsgNotificationBar.PRIORITY_CRITICAL_MEDIUM, > + buttons); > + > + var XULNS = "http://www.mozilla.org/keymaster.gatekeeper/there.is.only.xul"; Again, should be var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
Attachment #647473 -
Flags: feedback?(irving) → feedback+
Comment 87•12 years ago
|
||
How does this look in MacOSX?
Attachment #647473 -
Attachment is obsolete: true
Attachment #651257 -
Flags: feedback?(irving)
Comment 88•12 years ago
|
||
Hm, I don't know why, but with v26 I don't get any notification bar at all on OS X (checked with some Facebook mails which normally gives me the yellow notification bar). I build this patch into comm-beta, because comm-central doesn't build for me currently, but I don't think this could be the reason (it applied cleanly and your older patch worked just fine).
Comment 89•12 years ago
|
||
Comment on attachment 651257 [details] [diff] [review] Make use of the toolkit's notificationbox to display notifications (v26) Review of attachment 651257 [details] [diff] [review]: ----------------------------------------------------------------- After I manually cleaned up the errors in the patch, it passed a couple of quick checks but would still need full testing ::: mail/base/content/mailWindowOverlay.js @@ +2700,5 @@ > .getFormattedString("alwaysLoadRemoteContentForSender2", > [emailAddress ? emailAddress : aMsgHdr.author]); > + var displayName = headerParser.extractHeaderAddressName(aMsgHdr.author); > + var brandName = this.brandBundle.getString('brandShortName'); > + var remoteContentMsg = this.mMtringBundle.getFormattedString('remoteContentBarMessage', should be mStringBundle ::: mail/locales/en-US/chrome/messenger/messenger.properties @@ +151,5 @@ > sentFolderName=Sent > draftsFolderName=Drafts > +temp# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. Looks like a big chunk of stuff got moved or pasted accidentally?
Attachment #651257 -
Flags: feedback?(irving) → feedback-
Comment 90•12 years ago
|
||
Attachment #651257 -
Attachment is obsolete: true
Attachment #655267 -
Flags: feedback?(irving)
Comment 91•12 years ago
|
||
Comment on attachment 655267 [details] [diff] [review] Make use of the toolkit's notificationbox to display notifications (v27) Review of attachment 655267 [details] [diff] [review]: ----------------------------------------------------------------- The patch for mail/locales/en-US/chrome/messenger/messenger.properties still inserts a duplicate copy of lines 4 through 152, and removes text starting partway through line 549 until partway through line 691 of the original file. Please review all the differences in this file carefully and revert any that don't relate specifically to this bug.
Attachment #655267 -
Flags: feedback?(irving) → feedback-
Comment 92•12 years ago
|
||
(In reply to Irving Reid (:irving) from comment #91) > Comment on attachment 655267 [details] [diff] [review] > Make use of the toolkit's notificationbox to display notifications (v27) > > Review of attachment 655267 [details] [diff] [review]: > ----------------------------------------------------------------- > > The patch for mail/locales/en-US/chrome/messenger/messenger.properties still > inserts a duplicate copy of lines 4 through 152, and removes text starting > partway through line 549 until partway through line 691 of the original > file. Please review all the differences in this file carefully and revert > any that don't relate specifically to this bug. Thanks for the feedback. I'm completely baffled as to what happened with that.
Comment 93•12 years ago
|
||
Ewong still working on this ?
Comment 94•12 years ago
|
||
Yes he is but he's got some higher priority stuff to deal with first.
Comment 95•11 years ago
|
||
Because I wanted to try the latest patch out, but it was bitrotted, I unbitrotted it and fixed the comments for messenger.properties in #91. Maybe this also will help others, therefore I've added it here.
Comment 96•11 years ago
|
||
Sorry, patch accidently missed some of the images...
Attachment #692451 -
Attachment is obsolete: true
Attachment #692511 -
Flags: review?(irving)
Comment 97•11 years ago
|
||
Simply removed all whitespace that was in the patch.
Attachment #692511 -
Attachment is obsolete: true
Attachment #692511 -
Flags: review?(irving)
Attachment #695687 -
Flags: review?(irving)
Comment 98•11 years ago
|
||
By the way, I don't think it's a good idea to hardcode all the images, because this will interfere with Bug 781333 and we will need 2x images of all png's (see also Bug 800123).
Comment 99•11 years ago
|
||
Comment on attachment 695687 [details] [diff] [review] v27 - unbitrotted to current trunk (2012-12-26) Review of attachment 695687 [details] [diff] [review]: ----------------------------------------------------------------- I'm seeing warning messages on stderr, WARNING: We should have hit the document element...: file /Users/ireid/tbird/comm-central/mozilla/layout/xul/base/src/nsBoxObject.cpp, line 177 I suspect these are relatively harmless side effects of the GetNotificationBox call, but I can't remember for sure. The code works with my tests, as it stands - it still needs UI review and cleaning up of the icon references as pointed out by Nomis101.
Attachment #695687 -
Flags: review?(irving) → feedback+
Updated•11 years ago
|
Attachment #655267 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #632538 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #614572 -
Attachment is obsolete: true
Updated•11 years ago
|
Comment 100•11 years ago
|
||
Attachment #468697 -
Attachment is obsolete: true
Attachment #511015 -
Attachment is obsolete: true
Attachment #575742 -
Attachment is obsolete: true
Attachment #613971 -
Attachment is obsolete: true
Attachment #702433 -
Flags: ui-review?(bwinton)
Comment 101•11 years ago
|
||
Attachment #702435 -
Flags: ui-review?(bwinton)
Comment 102•11 years ago
|
||
Attachment #702436 -
Flags: ui-review?(bwinton)
Comment 103•11 years ago
|
||
Attachment #702437 -
Flags: ui-review?(bwinton)
Comment 104•11 years ago
|
||
Comment on attachment 702433 [details]
Scam warning
I think we should be using a white shield logo, and a white "x", but other than that, ui-r=me!
Thanks,
Blake.
Attachment #702433 -
Flags: ui-review?(bwinton) → ui-review+
Comment 105•11 years ago
|
||
Comment on attachment 702435 [details]
Remote content warning
We seem to be saying "remote content" a lot. Could we change the "always load" message to be a little shorter, so that the main message doesn't have to wrap?
Attachment #702435 -
Flags: ui-review?(bwinton) → ui-review-
Comment 106•11 years ago
|
||
Comment on attachment 702436 [details]
Message Disposition Notification question
As the first attachment, the icon and "x" should be in white. I also think we can tighten up the wording so that it doesn't take two lines.
Attachment #702436 -
Flags: ui-review?(bwinton) → ui-review-
Comment 107•11 years ago
|
||
Comment on attachment 702437 [details]
Junk Message warning
What is the "?"? (Other than that, I like it!)
Attachment #702437 -
Flags: ui-review?(bwinton) → ui-review+
Comment 108•11 years ago
|
||
(In reply to Blake Winton (:bwinton) from comment #107) > Comment on attachment 702437 [details] > Junk Message warning > > What is the "?"? (Other than that, I like it!) The "?" button brings up one of those pull-down overlays from the top of the window, containing a brief explanation of our junk mail system.
Comment 109•11 years ago
|
||
Okay, let's pick a better label for that button, too, then. ;)
Assignee | ||
Comment 110•11 years ago
|
||
Comment on attachment 702436 [details]
Message Disposition Notification question
(MDN, not DSN)
This already have better text, the patch just isn't using what we have.
Attachment #702436 -
Attachment description: Delivery Status Notification warning → Message Disposition Notification question
Assignee | ||
Comment 111•11 years ago
|
||
(In reply to Blake Winton (:bwinton) from comment #105) > Comment on attachment 702435 [details] > Remote content warning > > We seem to be saying "remote content" a lot. Could we change the "always > load" message to be a little shorter, so that the main message doesn't have > to wrap? Or perhaps have something like what the firefox popup blocker uses? (A "Preferences" button.)
Assignee | ||
Comment 112•11 years ago
|
||
I think this bug would be well worth persuing...
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 113•11 years ago
|
||
This is an updated version of what was earlier put together. This bug is long enough to give anyone an headache trying to read it. I think earlier colors of the phishing and remote content icons was requested to be changed. I haven't done that but besides that the patch should be working. There was an info icon added earlier in the patches, i removed that as we can choose not to specify one and get the toolkit notification info icon by using a suitable priority. Not in this bug, but for remote content it would be a huge win to be able to whitelist content from some certain domains (not senders, but from the domain the content is on). This new bar should make ui for that way easier. I'll attach some screenshots
Attachment #695687 -
Attachment is obsolete: true
Attachment #702436 -
Attachment is obsolete: true
Attachment #770401 -
Flags: ui-review?(bwinton)
Attachment #770401 -
Flags: review?(bwinton)
Assignee | ||
Comment 114•11 years ago
|
||
Assignee | ||
Comment 115•11 years ago
|
||
Comment 116•11 years ago
|
||
Comment on attachment 770401 [details] [diff] [review] proposed fix, v28 ui-r=me, based on the screenshots. One thing I will suggest is that the black looks really dark. Can we change it to the yellow that got a previous ui-r+ earlier in this bug? (review will come later, but if anyone else wants to steal it, please go ahead…)
Attachment #770401 -
Flags: ui-review?(bwinton) → ui-review+
Assignee | ||
Comment 117•11 years ago
|
||
The notification colors are all not "tampered" with, they come from what toolkit is using and depends on what priority you set for the notification. I didn't change what was given ui-r before, but i guess that screenshot is from another os (osx? mine were on ubuntu). Or toolkit changed from yellow to black for warning notifications. Let me know if you still want it changed. One thing i'd still change is the [Edit...] button on drafts. That should be just [Edit] (no dots)
Comment 118•11 years ago
|
||
(In reply to Magnus Melin from comment #117) > I didn't change what was given ui-r before, but i guess that screenshot is > from another os (osx? mine were on ubuntu). Yes, the screenshots with the yellow bar are from OS X.
Comment 119•11 years ago
|
||
Okay, I'll check out how it looks when I review the code on Mac… Thanks!
Comment 120•11 years ago
|
||
Comment on attachment 770401 [details] [diff] [review] proposed fix, v28 So, I think this is probably okay, but every time I run it and try to flag a message as junk (with the "j" key), I get the following assertion: Assertion failure: _mOwningThread.GetThread() == PR_GetCurrentThread() (nsMsgFolderNotificationService not thread-safe), at /Volumes/SSD/Programming/thunderbird/comm-central/mailnews/base/src/nsMsgFolderNotificationService.cpp:17 So, uh, obviously that needs to get fixed first. ;) >+++ b/mail/base/content/mailWindowOverlay.js >@@ -2764,158 +2759,265 @@ function HandleJunkStatusChanged(folder) > // Only reload message if junk bar display state has changed. >- if (msgHdr && junkBarWasDisplayed != gMessageNotificationBar.isFlagSet(kMsgNotificationJunkBar)) >+ if (msgHdr && junkBarWasDisplayed != gMessageNotificationBar.isShowingJunkNotification()) > { >+ I don't think the extra blank line here adds much. >- // If the current row isn't going to change, reload to show sanitized or >+// XXXmagnus: the below doesn't really work for non-inbox... >+ >+ // If the current row isn't going to change, reload to show sanitized or We should fix that before committing. ;) > var gMessageNotificationBar = > { >+ get mStringBundle() { >+ delete this.stringBundle; >+ return this.stringBundle = document.getElementById("bundle_messenger"); >+ }, >+ >+ get mBrandBundle() { >+ delete this.brandBundle; >+ return this.brandBundle = document.getElementById("brand_bundle"); Why are we deleting it every time we get it? Can't we cache it at all? >+++ b/mail/base/jar.mn >@@ -10,17 +10,17 @@ messenger.jar: >- content/messenger/mailWindowOverlay.js (content/mailWindowOverlay.js) >+* content/messenger/mailWindowOverlay.js (content/mailWindowOverlay.js) So, as I understand it, this means that we're going to pre-process the mailWindowOverlay.js file, which is something I think we try to avoid doing. Is there an attribute on something we could use instead? At worst, we could make a variable of our own. Something like http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/telemetry/TelemetryPing.js#123 >+++ b/mail/test/mozmill/composition/test-attachment-reminder.js >@@ -6,61 +6,64 @@ > // I'm not sure why this is the ID. But it is. :/ >+// XXX for ^^^ nBox.appendNotification("", "1", in MsgComposeCommands.js > const kNotificationID = "1"; Can we change the comment above, instead of adding an "XXX"? Apart from those, I'm pretty happy with the patch. r=me with the above things fixed. Thanks, Blake.
Attachment #770401 -
Flags: review?(bwinton) → review+
Assignee | ||
Comment 121•11 years ago
|
||
(In reply to Blake Winton (:bwinton) from comment #120) > >+ get mBrandBundle() { > >+ delete this.brandBundle; > >+ return this.brandBundle = document.getElementById("brand_bundle"); > > Why are we deleting it every time we get it? Can't we cache it at all? Yeah i don't know why its done like this. It seems to be a common pattern though. Anyone know why it's done this way?
Comment 122•11 years ago
|
||
I have not seen this pattern anywhere. The bundles are usually cached.
Assignee | ||
Comment 123•11 years ago
|
||
http://mxr.mozilla.org/comm-central/search?string=delete+this.bundle&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central lists a few, you can search more of |delete this.|
Comment 124•11 years ago
|
||
> Why are we deleting it every time we get it? Can't we cache it at all? Yes this code pattern is supposed to cache the bundle. Unfortunately it's been cargo culted, and cargo culted *wrong*. + get mStringBundle() { + delete this.stringBundle; + return this.stringBundle = document.getElementById("bundle_messenger"); + }, Should be: + get stringBundle() { + delete this.stringBundle; + return this.stringBundle = document.getElementById("bundle_messenger"); + }, So it's supposed to be a lazy getter which deletes itself the first time it's called. The second time around it's a javascript property instead. Better MXR query: http://mxr.mozilla.org/mozilla-central/search?string=delete+this.&find=%2Fbrowser%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Comment 125•11 years ago
|
||
OK, so the only difference is that the 'bundle' should be the same string in: get bundle() { delete this.bundle; return this.bundle = ... ; } So the patch needs to be fixed with this.
Assignee | ||
Comment 127•11 years ago
|
||
This should address the review comments. Added Application.platformIsWindows for platform checking. I also had to fix some more tests. I don't see the assertion, so i assume that's something unrelated that has gone away.
Attachment #770401 -
Attachment is obsolete: true
Attachment #778871 -
Flags: ui-review+
Attachment #778871 -
Flags: review?(bwinton)
Assignee | ||
Comment 128•11 years ago
|
||
Unbitrotted
Attachment #778871 -
Attachment is obsolete: true
Attachment #778871 -
Flags: review?(bwinton)
Attachment #814173 -
Flags: ui-review+
Attachment #814173 -
Flags: review?(bwinton)
Assignee | ||
Comment 129•11 years ago
|
||
Blake: ping on the re-review
Comment 130•11 years ago
|
||
Comment on attachment 814173 [details] [diff] [review] proposed fix, v30 So, I can't build on OS X 10.9, but I went through the differences between this and the previously r+ed version, and they all make sense to me, so I'll say r=me based on that. (Obviously, I'm assuming the tests all pass and stuff too. ;) Thanks, Blake.
Attachment #814173 -
Flags: review?(bwinton) → review+
Comment 131•11 years ago
|
||
(In reply to Magnus Melin from comment #129) > Blake: ping on the re-review Magnus can you start a try build ?
Assignee | ||
Comment 132•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=89c0015e6e75
Assignee | ||
Comment 133•11 years ago
|
||
Try looks good.
Assignee | ||
Comment 134•11 years ago
|
||
https://hg.mozilla.org/comm-central/rev/752e81dfb822 -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 28.0
Comment 135•7 years ago
|
||
Adding some keywords to summary to improve archaeological search results. Had a hard time figuring out that this is where [Show Remote Content] Button was replaced by [Options] dropdown button.
Depends on: 1062961
Summary: notification bar needs to look slicker → Notification bar needs to look slicker (Remote Content, Junk, Scam, Send Return Receipt, Edit Draft)
You need to log in
before you can comment on or make changes to this bug.
Description
•