Closed
Bug 453470
Opened 16 years ago
Closed 15 years ago
notification bar in fennec needs to be per tab.
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(fennec1.0+)
VERIFIED
FIXED
fennec1.0a2
Tracking | Status | |
---|---|---|
fennec | 1.0+ | --- |
People
(Reporter: dougt, Assigned: vingtetun)
References
Details
Attachments
(2 files, 6 obsolete files)
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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•16 years ago
|
||
madhava, neil, any thoughts here?
Flags: wanted-fennec1.0+
Target Milestone: --- → Fennec A2
Comment 2•16 years ago
|
||
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•16 years ago
|
||
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
Comment 4•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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
Comment 7•16 years ago
|
||
It appears that there are 90 more tests in toolkit/content/tests/widgets/test_notificationbox.xul
Updated•16 years ago
|
Attachment #353692 -
Flags: review?(gavin.sharp)
Comment 8•16 years ago
|
||
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•16 years ago
|
||
Applies to trunk now
Attachment #353692 -
Attachment is obsolete: true
Attachment #353861 -
Flags: review?(gavin.sharp)
Attachment #353692 -
Flags: review?(gavin.sharp)
Comment 10•16 years ago
|
||
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 11•16 years ago
|
||
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+
Comment 12•16 years ago
|
||
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
Updated•15 years ago
|
tracking-fennec: --- → 1.0+
Assignee | ||
Comment 14•15 years ago
|
||
Add a 'TabSelect' listener per notification. Sounds like what it says in #c2 but not exactly.
Comment 15•15 years ago
|
||
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.
Assignee | ||
Comment 16•15 years ago
|
||
(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.
Updated•15 years ago
|
Assignee: mark.finkle → 21
Comment 17•15 years ago
|
||
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-
Assignee | ||
Comment 18•15 years ago
|
||
(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
Assignee | ||
Comment 19•15 years ago
|
||
Trying to get rid as much as possible of Fennec inner structure.
Attachment #392260 -
Attachment is obsolete: true
Assignee | ||
Comment 20•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #393163 -
Flags: review?(mark.finkle) → review+
Comment 21•15 years ago
|
||
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: 15 years ago
Resolution: --- → FIXED
Comment 22•15 years ago
|
||
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.
Description
•