Closed Bug 1093870 Opened 5 years ago Closed 5 years ago

Devedition Theme: the pinned tab glow is a bit much

Categories

(Firefox :: Theme, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 36
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: bgrins, Assigned: vporof)

References

Details

Attachments

(4 files, 6 obsolete files)

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 v2Splinter 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: 5 years ago
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.