Closed Bug 823180 (australis-tabs-osx) Opened 11 years ago Closed 11 years ago

Australis tabs styling for OS X

Categories

(Firefox :: Theme, enhancement)

All
macOS
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: MattN, Assigned: MattN)

References

()

Details

(Whiteboard: [depends on windows implementation])

Attachments

(1 file, 4 obsolete files)

Once bug 738491 (Winstripe) and bug 813786 (LWT) are reviewed, we can begin porting the Australis tabs design to OS X (Pinstripe).
Alias: australis-tabs-osx
Blocks: 826689
(In reply to Matthew N. [:MattN] from comment #0)
We're no longer blocking porting on LWT.
Assignee: nobody → mnoorenberghe+bmo
Status: NEW → ASSIGNED
Summary: Australis tabs styling for Pinstripe → Australis tabs styling for OS X
Whiteboard: [waiting on winstripe reviews] → [depends on windows implementation]
This builds on top of patches in bug 738491.
Attached image Screenshot of WIP1 (obsolete) —
(In reply to Matthew N. [:MattN] from comment #3)
> Created attachment 719404 [details]
> Screenshot of WIP1

Looks very good ! Same remark concerning the close buttons though : they are a bit greyer on the mockups.
I forgot to add that the text of unselected tabs should be grey too.
There is a known issue with the seams of background tabs but they are not caused by this patch AFAICT and it seems to be a rounding issue so I'd like to handle that outside this bug.

(In reply to Guillaume C. [:ge3k0s] from comment #4)
> Looks very good ! Same remark concerning the close buttons though : they are
> a bit greyer on the mockups.

We'll handle this in bug 851001 since there was a discrepancy with Windows mockups and there is no Hi-DPI image in the OS X mockups.

(In reply to Guillaume C. [:ge3k0s] from comment #5)
> I forgot to add that the text of unselected tabs should be grey too.

Fixed.
Attachment #719400 - Attachment is obsolete: true
Attachment #719404 - Attachment is obsolete: true
Attachment #724809 - Flags: review?(mconley)
Attachment #724809 - Flags: review?(dao)
Comment on attachment 724809 [details] [diff] [review]
v.1 Australis tabs styling for OS X

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

Hey Matt,

Everything in here looks OK to me. The shared tabs.inc.css include continues to make me really, really happy.

-Mike
Attachment #724809 - Flags: review?(mconley) → review+
(In reply to Matthew N. [:MattN] from comment #6)
> (In reply to Guillaume C. [:ge3k0s] from comment #5)
> > I forgot to add that the text of unselected tabs should be grey too.
> 
> Fixed.

I have tested UX branch on OSX this morning and text of unselected tabs hadn't appeared grey. Does someone see it grey ?
(In reply to Guillaume C. [:ge3k0s] from comment #8)
> (In reply to Matthew N. [:MattN] from comment #6)
> > (In reply to Guillaume C. [:ge3k0s] from comment #5)
> > > I forgot to add that the text of unselected tabs should be grey too.
> > 
> > Fixed.
> 
> I have tested UX branch on OSX this morning and text of unselected tabs
> hadn't appeared grey. Does someone see it grey ?

I did lighten the label color for all tabs but forget to adjust the opacity for background tabs.
Comment on attachment 724809 [details] [diff] [review]
v.1 Australis tabs styling for OS X

> .tab-stack {
>   /* ensure stable tab height with and without toolbarbuttons on the tab bar */
>-  height: 26px;
>+  height: @tabHeight@;
> }

Is this still needed? Generally, if you need to set a height for OS X where you didn't need it for Windows, that seems suspicious.

> .tabbrowser-tab,
> .tabs-newtab-button {
>   -moz-appearance: none;
>   font: message-box;
>   font-weight: bold;
>   text-shadow: @loweredShadow@;
>-  margin: 0;
>-  padding: 0;
>   border: none;
>   text-align: center;
>   -moz-box-align: stretch;
> }

I thought the tab label shouldn't be centered anymore.

>+.tabbrowser-tab:not(:-moz-lwtheme) {
>+  color: #333;
> }

Remove :not(:-moz-lwtheme), .tabbrowser-tab:-moz-lwtheme overrides this anyway.

> #TabsToolbar {
>   -moz-appearance: none;
>-  height: 26px;
>+  height: @tabHeight@;
>   background-repeat: repeat-x;
> }

Does the height still need to be specified here?

> #tabbrowser-tabs {
>   -moz-box-align: stretch;
>-  height: 26px;
>+  height: @tabHeight@;
> }

And here?

>+/* image preloading hack */
>+#TabsToolbar::after {
>+  content: '';
>+  display: block;
>+  background-image:
>+    url(chrome://browser/skin/tabbrowser/tab-active-middle.png),
>+    url(chrome://browser/skin/tabbrowser/tab-background-end.png),
>+    url(chrome://browser/skin/tabbrowser/tab-background-middle.png),
>+    url(chrome://browser/skin/tabbrowser/tab-background-start.png),
>+    url(chrome://browser/skin/tabbrowser/tab-stroke-end.png),
>+    url(chrome://browser/skin/tabbrowser/tab-stroke-start.png);
>+}

There's no point in preloading images that are displayed anyway in the selected tab, right?
Is any of this still useful in the first place? Seems like we didn't see the need for this in bug 738491.
Attachment #724809 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 724809 [details] [diff] [review]
> v.1 Australis tabs styling for OS X
> 
> > .tabbrowser-tab,
> > .tabs-newtab-button {
> >   -moz-appearance: none;
> >   font: message-box;
> >   font-weight: bold;
> >   text-shadow: @loweredShadow@;
> >-  margin: 0;
> >-  padding: 0;
> >   border: none;
> >   text-align: center;
> >   -moz-box-align: stretch;
> > }
> 
> I thought the tab label shouldn't be centered anymore.

Yup. They look much better with left-align, as per the spec. http://cl.ly/image/1T1X420X2s3p
* Fixed background tab label opacity (comment 9)
* Removed unnecessary heights since things seem to work fine with the min-heights
* Change tab label to left-alignment
* Removed preloading of selected tab images

(In reply to Dão Gottwald [:dao] from comment #10)
> Is any of this still useful in the first place? Seems like we didn't see the
> need for this in bug 738491.
I did sometimes notice a delay loading the images on first hover in a Windows debug build on a fast computer so I left this in.
Attachment #724809 - Attachment is obsolete: true
Attachment #729416 - Flags: review?(dao)
Hey Dao,

We're hoping to have this in an r+ state by April 3rd. Do you think you could take another look at this today?

Thanks,

-Mike
Flags: needinfo?(dao)
Comment on attachment 729416 [details] [diff] [review]
v.2 Address review comments and fix label opacity on bg. tabs

>+.tabbrowser-tab > .tab-stack > .tab-content > .tab-label:not([selected="true"]) {
>+  opacity: .7;
> }

remove ".tabbrowser-tab > .tab-stack > .tab-content > "
Attachment #729416 - Flags: review?(dao) → review+
Flags: needinfo?(dao)
Thanks for the reviews.
Attachment #729416 - Attachment is obsolete: true
Attachment #732698 - Flags: review+
Whiteboard: [depends on windows implementation] → [fixed-in-ux][depends on windows implementation]
Depends on: 857642
Depends on: 857886
Depends on: 858089
No longer depends on: 856107
Depends on: 865178
No longer blocks: 826689
Depends on: 868807
No longer depends on: 868807
Depends on: 869660
No longer depends on: 869660
Depends on: 873464
Depends on: 878023
Depends on: 879595
Depends on: 879602
Depends on: 879607
Depends on: 879679
Depends on: 882578
Depends on: 884492
Depends on: 887397
Depends on: 893065
Depends on: 897471
No longer depends on: 879607
Depends on: 839888
Depends on: 925884
Depends on: 932597
Depends on: 933964
Depends on: 932344
Restrict Comments: true
Depends on: 936112
https://hg.mozilla.org/mozilla-central/rev/bc3e29ff7277
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Restrict Comments: false
Whiteboard: [fixed-in-ux][depends on windows implementation] → [depends on windows implementation]
Target Milestone: --- → Firefox 28
Depends on: 941831
Depends on: 939010
Depends on: 898875
Depends on: 946768
Depends on: 945547
Depends on: 964621
Depends on: 964300
Depends on: 973694
Depends on: 979953
Depends on: 983761
Depends on: 989604
Depends on: 1218225
Depends on: 1246444
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: