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: