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
(deleted), image/png
Details
v2
(deleted), patch
MattN
: review+
Details | Diff | Splinter Review
implementation glow v2
(deleted), image/jpeg
Details
non-retina-screenshot.png
(deleted), image/png
shorlander
: ui-review+
Details
Attached image pinned-tab.png (obsolete) (deleted) —
See screenshot
Group: mozilla-employee-confidential
Attached image glow.png (deleted) —
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) (deleted) — — 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) (deleted) —
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) (deleted) — — 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) (deleted) —
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) (deleted) — — 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 (deleted) — — 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 (deleted) —
(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.
Attached image non-retina-screenshot.png (deleted) —
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: