Closed Bug 875326 Opened 7 years ago Closed 7 years ago

Polish Australis tab stroke inner highlight

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: MattN, Assigned: MattN)

References

Details

(Whiteboard: [Australis:M6])

Attachments

(2 files, 2 obsolete files)

More detailed notes:
* I removed remove tab-active-middle-lwtheme.png because the updated remove tab-active-middle.png images from Stephen don't have the tab fill. This means we will always use fgTabTexture behind the stroke (tab-active-middle.png) rather than having the linear CSS gradient from start/end beside the image gradient in the middle which may not match perfectly.
* deleted redundant styles from browser/themes/linux/browser.css
* consolidate two ".tab-background-middle[selected=true]:-moz-lwtheme" rules

(In reply to Stephen Horlander from bug 858089 comment #17)
> Created attachment 752017 [details]
> Tab Images Linux/OS X
> 
> > Stephen: 
> > 1) What were the tab-texture-*.png images for in browser-aero.css? Was it
> > just that the colour should change in that case? Any reason why we can't
> > just override the colours in the linear-gradient instead of using an image?
> 
> I think it was the only way I could get it to layer properly. If you can do
> it another way it should be fine.

This patch fixes the layering of ::after, as it wasn't above ::before for some reason, which may help. With this patch there is a slight issue with LWT which seems to require a clip-path tweak. I'll do that in a later patch.
 
> > 3) Could you provide the revised tab-stroke-*.png images for OS X (+@2x) and
> > Linux? They should be like Windows where they only contain the stroke and
> > inner highlight.
> 
> Should be able to use these on OS X and Linux.

I thought your Windows images had the highlight but I don't see them now. Are they in some other bug or did I accidentally overwrite them locally?
Attachment #753287 - Flags: review?(mconley)
Flags: needinfo?(shorlander)
Status: NEW → ASSIGNED
(In reply to Matthew N. [:MattN] from comment #0)
> This patch fixes the layering of ::after, as it wasn't above ::before for
> some reason, which may help. With this patch there is a slight issue with
> LWT which seems to require a clip-path tweak. I'll do that in a later patch.

Might need to tweak the clip-path and the stroke to make it line up exactly.


> I thought your Windows images had the highlight but I don't see them now.
> Are they in some other bug or did I accidentally overwrite them locally?

The images from the WIP patch (https://bug858089.bugzilla.mozilla.org/attachment.cgi?id=733347) have all of the correct borders and highlights.
Flags: needinfo?(shorlander)
So, I'm confused here...on Windows, I'm not seeing the inner highlight on the selected tab's curvy strokes:

http://i.imgur.com/VzZdmro.png

I re-applied the tab-stroke-start and tab-stroke-end images that Stephen linked to, and same result.

Is this the way it's supposed to look?
Flags: needinfo?(shorlander)
(In reply to Mike Conley (:mconley) from comment #2)
> So, I'm confused here...on Windows, I'm not seeing the inner highlight on
> the selected tab's curvy strokes:
> 
> http://i.imgur.com/VzZdmro.png
> 
> I re-applied the tab-stroke-start and tab-stroke-end images that Stephen
> linked to, and same result.
> 
> Is this the way it's supposed to look?

No, my WIP patch had two images; one for the border/shadow and one for the highlight. I need to combine them into one.
Flags: needinfo?(shorlander)
Comment on attachment 753287 [details] [diff] [review]
v.0.9 Use new images from shorlander, fix layering of ::after, remove tab-active-middle-lwtheme.png

Ok, cancelling review until we get the new asset.
Attachment #753287 - Flags: review?(mconley)
(In reply to Mike Conley (:mconley) from comment #4)
> Comment on attachment 753287 [details] [diff] [review]
> v.0.9 Use new images from shorlander, fix layering of ::after, remove
> tab-active-middle-lwtheme.png
> 
> Ok, cancelling review until we get the new asset.

Linux and OS X already have the proper images so I was hoping to get review on those platforms.
Comment on attachment 753287 [details] [diff] [review]
v.0.9 Use new images from shorlander, fix layering of ::after, remove tab-active-middle-lwtheme.png

Ok, can do.
Attachment #753287 - Flags: review?(mconley)
Comment on attachment 753287 [details] [diff] [review]
v.0.9 Use new images from shorlander, fix layering of ::after, remove tab-active-middle-lwtheme.png

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

Code looks good, and testing on Linux and OSX shows the highlighted stroke. Looks good.
Attachment #753287 - Flags: review?(mconley) → review+
Changes since v0.9:
* Clip-path adjusted to work better with updates stroke images and to fix regression with the sliver when flexing tabs on Windows. (with 0.05 overlap).
* Tweaked @2x stroke images by making some pixels transparent in the bottom corner where the highlight intersects the nav-bar so it looks better. 
* Image swap for tab-active-middle@2x.png because it still had the tab fill and thus didn't match the start and end because the texture was missing/different. This is also for consistency with the 1x version
* Adjusted margin-bottom: 1px; of the separator in tabs.inc.css because they were overlapping the highlight instead of just the border.
* browser/themes/windows/browser.css: fixed the position of the #TabsToolbar border as it was being covered up after bug 858089

Stephen: The highlight is not perfect with dark themes (except HiDPI OS X) but the problem is not very noticeable with the default theme on Windows/OS X. The highlight seems to get a bit brighter near the bottom of the stroke. It seems like some pixels need tweaking by hand.  An easy way to test is with a solid black LWT.[1] See above for how I tweaked the @2x strokes. Do you want to try tweak the images some more? I'd like to land this in the next day so could we land with the existing images in the meantime?

https://addons.mozilla.org/en-US/firefox/addon/black-15433/
Attachment #753287 - Attachment is obsolete: true
Attachment #755668 - Flags: ui-review?(shorlander)
Attachment #755668 - Flags: review?(mconley)
(In reply to Matthew N. [:MattN] from comment #9)
> Stephen: The highlight is not perfect with dark themes (except HiDPI OS X)
> but the problem is not very noticeable with the default theme on Windows/OS
> X. The highlight seems to get a bit brighter near the bottom of the stroke.
> It seems like some pixels need tweaking by hand.  An easy way to test is
> with a solid black LWT.[1] See above for how I tweaked the @2x strokes. Do
> you want to try tweak the images some more? I'd like to land this in the
> next day so could we land with the existing images in the meantime?

I can tweak them as a followup. Should be a simple image swap once this is landed.

New Tab button seems odd: http://cl.ly/image/1R3r213y2G0f
(In reply to Stephen Horlander from comment #10)
> (In reply to Matthew N. [:MattN] from comment #9)
> > Stephen: The highlight is not perfect with dark themes (except HiDPI OS X)
> > but the problem is not very noticeable with the default theme on Windows/OS
> > X. The highlight seems to get a bit brighter near the bottom of the stroke.
> > It seems like some pixels need tweaking by hand.  An easy way to test is
> > with a solid black LWT.[1] See above for how I tweaked the @2x strokes. Do
> > you want to try tweak the images some more? I'd like to land this in the
> > next day so could we land with the existing images in the meantime?
> 
> I can tweak them as a followup. Should be a simple image swap once this is
> landed.
> 
> New Tab button seems odd: http://cl.ly/image/1R3r213y2G0f

That's bug 875894.
Comment on attachment 755668 [details] [diff] [review]
v.1 Adjusted clip-path, tweaked @2x images, and fix Windows border

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

This code all looks good to me. If shorlander is happy with the visual result, r=me.
Attachment #755668 - Flags: review?(mconley) → review+
Attachment #756757 - Flags: review?(mconley) → review+
https://hg.mozilla.org/projects/ux/rev/2b3f36112908
Flags: in-testsuite-
Whiteboard: [Australis:M6] → [Australis:M6][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/2b3f36112908
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M6][fixed-in-ux] → [Australis:M6]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.