notification bar in fennec needs to be per tab.

VERIFIED FIXED in fennec1.0a2

Status

defect
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: dougt, Assigned: vingtetun)

Tracking

Trunk
fennec1.0a2
x86
macOS
Dependency tree / graph
Bug Flags:
wanted-fennec1.0 +

Firefox Tracking Flags

(fennec1.0+)

Details

Attachments

(2 attachments, 6 obsolete attachments)

currently the notification bar in fennec is global (there is one for all of the tabs).  Some notifications, however, are really specific to the tab that they occur on.

we should make the notification bar per tab.
madhava, neil, any thoughts here?
Flags: wanted-fennec1.0+
Target Milestone: --- → Fennec A2
I had a brutish idea of keeping one <notificationbox>, but "linking" each contained <notification> to the tab. Then on TabSelect we can spin through the <noticiation>s and hide the ones not linked to the current tab.

Not sure if this idea breaks the <notificationbox> behavior or not, but it keeps us from needing to maintain a different notificationbox for each tab
The notificationbox doesn't really work how we want (for several reasons), in that we don't want to push the content down.  We really want to just use a panel and overlay the content here.
Assignee: nobody → mark.finkle
Posted patch WIP 1: Add notifications back (obsolete) — Splinter Review
This patch adds the <notificationbox> back as a sibling of the nav toolbar. The notifications display over the content _and_ any sidebars. This patch also fails to show the notifications if the view has been panned away from the top of the page. We need to add some kind of event for show/hide notifications so we can move the toolbar-container into position - like we do with the toolbar when show/hide sidebars.
We have some passwordmgr mochitests which fail as a result of this.  

Here are the two test files:
toolkit/components/passwordmgr/test/test_notifications.html
toolkit/components/passwordmgr/test/test_prompt.html

These two tests account for 440 tests which are not run as notifyBox is null.
This patch does the same as WIP 1, but forces the notification to become visible regardless of where the content has been panned and adds a close button to the notification.

The fact that the notifications are _over_ the content is a bit disconcerting for me.
Attachment #353355 - Attachment is obsolete: true
It appears that there are 90 more tests in toolkit/content/tests/widgets/test_notificationbox.xul
Attachment #353692 - Flags: review?(gavin.sharp)
Comment on attachment 353692 [details] [diff] [review]
WIP 2: Show notifications when present

I'm not really happy with this patch and the patch does not fix the bug (per tab notifications) but it does get notifications back into Fennec
Posted patch WIP 3: fixed bitrot (obsolete) — Splinter Review
Applies to trunk now
Attachment #353692 - Attachment is obsolete: true
Attachment #353861 - Flags: review?(gavin.sharp)
Attachment #353692 - Flags: review?(gavin.sharp)
This patch tries to make the notificationbox act more like a panel
Attachment #353861 - Attachment is obsolete: true
Attachment #353892 - Flags: review?(gavin.sharp)
Attachment #353861 - Flags: review?(gavin.sharp)
Comment on attachment 353892 [details] [diff] [review]
WIP 4: slight changes - acts more like a panel

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

>+notificationbox {
>+  -moz-binding: url("chrome://global/content/bindings/notification.xml#notificationbox");
>+  -moz-box-orient: vertical;

These are redundant (they're in xul.css); presumably left over from testing a custom binding?
Attachment #353892 - Flags: review?(gavin.sharp) → review+
Duplicate of this bug: 501142
tracking-fennec: --- → 1.0+
Posted patch Patch v0.1 (obsolete) — Splinter Review
Add a 'TabSelect' listener per notification.

Sounds like what it says in #c2 but not exactly.
Comment on attachment 391109 [details] [diff] [review]
Patch v0.1

This is looking pretty good. I want to test it a little.

Can you change the single quote strings to use double quotes?

Also, I need to think it using the selectedTab in the constructor will work well enough for most cases.
(In reply to comment #15)
> Can you change the single quote strings to use double quotes?

Done in a local patch, I'm waiting for the end of your test to upload it.

> Also, I need to think it using the selectedTab in the constructor will work
> well enough for most cases.

Yes, it's not very elegant, but I need a 'glue' and that the first relevant I've found. I'll look a bit more too in case I found something better.
Assignee: mark.finkle → 21
Comment on attachment 391109 [details] [diff] [review]
Patch v0.1

We'll need to listen for "TabClose" events, in case we have to cleanup any notifications when a tab is closed
Attachment #391109 - Flags: review-
Posted patch Patch v0.2 (obsolete) — Splinter Review
(In reply to comment #17)
> (From update of attachment 391109 [details] [diff] [review])
> We'll need to listen for "TabClose" events, in case we have to cleanup any
> notifications when a tab is closed

Oups! 

Corrected.
Attachment #391109 - Attachment is obsolete: true
Posted patch Patch v0.3 (obsolete) — Splinter Review
Trying to get rid as much as possible of Fennec inner structure.
Attachment #392260 - Attachment is obsolete: true
Posted patch Updated patchSplinter Review
The reason why I have move the destructor code on the previous iteration, was because it was never been called. In fact there is an open bug (bug 230086) for that.

In this patch I override the 'close' method of the parent binding to properly remove the binding and its listeners.
Attachment #393048 - Attachment is obsolete: true
Attachment #393163 - Flags: review?(mark.finkle)
Attachment #393163 - Flags: review?(mark.finkle) → review+
pushed: http://hg.mozilla.org/mobile-browser/rev/27f098ef85d5

(yes, I messed up the checkin comment. I am too dependent on qimportbz)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
verified FIXED on build:

Mozilla/5.0 (Macintosh; U; Intel Mac OSX 10.5; en-US; rv:1.9.2a2pre) Gecko/20090808 Fennec/1.0b3pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.