Closed Bug 904662 Opened 11 years ago Closed 11 years ago

Layered Infobar Effect for Multiple Notifications

Categories

(Firefox for Metro Graveyard :: Downloads, defect)

Product:
Firefox for Metro Graveyard
Component:
Downloads
Platform:
x86_64
Windows 8
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 26

People

(Reporter: emtwo, Assigned: emtwo)

References

Details

Attachments

(2 files, 2 obsolete files)

layers.png
(deleted), image/png
Details
v3: Show layered notifications
(deleted), patch
mbrubeck
: review+
Details | Diff | Splinter Review
No description provided.
Blocks: 893066
Assignee: nobody → msamuel
Attached patch v1: Show layered notifications (obsolete) (deleted) — — Splinter Review
* I wanted to override removeNotification() and simply call super.removeNotification() (or similar) and then add my own stuff but I couldn't find something similar, so some code is duplicated. * It looks like Sam won't be adding a binding for bug 893091, but I think it may be useful for bug 904702 since we want to override functionality for the [X] button.
Attachment #793498 - Flags: review?(mbrubeck)
Attached image layers.png (deleted) —
Comment on attachment 793498 [details] [diff] [review] v1: Show layered notifications Review of attachment 793498 [details] [diff] [review]: ----------------------------------------------------------------- The styling looks good! But I think some of the code could be approached differently (see below). Sorry for the hassle. :( ::: browser/metro/base/content/bindings/notification.xml @@ +29,5 @@ > + <method name="_adjustLayers"> > + <body><![CDATA[ > + // Clear whatever layers existed. > + this._setHidden('layer1', true); > + this._setHidden('layer2', true); Rather than do this in code, I think it would be better (simpler, easier to review) to do this in pure CSS if possible, something like this: .notification-layer { visibility: collapse; } notification + notification:last-child > vbox > .notification-layer { visibility: visible; } @@ +43,5 @@ > + > + <method name="showNotification"> > + <parameter name="aLabel"/> > + <parameter name="aValue"/> > + <parameter name="aImage"/> One downside of extending the binding is that code outside our control might not use this new method; it will still use the old appendNotification method. And XBL in Gecko has no good way of overriding a method while still calling the version from the base binding -- often you have to duplicate code, as with removeNotification below. :( So if we do extend the binding, I think we'd want to listen for events dispatched by the base binding, rather than adding this new method. @@ +78,5 @@ > + <content> > + <xul:vbox flex="1" xbl:inherits="type"> > + <html:div anonid="layer2" class="notification-layer" hidden="true"></html:div> > + <xul:vbox flex="1" xbl:inherits="type"> > + <html:div anonid="layer1" class="notification-layer" hidden="true"></html:div> Maybe we can even add these layer elements in our CSS using generated content (:after or :before), without extending the <notification> binding either? Nit: Indentation does not match structure.
Attachment #793498 - Flags: review?(mbrubeck) → review-
Attached patch v2: Show layered notifications (obsolete) (deleted) — — Splinter Review
This patch has a minor bug that I was hoping to get advice on, but otherwise it works as expected. I was unable to completely get rid of the binding because I couldn't get the bar to look/act the way we want with :after/:before. Also, since there is no event fired for a closed notification, I had to add one in removeNotification. The bug seems to be that "AlertClose" is not dispatched when a notification being removed is not the top notification. This can be seen when a download completes and it looks like there are 2 notifications but there should be one. I assumed there might be an exception thrown by dispatchEvent() but when I tried to explore that I ended up at bug 574941 which I think is why I couldn't see any exceptions. Any ideas what the issue could be? If not, perhaps for AlertClose we can use an observer instead of dispatching an event?
Attachment #793498 - Attachment is obsolete: true
Attachment #796343 - Flags: feedback?(mbrubeck)
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/AlertClose indicates that the target of an AlertClose event should be the notificationbox, not the item. So: + let event = new Event('AlertClose'); + aItem.dispatchEvent(event); ..should be: + let event = new Event('AlertClose'); + this.dispatchEvent(event);
Comment on attachment 796343 [details] [diff] [review] v2: Show layered notifications Review of attachment 796343 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Marina Samuel [:emtwo] from comment #4) > The bug seems to be that "AlertClose" is not dispatched when a notification > being removed is not the top notification. Did sfoster's suggestion above help with this? Would it be easier to go back to layers on the <notification> itself, and sibling or :nth-child selectors to activate them? The patch looks good as long as we can work out the bugs. ::: browser/metro/base/content/bindings/notification.xml @@ +39,5 @@ > + this._removeNotificationElement(aItem); > + > + // Fire notification closed event. > + let event = new Event('AlertClose'); > + aItem.dispatchEvent(event); If sfoster's suggestion doesn't work, try wraping a try/catch around here to test your exception hypothesis. ::: browser/metro/base/content/browser.js @@ +1695,5 @@ > // we're intercepting touch input. > let notification = this._notification = document.createElement("notificationbox"); > + this._notification.setAttribute("count", 0); > + notification.addEventListener("AlertActive", this, true); > + notification.addEventListener("AlertClose", this, false); Could you handle these in your notificationbox binding instead of in browser.js? ::: browser/metro/theme/platform.css @@ +448,5 @@ > } > > +notificationbox[count="0"] .notification-layer, > +notificationbox[count="1"] .notification-layer, > +notificationbox[count="2"] [anonid="layer2"] { Please change [anonid="layer2"] to .notification-layer[anonid="layer2"] for performance: https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS#Universal_Rules @@ +454,5 @@ > +} > + > +notificationbox[count="2"] [anonid="layer1"], > +notificationbox[count="3"] [anonid="layer1"], > +notificationbox[count="3"] [anonid="layer2"] { (same here)
Attachment #796343 - Flags: feedback?(mbrubeck) → feedback+
Attached patch v3: Show layered notifications (deleted) — — Splinter Review
sfoster's suggestion worked :) Thanks for pointing that out, sfoster! Also made other suggested changes.
Attachment #796343 - Attachment is obsolete: true
Attachment #797992 - Flags: review?(mbrubeck)
Attachment #797992 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/mozilla-central/rev/cb7653a95aa8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: