Closed Bug 960144 Opened 6 years ago Closed 6 years ago

Toast Notifications Image - Implementation

Categories

(Firefox for Metro Graveyard :: Shell, defect, P2)

x86_64
Windows 8.1
defect

Tracking

(firefox28 verified, firefox29 verified)

VERIFIED FIXED
Firefox 29
Tracking Status
firefox28 --- verified
firefox29 --- verified

People

(Reporter: MarcoM, Assigned: rsilveira)

References

()

Details

(Whiteboard: [release28] [feature] p=2)

Attachments

(3 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #893856 +++

Implementation of the designs attached to resolved Bug 893856.
Whiteboard: [triage] [feature] p=0 → [release28] [feature] p=0
Whiteboard: [release28] [feature] p=0 → [release28] [feature] p=2
Assignee: nobody → rsilveira
Blocks: metrov1it23
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
QA Contact: jbecerra
Component: Metro Operations → Shell
Product: Tracking → Firefox for Metro
Version: --- → Trunk
Attached patch Patch v1 (obsolete) — Splinter Review
The toast layout is totally controlled by windows, we can only choose from the templates at [1]. I've added an option to show the ToastText03 template when an image is not provided, and removed the image from the download toast. It seems like download is the only toast we currently support, but I kept the image support in case we add some other toast.

We can't change the logo image on the bottom right corner, windows uses the metro application icon. There is an option to hide the logo [2], but it's currently not supported.

[1] - http://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.notifications.toasttemplatetype.Aspx
[2] - http://msdn.microsoft.com/en-us/library/windows/apps/br230843.aspx
Attachment #8364070 - Flags: review?(msamuel)
Comment on attachment 8364070 [details] [diff] [review]
Patch v1

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

Looks good overall. My only request is to please factor out some of the repetitive code if possible. Also, in case mmaslaney isn't following the bug we should let him know that we can't do anything about the image. Any idea if we have control over the notification colour?

::: widget/windows/winrt/ToastNotificationHandler.cpp
@@ +103,5 @@
> +
> +  SetNodeValueString(title, titleTextNodeRoot.Get(), toastXml.Get());
> +  SetNodeValueString(msg, msgTextNodeRoot.Get(), toastXml.Get());
> +
> +  ComPtr<IToastNotification> notification;

I think from this line down until the end of the function is identical to code in DisplayNotification(). We could perhaps have them both call a separate function with this.
Attachment #8364070 - Flags: review?(msamuel) → review+
Marina, does the image get pulled from the application icon? That was my intention for the proposed design.
Attached image toasts.png
Yes, the image does get pulled from the application icon. My only concern is that the image doesn't smoothly blend with the background colour currently (see attachment) like you wanted in your design.
Attached patch Patch v2 (obsolete) — Splinter Review
Refactored a bit, sorry for the laziness previously :). r? again for sanity checking.

We don't have direct control over the bg color for the toast. Windows gets it from the app icon it seems. The results look pretty close to what we expect though.
Attachment #8364070 - Attachment is obsolete: true
Attachment #8365373 - Flags: review?(msamuel)
Comment on attachment 8365373 [details] [diff] [review]
Patch v2

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

Nice, thank you!

::: widget/windows/winrt/ToastNotificationHandler.cpp
@@ +80,5 @@
> +
> +ComPtr<IXmlDocument>
> +ToastNotificationHandler::InitializeXmlForTemplate(ToastTemplateType templateType) {
> +  ComPtr<IXmlDocument> toastXml;
> +  

nit: whitespace
Attachment #8365373 - Flags: review?(msamuel) → review+
Attached patch Patch v3Splinter Review
With whitespace nits addressed.
Attachment #8365422 - Flags: review+
Attachment #8365373 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5d96e38e3785
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Keywords: checkin-needed
Whiteboard: [release28] [feature] p=2 → [release28] [feature] p=2 [approval-mozilla-aurora=metro-only]
https://hg.mozilla.org/releases/mozilla-aurora/rev/899194fcd81f
Keywords: checkin-needed
Whiteboard: [release28] [feature] p=2 [approval-mozilla-aurora=metro-only] → [release28] [feature] p=2
Attached image Screenshot1.png
With both latest Nightly and Aurora on Win 8 64-bit, I can't see the toast notifications image. I tried downloading an image and a .pdf file.

All I can see is the old notification, as displayed in the attached screenshot.

Any thoughts/suggestions?
Flags: needinfo?(rsilveira)
You have to navigate away from the browser in order for the toast notification to appear.
Yes, what Marina said. Start a download and go to desktop (win+d will show you desktop) and you should see the toast once the download is over.
Flags: needinfo?(rsilveira)
(In reply to Rodrigo Silveira [:rsilveira] from comment #13)
> Yes, what Marina said. Start a download and go to desktop (win+d will show
> you desktop) and you should see the toast once the download is over.

Thanks Rodrigo and Marina! Verified as fixed, for iteration #23, with both latest Nightly and Aurora on Win 8 64-bit.
Status: RESOLVED → VERIFIED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.