Closed Bug 1093870 Opened 10 years ago Closed 10 years ago

Devedition Theme: the pinned tab glow is a bit much

Categories

(Firefox :: Theme, defect)

Product:
Firefox
Component:
Theme
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 36
Tracking Flags:
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: bgrins, Assigned: vporof)

References

Details

Attachments

(4 files, 6 obsolete files)

glow.png
20.40 KB, image/png
Details
v2
1.62 KB, patch
MattN
: review+
Details | Diff | Splinter Review
implementation glow v2
40.24 KB, image/jpeg
Details
non-retina-screenshot.png
8.93 KB, image/png
shorlander
: ui-review+
Details
Attached image pinned-tab.png (obsolete) — 
See screenshot
Group: mozilla-employee-confidential
Attached image glow.png — 
Design from shorlander
The easiest way I've found to test this is to open https://tbpl.mozilla.org/ in a pinned tab, click on a tree link then switch to a different tab.
Much easier way to test - open a pinned tab with: data:text/html,<!DOCTYPE html><html><meta charset=utf8><link rel=icon href="https://abs.twimg.com/favicons/favicon.ico"<div id=counter></div><script>var countr = document.getElementById('counter');setInterval(function foo(){ countr.textContent = document.title = Date.now(); }, 2000);</script>
Attached patch pinned-tab-glow.patch (obsolete) — — Splinter Review 
An idea of what it could look like.  Keeping the current glow but making it more subtle may be a safer way to go over redoing the whole gradient.
Do you still want me to take a look at this, Brian?
Assignee: nobody → vporof
(In reply to Victor Porof [:vporof][:vp] from comment #6)
> Do you still want me to take a look at this, Brian?

Please.  There is something a bit weird with the radial gradient where there is a black gap that makes it appear to be hanging over the nav bar: https://www.dropbox.com/s/k3kcsgm6e61x7ed/Screenshot%202014-11-04%2016.49.33.png?dl=0.  I haven't yet quite been able to figure out the radial gradient syntax enough to fix that, so if you could take over that would be great.
So, I'm not sure exactly what you should do to get rid of that dark px at the bottom of the glow.  Maybe it would be possible to get the gradient to cover that box-shadow on the nav-bar?  It seems like it's somehow doing this on Australis, but I don't know how.
Attached image glow-wip.png (obsolete) — 
Also, ni? shorlander for UX help (but I believe he may not be around for the next day or two).  Stephen, for a fix to the pinned tab glow this week, do you prefer something like this (a more subtle variation of the current one) or something like: https://bugzilla.mozilla.org/attachment.cgi?id=8517009.
Flags: needinfo?(shorlander)
Status: NEW → ASSIGNED
Attached patch pinned-glow.patch (obsolete) — — Splinter Review 

        Attachment #8516969 -
        Attachment is obsolete: true

        Attachment #8517081 -
        Attachment is obsolete: true

        Attachment #8517109 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached image
          
        implementation glow.png (obsolete)
        — 
      

    


  

    
  

        Attachment #8517610 -
        Flags: review?(bgrinstead)

    
    

    
  
Comment on attachment 8517611 [details]
implementation glow.png

What do you think about this pinned tab glow, Stephen?
Flags: needinfo?(shorlander)

        Attachment #8517611 -
        Flags: ui-review?(shorlander)

    
    

    
  
Comment on attachment 8517610 [details] [diff] [review]
pinned-glow.patch

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

Forwarding this review request to Matt

        Attachment #8517610 -
        Flags: review?(bgrinstead) → review?(MattN+bmo)

    
    

    
  
Comment on attachment 8517610 [details] [diff] [review]
pinned-glow.patch

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

::: browser/themes/shared/devedition.inc.css
@@ +199,5 @@
>    background-color: var(--tab-background-color);
>  }
>  
> +.tabbrowser-tab[pinned][titlechanged]:not([selected="true"]) > .tab-stack > .tab-content {
> +  background-image: radial-gradient(farthest-corner at center, rgba(76, 158, 217, 0.9) 13%, rgba(0,0,0,0.5) 16%, rgba(29, 79, 115, 0) 70%);

Nit: remove the spaces after the commas inside rgba(…)

@@ +201,5 @@
>  
> +.tabbrowser-tab[pinned][titlechanged]:not([selected="true"]) > .tab-stack > .tab-content {
> +  background-image: radial-gradient(farthest-corner at center, rgba(76, 158, 217, 0.9) 13%, rgba(0,0,0,0.5) 16%, rgba(29, 79, 115, 0) 70%);
> +  background-position: center bottom -15px;
> +  background-size: 100% 107%;

How did you come up with 107%? Use a calc to show the math.

        Attachment #8517610 -
        Flags: review?(MattN+bmo)

    
  
No longer depends on: 1094505

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v2 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to Matthew N. [:MattN] (focused on Loop) from comment #14)
> Comment on attachment 8517610 [details] [diff] [review]
> pinned-glow.patch
> 
> Review of attachment 8517610 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/shared/devedition.inc.css
> @@ +199,5 @@
> >    background-color: var(--tab-background-color);
> >  }
> >  
> > +.tabbrowser-tab[pinned][titlechanged]:not([selected="true"]) > .tab-stack > .tab-content {
> > +  background-image: radial-gradient(farthest-corner at center, rgba(76, 158, 217, 0.9) 13%, rgba(0,0,0,0.5) 16%, rgba(29, 79, 115, 0) 70%);
> 
> Nit: remove the spaces after the commas inside rgba(…)
> 

Done

> @@ +201,5 @@
> >  
> > +.tabbrowser-tab[pinned][titlechanged]:not([selected="true"]) > .tab-stack > .tab-content {
> > +  background-image: radial-gradient(farthest-corner at center, rgba(76, 158, 217, 0.9) 13%, rgba(0,0,0,0.5) 16%, rgba(29, 79, 115, 0) 70%);
> > +  background-position: center bottom -15px;
> > +  background-size: 100% 107%;
> 
> How did you come up with 107%? Use a calc to show the math.

There is no math. The circle glow is simply a subtle ellipse instead of a perfect circle.

        Attachment #8517610 -
        Attachment is obsolete: true

        Attachment #8517829 -
        Flags: review?(MattN+bmo)

    
    

    
  
Comment on attachment 8517829 [details] [diff] [review]
v2

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

::: browser/themes/shared/devedition.inc.css
@@ +200,5 @@
>  }
>  
> +.tabbrowser-tab[pinned][titlechanged]:not([selected="true"]) > .tab-stack > .tab-content {
> +  background-image: radial-gradient(farthest-corner at center, rgba(76,158,217,0.9) 13%,rgba(0,0,0,0.5) 16%,rgba(29,79,115,0) 70%);
> +  background-position: center bottom -15px;

What about -15px?

    
    

    
  
(In reply to Matthew N. [:MattN] (focused on Loop) from comment #16)
> Comment on attachment 8517829 [details] [diff] [review]
> 
> What about -15px?

The distance between the center of the gradient to where the glow should be... Again, this is simply visual stuff, not depending on anything, nor something that should be depended on.

    
    

    
  
Comment on attachment 8517611 [details]
implementation glow.png

I like the way it looks. But it seems kind of jaggy in that screenshot.
Flags: needinfo?(bgrinstead)

        Attachment #8517611 -
        Flags: ui-review?(shorlander) → ui-review-

    
    

    
  
(In reply to Stephen Horlander [:shorlander] from comment #18)
> Comment on attachment 8517611 [details]
> implementation glow.png
> 
> I like the way it looks. But it seems kind of jaggy in that screenshot.
Flags: needinfo?(bgrinstead) → needinfo?(vporof)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v2
        — 
          — Splinter Review
      

    


  
This should make more sense and doesn't use magic numbers.

        Attachment #8517611 -
        Attachment is obsolete: true

        Attachment #8517829 -
        Attachment is obsolete: true

        Attachment #8517829 -
        Flags: review?(MattN+bmo)
Flags: needinfo?(vporof)

        Attachment #8518452 -
        Flags: review?(MattN+bmo)

    
    

    
  

      
      
      
      

        Attached image
          
        implementation glow v2
        — 
      

    


  

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #19)
> (In reply to Stephen Horlander [:shorlander] from comment #18)
> > Comment on attachment 8517611 [details]
> > implementation glow.png
> > 
> > I like the way it looks. But it seems kind of jaggy in that screenshot.

What you're seeing is a 200% zoom because this is a retina screenshot. Scale it down to 100% and it's perfectly smooth.

    
    

    
  


  
Sounds like the jaggedness was just an artifact of a retina screenshot - how does this look?

        Attachment #8518475 -
        Flags: ui-review?(shorlander)

    
    

    
  
Comment on attachment 8518475 [details]
non-retina-screenshot.png

(In reply to Brian Grinstead [:bgrins] from comment #23)
> Created attachment 8518475 [details]
> non-retina-screenshot.png
> 
> Sounds like the jaggedness was just an artifact of a retina screenshot - how
> does this look?

Still looks squared off, but I think it's ok. I don't think radial-gradients anti-alias well. Might want to replace it with an image at some point.

        Attachment #8518475 -
        Flags: ui-review?(shorlander) → ui-review+

    
    

    
  
Comment on attachment 8518452 [details] [diff] [review]
v2

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

::: browser/themes/shared/devedition.inc.css
@@ +199,5 @@
>    background-color: var(--tab-background-color);
>  }
>  
> +.tabbrowser-tab[pinned][titlechanged]:not([selected="true"]) > .tab-stack > .tab-content {
> +  background-image: radial-gradient(22px at center calc(100% - 2px), rgba(76,158,217,0.9) 13%,rgba(0,0,0,0.4) 16%,rgba(29,79,115,0) 70%);

Change the "2px" to "1px" so that it's a half-circle. The 1px comes from tabToolbarNavbarOverlap so you should mention that in a comment.
You should put a space after the commas separating the gradient stops.

        Attachment #8518452 -
        Flags: review?(MattN+bmo) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/7f0e90fc4932
Status: ASSIGNED → RESOLVED
Closed: 10 years ago

          status-firefox36:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36

    
  
Depends on: 1096371

    
  
Group: mozilla-employee-confidential

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: