453470 - notification bar in fennec needs to be per tab.

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Doug Turner (:dougt)]

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.

Comment 1

[Stuart Parmenter]

madhava, neil, any thoughts here?

Comment 2

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

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

Comment 3

[Stuart Parmenter]

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.

Comment 4

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

Attachment: WIP 1: Add notifications back

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.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 6

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

Attachment: WIP 2: Show notifications when present

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.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

It appears that there are 90 more tests in toolkit/content/tests/widgets/test_notificationbox.xul

Comment 8

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

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

Comment 9

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

Attachment: WIP 3: fixed bitrot

Applies to trunk now

Comment 10

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

Attachment: WIP 4: slight changes - acts more like a panel

This patch tries to make the notificationbox act more like a panel

Comment 11

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

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?

Comment 12

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

http://hg.mozilla.org/mobile-browser/rev/ff2b0ee8b49e
http://hg.mozilla.org/mobile-browser/rev/b99d2e373e58

this is only a partial fix for this bug - not closing bug

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.1

Add a 'TabSelect' listener per notification.

Sounds like what it says in #c2 but not exactly.

Comment 15

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

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.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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.

Comment 17

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

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

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

(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.

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

Trying to get rid as much as possible of Fennec inner structure.

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Updated patch

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.

Comment 21

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

pushed: http://hg.mozilla.org/mobile-browser/rev/27f098ef85d5

(yes, I messed up the checkin comment. I am too dependent on qimportbz)

Comment 22

[Aakash Desai [:aakashd]]

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