479279 - fennec should have a system for browser-level notifications

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

In addition to the page-level notifications that come up attached to the titlebar (for things related to the current page -- e.g. "Fennec has blocked popups" or "This page would like to install something"), we need to have a lightweight mechanism for alerts about the _broswer_ that will fade away.  Some examples are messages like the following:

- download compete
- update available
- add-ons installed

and so on.  The mechanism I'd like to see is described here:

https://wiki.mozilla.org/Mobile/UI/Designs/TouchScreen/workingUI#Downloads

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

My initial thought is to override alerts.xul, so the nsIAlertsService will still be the front-end to this system.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Even better, we can override the Alerts Service implementation to use a box in the Fennec stack. Patch coming.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 1

This patch overrides the standard alerts service with a simple service that redirects the display of alerts to browser.js using a <hbox> in the <stack>

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

Basic alert service implementation. Should handle most use cases. Things to add:
* textClickable support (found in standard alerts service)
* progress indicator support (not found in standard alerts service)
* slide off-screen toward a UI element (not found in standard alerts service)

We could wait to land for any of these feature or we could add the feature/support in a followup bug.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

Same as previous patch but adds support for text-clickable and the alert listener.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 371618 [details] [diff] [review]
patch 2

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+function showAlertNotification(aImageURL, aTitle, aText, aTextClickable, aCookie, aListener) {
>+  AlertsHelper.showAlertNotification(aImageURL, aTitle, aText, aTextClickable, aCookie, aListener);

No need for the global function, right? Just have the service call it on AlertsHelper?

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>+    <hbox id="alerts-container" hidden="true" style="-moz-stack-sizing: ignore;" align="start" top="0" left="0" width="200"
>+          onclick="AlertsHelper.click(event);">
>+      <image id="alerts-image"/>
>+      <vbox>
>+        <label id="alerts-title" value=""/>
>+        <description id="alerts-text"/>

I wonder if it doesn't make sense to use a layout similar to the existing alert.xul... in particular, it uses labels for both the title and text, and sets the "value" attribute rather than setting textContent. Isn't one better than the other for wrapping?

>diff --git a/components/alertsService.js b/components/alertsService.js

>+  showAlertNotification: function(aImageUrl, aTitle, aText, aTextClickable, aCookie, aAlertListener, aName) {

>+    browser.showAlertNotification(aImageUrl, aTitle, aText);

Need to pass in the other arguments, presumably.

Also need to update the wince theme, right?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #6)
> >+function showAlertNotification(aImageURL, aTitle, aText, aTextClickable, aCookie, aListener) {
> >+  AlertsHelper.showAlertNotification(aImageURL, aTitle, aText, aTextClickable, aCookie, aListener);
> 
> No need for the global function, right? Just have the service call it on
> AlertsHelper?

Right. Fixed.

> >+    <hbox id="alerts-container" hidden="true" style="-moz-stack-sizing: ignore;" align="start" top="0" left="0" width="200"
> >+          onclick="AlertsHelper.click(event);">
> >+      <image id="alerts-image"/>
> >+      <vbox>
> >+        <label id="alerts-title" value=""/>
> >+        <description id="alerts-text"/>
> 
> I wonder if it doesn't make sense to use a layout similar to the existing
> alert.xul... in particular, it uses labels for both the title and text, and
> sets the "value" attribute rather than setting textContent. Isn't one better
> than the other for wrapping?

I thought about that, but I wanted to make sure the alert's height could grow (the width is fixed). Using a description and textContent and white-space: pre-wrap should be enough to get the description text to wrap. I forgot the pre-wrap so I added it.

> >+  showAlertNotification: function(aImageUrl, aTitle, aText, aTextClickable, aCookie, aAlertListener, aName) {
> 
> >+    browser.showAlertNotification(aImageUrl, aTitle, aText);
> 
> Need to pass in the other arguments, presumably.

Yep. Done.

> Also need to update the wince theme, right?

Oops. Done.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/mobile-browser/rev/278856ffac9b

Comment 9

[Anna (Waverley)]

VERIFIED FIXED on:

Build Id: Mozilla /5.0 (Android;Linux armv7l;rv:6.0a1) Gecko/20110523 Firefox/6.0a1 Fennec/6.0a1 

Device: HTC Desire Z (Android 2.2)