Closed Bug 872742 Opened 11 years ago Closed 11 years ago

Work - Separate context bar and add slide in transition

Categories

(Firefox for Metro Graveyard :: App Bar, defect)

Product:
Firefox for Metro Graveyard
Component:
App Bar
Platform:
x86_64
Windows 8.1
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 24

People

(Reporter: rsilveira, Assigned: rsilveira)

References

Details

(Whiteboard: feature=work)

Attachments

(1 file, 1 obsolete file)

Patch v2
15.06 KB, patch
fryn
: review+
Details | Diff | Splinter Review 
We need to separate the context app bar and add the slide in transition.

This is blocking bug 860899 because a bunch of tests were relying on the transition.
Summary: change - Separate context bar and add slide in transition → Work - Separate context bar and add slide in transition
Whiteboard: feature=work
Attached patch Patch v1 (obsolete) — — Splinter Review 

        Attachment #750066 -
        Flags: review?(fyan)

    
    

    
  
Comment on attachment 750066 [details] [diff] [review]
Patch v1

Review of attachment 750066 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Thanks!
r+ with comment addressed.

::: browser/metro/theme/browser.css
@@ +547,4 @@
>  }
>  
>  #contextualactions-tray {
>    background-color: @metro_orange@;

You duplicated a rule here.

        Attachment #750066 -
        Flags: review?(fyan) → review+

    
    

    
  
Comment on attachment 750066 [details] [diff] [review]
Patch v1

Review of attachment 750066 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/theme/browser.css
@@ +538,5 @@
> +  transform: translateY(100%);
> +}
> +
> +#contextappbar[expanded] {
> +  transform: none;

Actually, you could do the following if you'd like:

#contextappbar:not([expanded]) {
  transform: translateY(100%);
}

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2
        — 
          — Splinter Review
      

    


  
Applied review comments.

Also, because we now have 2 appbars, I moved event handling from appbar.xml to appbar.js to trigger actions on the right appbar.

        Attachment #750066 -
        Attachment is obsolete: true

        Attachment #750123 -
        Flags: review?(fyan)

    
    

    
  
Comment on attachment 750123 [details] [diff] [review]
Patch v2

Review of attachment 750123 [details] [diff] [review]:
-----------------------------------------------------------------

Nice investigation. :)
r+ with comments addressed.

::: browser/metro/base/content/bindings/appbar.xml
@@ -11,5 @@
> -      <constructor>
> -        <![CDATA[
> -          window.addEventListener('MozContextUIShow', this);
> -          window.addEventListener('MozContextUIDismiss', this);
> -          window.addEventListener('MozAppbarDismiss', this);

Remove this line. It will result in an infinite loop, I think.

@@ -19,5 @@
> -      <destructor>
> -        <![CDATA[
> -          window.removeEventListener('MozContextUIShow', this);
> -          window.removeEventListener('MozContextUIDismiss', this);
> -          window.removeEventListener('MozAppbarDismiss', this);

And this one.

@@ +43,5 @@
>          <body>
>            <![CDATA[
> +            if (this.isShowing)
> +              return;
> +

Could you also add this type of guard for <method name="dismiss"> ?

@@ -83,5 @@
> -            switch (aEvent.type) {
> -              case 'MozContextUIShow':
> -                this.show();
> -                break;
> -              case 'MozAppbarDismiss':

And this one.

    
    

    
  
Comment on attachment 750123 [details] [diff] [review]
Patch v2

Review of attachment 750123 [details] [diff] [review]:
-----------------------------------------------------------------

Nice investigation. :)
r+ with comments addressed.

::: browser/metro/base/content/bindings/appbar.xml
@@ -11,5 @@
> -      <constructor>
> -        <![CDATA[
> -          window.addEventListener('MozContextUIShow', this);
> -          window.addEventListener('MozContextUIDismiss', this);
> -          window.addEventListener('MozAppbarDismiss', this);

Remove this line. It will result in an infinite loop, I think.

@@ -19,5 @@
> -      <destructor>
> -        <![CDATA[
> -          window.removeEventListener('MozContextUIShow', this);
> -          window.removeEventListener('MozContextUIDismiss', this);
> -          window.removeEventListener('MozAppbarDismiss', this);

And this one.

@@ +43,5 @@
>          <body>
>            <![CDATA[
> +            if (this.isShowing)
> +              return;
> +

Could you also add this type of guard for <method name="dismiss"> ?

@@ -83,5 @@
> -            switch (aEvent.type) {
> -              case 'MozContextUIShow':
> -                this.show();
> -                break;
> -              case 'MozAppbarDismiss':

And this one.

        Attachment #750123 -
        Flags: review?(fyan) → review+

    
    

    
  
Comment on attachment 750123 [details] [diff] [review]
Patch v2

Review of attachment 750123 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, wait. You did remove those lines! I was reading splinter incorrectly. <facepalm/>

    
    

    
  
OK, I was struggling to understand what you meant :) I'll add the guard to dismiss and land this. Thanks!

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/5f5d8b3adbc6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
OS: Windows 8 Metro → Windows 8.1

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: