fennec should have a system for browser-level notifications

VERIFIED FIXED

Status

VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: madhava, Assigned: mfinkle)

Tracking

(Blocks: 1 bug)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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
(Reporter)

Updated

10 years ago
tracking-fennec: --- → ?

Updated

10 years ago
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
Created attachment 371192 [details] [diff] [review]
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>
Created attachment 371310 [details] [diff] [review]
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.
Attachment #371192 - Attachment is obsolete: true
Attachment #371310 - Flags: review?(gavin.sharp)
Created attachment 371618 [details] [diff] [review]
patch 2

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
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 9

8 years ago
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.