Closed
Bug 960144
Opened 11 years ago
Closed 11 years ago
Toast Notifications Image - Implementation
Categories
(Firefox for Metro Graveyard :: Shell, defect, P2)
Tracking
(firefox28 verified, firefox29 verified)
VERIFIED
FIXED
Firefox 29
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.
Reporter | ||
Updated•11 years ago
|
Whiteboard: [triage] [feature] p=0 → [release28] [feature] p=0
Reporter | ||
Updated•11 years ago
|
Whiteboard: [release28] [feature] p=0 → [release28] [feature] p=2
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → rsilveira
Reporter | ||
Updated•11 years ago
|
Updated•11 years ago
|
Component: Metro Operations → Shell
Product: Tracking → Firefox for Metro
Version: --- → Trunk
Assignee | ||
Comment 1•11 years ago
|
||
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 2•11 years ago
|
||
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+
Comment 3•11 years ago
|
||
Marina, does the image get pulled from the application icon? That was my intention for the proposed design.
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
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 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
With whitespace nits addressed.
Attachment #8365422 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8365373 -
Attachment is obsolete: true
Assignee | ||
Comment 8•11 years ago
|
||
Comment 9•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Whiteboard: [release28] [feature] p=2 → [release28] [feature] p=2 [approval-mozilla-aurora=metro-only]
Comment 10•11 years ago
|
||
status-firefox28:
--- → fixed
status-firefox29:
--- → fixed
Keywords: checkin-needed
Whiteboard: [release28] [feature] p=2 [approval-mozilla-aurora=metro-only] → [release28] [feature] p=2
Comment 11•11 years ago
|
||
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)
Comment 12•11 years ago
|
||
You have to navigate away from the browser in order for the toast notification to appear.
Assignee | ||
Comment 13•11 years ago
|
||
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)
Comment 14•11 years ago
|
||
(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.
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•