Jim Mathies [:jimm] Reporter
	Description • 12 years ago

These are currently being used for things like download completion, copy text, and bookmarks created. Will attach a screen cap of how they currently look.

	Jonathan Wilde [:jwilde]
	Comment 2 • 12 years ago

Restyling these to match the Win 8 wouldn't be that hard.

However, in the case of download notifications, it would be really awesome to have these notifications appear on the desktop/other apps.  Would it be possible to use the OS-wide native toast notifications for this?

http://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.notifications.toastnotificationmanager

	Jim Mathies [:jimm] Reporter
	Comment 3 • 12 years ago

(In reply to Jonathan Wilde [:jwilde] from comment #2)
> Restyling these to match the Win 8 wouldn't be that hard.
> 
> However, in the case of download notifications, it would be really awesome
> to have these notifications appear on the desktop/other apps.  Would it be
> possible to use the OS-wide native toast notifications for this?
> 
> http://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.
> notifications.toastnotificationmanager

We have to work out background downloads but yes, I think the download manager (or whatever we use to download files in the background after the browser loses foreground status) could pick and choose which ui to invoke. For starters though I think we want to get the in-content alerts looking more metroish.

Download issues on suspend are being tracked by bug 769400.

	Jonathan Wilde [:jwilde]
	Comment 4 • 12 years ago

Okay, cool.  Sounds like a plan.  :D

	Jim Mathies [:jimm] Reporter
	Comment 5 • 12 years ago

These are used for bookmarks, downloads, and text copying. Maybe a few other simple notifications too.

	Asa Dotzler [:asa]
	Comment 6 • 12 years ago

Is the goal here to convert all of these to Windows 8 toast notifications?  

The usage guidelines for toast notifications are here http://msdn.microsoft.com/en-us/library/windows/apps/hh779727.aspx and http://msdn.microsoft.com/en-us/library/windows/apps/hh465391.aspx 

Yuan, does this seem right? The doc says "Toast notifications are an optional part of the app experience and are intended to be raised only when your app is not the active foreground app", "Don't raise a toast notification when your app is in the foreground and a more contextual surface such as an inline element, Flyout, dialog, or app bar is available" and "A toast notification should be used only for information of high interest to the user, typically involving some form of user opt-in. " which makes sense for downloads but I'm not sure it makes sense for bookmarks or text copying.

My proposal to Yuan and team is that we use toasts for downloads when our app isn't focused and wee drop the bookmark created or text copied notices.

	Brian R. Bondy [:bbondy]
	Comment 7 • 11 years ago

Comment 6

	Yuan Wang(:Yuan) – Mobile Firefox Design Lead
	Comment 8 • 11 years ago

(In reply to Asa Dotzler [:asa] from comment #6)
> Is the goal here to convert all of these to Windows 8 toast notifications?  
> 
> The usage guidelines for toast notifications are here
> http://msdn.microsoft.com/en-us/library/windows/apps/hh779727.aspx and
> http://msdn.microsoft.com/en-us/library/windows/apps/hh465391.aspx 
> 
> Yuan, does this seem right? The doc says "Toast notifications are an
> optional part of the app experience and are intended to be raised only when
> your app is not the active foreground app", "Don't raise a toast
> notification when your app is in the foreground and a more contextual
> surface such as an inline element, Flyout, dialog, or app bar is available"
> and "A toast notification should be used only for information of high
> interest to the user, typically involving some form of user opt-in. " which
> makes sense for downloads but I'm not sure it makes sense for bookmarks or
> text copying.
> 
> My proposal to Yuan and team is that we use toasts for downloads when our
> app isn't focused and wee drop the bookmark created or text copied notices.

I agree with the guidelines. I think we should use toast notifications when the download is done in the background. And tapping on the notification should take the users to the download file system. 

For bookmarks and secondary tiles, we should present the notification via an in-context flyout. For example, after the user tap "Bookmark" icon, a flyout "Bookmark added" comes up above the bookmark icon. And the flyout automatically disappear after 2 seconds. 

For text copying, I don't think a notification is necessary. Most of the editing tools don't have notifications for that. And the users should know that copying is working if they can paste the copied content to somewhere else.

I will put together mockups for this bug. Thanks!

	Brian R. Bondy [:bbondy]
	Comment 9 • 11 years ago

Great, moving out of triage and into planning.

	Marco Mucci [:MarcoM]
	Comment 10 • 11 years ago

Hi Asa, moved into planning backlog.  See comments above for context.

	Asa Dotzler [:asa]
	Comment 11 • 11 years ago

(In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #8)
> (In reply to Asa Dotzler [:asa] from comment #6)
> > Is the goal here to convert all of these to Windows 8 toast notifications?  
> > 
> > The usage guidelines for toast notifications are here
> > http://msdn.microsoft.com/en-us/library/windows/apps/hh779727.aspx and
> > http://msdn.microsoft.com/en-us/library/windows/apps/hh465391.aspx 
> > 
> > Yuan, does this seem right? The doc says "Toast notifications are an
> > optional part of the app experience and are intended to be raised only when
> > your app is not the active foreground app", "Don't raise a toast
> > notification when your app is in the foreground and a more contextual
> > surface such as an inline element, Flyout, dialog, or app bar is available"
> > and "A toast notification should be used only for information of high
> > interest to the user, typically involving some form of user opt-in. " which
> > makes sense for downloads but I'm not sure it makes sense for bookmarks or
> > text copying.
> > 
> > My proposal to Yuan and team is that we use toasts for downloads when our
> > app isn't focused and wee drop the bookmark created or text copied notices.
> 
> I agree with the guidelines. I think we should use toast notifications when
> the download is done in the background. And tapping on the notification
> should take the users to the download file system. 
> 
> For bookmarks and secondary tiles, we should present the notification via an
> in-context flyout. For example, after the user tap "Bookmark" icon, a flyout
> "Bookmark added" comes up above the bookmark icon. And the flyout
> automatically disappear after 2 seconds. 
> 
> For text copying, I don't think a notification is necessary. Most of the
> editing tools don't have notifications for that. And the users should know
> that copying is working if they can paste the copied content to somewhere
> else.
> 
> I will put together mockups for this bug. Thanks!


OK. That sounds good. So what we're really asking for here is to wire complete download up to the system notifications and to have a flyout for bookmarked and pin to Windows Start.

Is there a reason to have this bug or should we simply add those two stories to their respective Epics?

	Jim Mathies [:jimm] Reporter
	Comment 12 • 11 years ago

> OK. That sounds good. So what we're really asking for here is to wire
> complete download up to the system notifications and to have a flyout for
> bookmarked and pin to Windows Start.
> 
> Is there a reason to have this bug or should we simply add those two stories
> to their respective Epics?

I think we can morph this bug into getting native toasts hooked up to alert service, and to make sure these alerts only show up when the browser is in the background.

For this -

> > I agree with the guidelines. I think we should use toast notifications when
> > the download is done in the background. And tapping on the notification
> > should take the users to the download file system. 

We'll need a fresh bug. Filed bug 867978.

	Marco Mucci [:MarcoM]
	Comment 13 • 11 years ago

Team thinks P3 Priority - looking for your input?

	Marina Samuel [:emtwo] Assignee
	Comment 14 • 11 years ago

Note: This patch should be applied on top of the patches in bug 883951.

Basic Tests Completed:
* Single & multiple downloads do not show a toast if FF is in foreground
* A single download gives a "Run download" toast if FF is not in foreground
* Multiple downloads give a "Show in files" toast if FF is not in foreground
* Single download toast clicked will run the download
* Multiple download toast clicked will show the download folder

	Matt Brubeck (:mbrubeck)
	Comment 17 • 11 years ago

Comment on attachment 774647 [details] [diff] [review]
V1: Hook up native toasts to nsIAlertService + use them for downloads

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

Looks good! Just some minor nits/suggestions below.  I haven't really looked at the C++ parts; I'll leave that to someone more qualified.

::: browser/metro/base/content/browser.xul
@@ -728,5 @@
>        </vbox>
>      </box>
> -
> -    <!-- alerts for content -->
> -    <hbox id="alerts-container" hidden="true" align="start" bottom="0" onclick="AlertsHelper.click(event);">

There are some styles for this in browser.css that you can now remove too.

::: browser/metro/base/content/helperui/AlertsHelper.js
@@ +13,2 @@
>  
> +    MetroUtils.showNativeToast(aTitle, aText, aImageURL);

This will fail on non-Metro platforms (i.e. -metrodesktop).  Just for development purposes, it might be nice to add a fallback that uses the standard alerts service -- either in a try/catch block here, or in the stub implementation in browser-scripts.js.

::: browser/metro/components/AlertsService.js
@@ +18,5 @@
>    classID: Components.ID("{fe33c107-82a4-41d6-8c64-5353267e04c9}"),
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIAlertsService]),
>  
>    showAlertNotification: function(aImageUrl, aTitle, aText, aTextClickable, aCookie, aAlertListener, aName) {
> + 	let browser = Services.wm.getMostRecentWindow("navigator:browser");

(looks like indentation accidentally changed to a tab on this line)

::: browser/metro/locales/en-US/chrome/browser.properties
@@ +35,2 @@
>  downloadShowInFiles=Show in Files
> +downloadsShowInFiles=Show them in the Files app

You should use PluralForm.jsm to handle the singular/plural differences here:
https://developer.mozilla.org/en/docs/Localization_and_Plurals#Developing_with_PluralForm

It looks like the singular "downloadShowInFiles" is not used (unless I missed something from another patch); do we plan to use it?

@@ +44,5 @@
>  alertDownloadsSize=Download too big
>  alertDownloadsNoSpace=Not enough storage space
>  alertDownloadSave=Do you want to run or save %S (%S) from %S?
>  alertDownloadMultiple=Downloading %S files, %S, %S
> +alertMultipleDownloadsComplete=%S downloads have been completed

...and here (even though the singular will be unused; some languages will need multiple plural forms).

::: widget/nsIWinMetroUtils.idl
@@ +68,5 @@
>  
>    /**
> +   * Displays a native Windows 8 toast.
> +   */
> +   void showNativeToast(in AString aTitle, in AString aMessage, in AString anImage);

You should change the uuid in this IDL file when changing the interface.

	Marina Samuel [:emtwo] Assignee
	Comment 18 • 11 years ago

In response to comment 17:

All suggested changes were made. Just had a couple of notes/questions:

1) In your suggestion to using the "standard alerts service" on a try/catch block. I wasn't sure what you meant by that. Did you mean keeping the existing implementation we had of nsIAlertsService and using that as a fallback? For now I made it so that it uses an infobar alert instead.

2) downloadShowInFiles is used in another patch (bug 883951). I pluralized it as suggested.

	Marina Samuel [:emtwo] Assignee
	Comment 19 • 11 years ago

Stephen, would we be able to get an icon/image for these toast notifications? If you take a look at the "Single Download Complete Toast" and "Multiple Download Complete Toast" screenshots attached, you can see where we'd like them to be placed.

	Marco Mucci [:MarcoM]
	Comment 20 • 11 years ago

Edits incorporated and work completed.  Will land as a bundle in a future iteration.

	Marina Samuel [:emtwo] Assignee
	Comment 21 • 11 years ago

* Updated patch with latest pull
* Made change so that in-app download-complete notifications don't show up if native notifications are clicked

	Jim Mathies [:jimm] Reporter
	Comment 22 • 11 years ago

Comment on attachment 786916 [details] [diff] [review]
v3: Hook up native toasts to nsIAlertService + use them for downloads

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

Note, ToastNotificationHandler.h/.cpp have windows line endings, please convert those to unix.

-- widget parts --

::: widget/windows/winrt/ToastNotificationHandler.cpp
@@ +50,5 @@
> +
> +  ComPtr<IToastNotification> notification;
> +  ComPtr<IToastNotificationFactory> factory;
> +  AssertHRESULT(GetActivationFactory(HStringReference(RuntimeClass_Windows_UI_Notifications_ToastNotification).Get(),
> +                            factory.GetAddressOf()));

nit - indent off

@@ +73,5 @@
> +}
> +
> +HRESULT ToastNotificationHandler::OnActivate(IToastNotification *notification, IInspectable *inspectable) {
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +  obs->NotifyObservers(nullptr, "nativetoastclicked", nullptr);

You can use MetroUtils::FireObserver, and lets add 'metro_' to the name, which kinda keeps with what we've been doing down here. 

http://mxr.mozilla.org/mozilla-central/search?find=%2Fwidget%2Fwindows%2Fwinrt%2F&string=MetroUtils%3A%3AFireObserver

::: widget/windows/winrt/ToastNotificationHandler.h
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#pragma once
> +
> +#include "windows.ui.notifications.h"

nit - <>

@@ +20,5 @@
> +                struct ToastNotification;
> +                struct IToastNotification;
> +                struct ToastNotificationFactory;
> +            }
> +        };

nit - ;

@@ +34,5 @@
> +                }
> +            }
> +        }
> +    }
> +};

Odd that we need these declarations, do the headers not pull these types in?

	Jim Mathies [:jimm] Reporter
	Comment 23 • 11 years ago

Comment on attachment 786916 [details] [diff] [review]
v3: Hook up native toasts to nsIAlertService + use them for downloads

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

well done!

::: browser/metro/base/content/downloads.js
@@ +3,5 @@
>   * 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/. */
>  
>  const URI_GENERIC_ICON_DOWNLOAD = "chrome://browser/skin/images/alert-downloads-30.png";
> +const TOAST_URI_GENERIC_ICON_DOWNLOAD = "ms-appx:///metro/chrome/chrome/skin/images/alert-downloads-30.png"

This doesn't seem to mesh well with the notification panel, looks like it needs a transparent background. Lets file a follow up to get the right icon.

::: browser/metro/components/AlertsService.js
@@ +20,5 @@
>  
>    showAlertNotification: function(aImageUrl, aTitle, aText, aTextClickable, aCookie, aAlertListener, aName) {
>      let browser = Services.wm.getMostRecentWindow("navigator:browser");
> +    try {
> +      browser.AlertsHelper.showAlertNotification(aImageUrl, aTitle, aText, aTextClickable, aCookie, aAlertListener);

under what circumstance would this throw?

	Marina Samuel [:emtwo] Assignee
	Comment 24 • 11 years ago

(In reply to Jim Mathies [:jimm] from comment #23)
> under what circumstance would this throw?

In Comment 17, mbrubeck says "This will fail on non-Metro platforms (i.e. -metrodesktop).  Just for development purposes, it might be nice to add a fallback that uses the standard alerts service -- either in a try/catch block here, or in the stub implementation in browser-scripts.js." so my understanding is that an exception is thrown if MetroUtils.showNativeToast() is called with -metrodesktop.

	Marina Samuel [:emtwo] Assignee
	Comment 25 • 11 years ago

(In reply to Jim Mathies [:jimm] from comment #22)
> Odd that we need these declarations, do the headers not pull these types in?

Actually didn't need the declarations. Thanks for pointing that out.

Landed in fx-team:
https://hg.mozilla.org/integration/fx-team/rev/7b37baf1e8b8

	Tim Taubert [:ttaubert] (inactive)
	Comment 26 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/7b37baf1e8b8