Closed Bug 479279 Opened 13 years ago Closed 12 years ago

fennec should have a system for browser-level notifications

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(fennec1.0b2+)

VERIFIED FIXED
Tracking Status
fennec 1.0b2+ ---

People

(Reporter: madhava, Assigned: mfinkle)

References

Details

Attachments

(1 file, 2 obsolete files)

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
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0b2+
My initial thought is to override alerts.xul, so the nsIAlertsService will still be the front-end to this system.
Even better, we can override the Alerts Service implementation to use a box in the Fennec stack. Patch coming.
Assignee: nobody → mark.finkle
Attached patch WIP 1 (obsolete) — Splinter Review
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>
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #371192 - Attachment is obsolete: true
Attachment #371310 - Flags: review?(gavin.sharp)
Attached patch patch 2Splinter Review
Same as previous patch but adds support for text-clickable and the alert listener.
Attachment #371310 - Attachment is obsolete: true
Attachment #371618 - Flags: review?(gavin.sharp)
Attachment #371310 - Flags: review?(gavin.sharp)
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?
Attachment #371618 - Flags: review?(gavin.sharp) → review+
(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.
http://hg.mozilla.org/mobile-browser/rev/278856ffac9b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.