Bug 823176 (australis-tabs-linux)

Australis tabs styling for Linux

RESOLVED FIXED in Firefox 28

Status

()

enhancement
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: MattN, Assigned: mconley)

Tracking

Trunk
Firefox 28
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [depends on Windows implementation], )

Attachments

(1 attachment, 10 obsolete attachments)

Once bug 738491 (Winstripe) and bug 813786 (LWT) are reviewed, we can begin porting the Australis tabs design to Linux (Gnomestripe).

Updated

7 years ago
Blocks: 826689
Assignee

Updated

7 years ago
Assignee: nobody → mconley
Posted patch WIP Patch 1 (obsolete) — Splinter Review
This is more or less a direct port so far. Still some cruft and commented out blocks of stuff here that you should probably not pay attention to.

Also, this patch looks awful with lightweight themes. Still working on that - just wanted to checkpoint my work.
Posted patch WIP Patch 2 (obsolete) — Splinter Review
Getting closer - there's still some detail work to do:

* Background tab separators do not match spec
* Lightweight themes look a bit odd, since the tab outline terminates abruptly at the bottom of the selected tab
* Background tab text is the wrong colour in tabs-on-bottom mode
Attachment #703368 - Attachment is obsolete: true
Posted patch WIP Patch 3 (obsolete) — Splinter Review
This fixes the tab labels in tabs-on-bottom mode.
Attachment #705586 - Attachment is obsolete: true
I think, like MattN, I'm going to try to deal with lw-theme issues in a separate follow-up bug. Also, I think the tab separators are off-spec for winstripe too. I'll likely be copying the same solution (if any) that we come up with from there, so I'll just back off that for now.

So, I think this patch is ready for some feedback / poking.
Assignee

Updated

7 years ago
Depends on: 834284
Comment on attachment 705884 [details] [diff] [review]
WIP Patch 3

Hey Andreas - you said you wanted to try this patch out?

Just apply on top of the UX branch. Let me know what you think.
Attachment #705884 - Flags: feedback?(bugs)
Hey Mike, could you update this to build atop the new patch in bug 738491 which moves the common Australis tab styles to themes/shared/.  You can take a look at the WIP OS X patch in bug 823180 which is already using that.
Status: NEW → ASSIGNED
Summary: Australis tabs styling for Gnomestripe → Australis tabs styling for Linux
Whiteboard: [waiting on winstripe reviews] → [depends on Windows implementation]

Comment 7

6 years ago
Does this patch also include new icons for the tabstrip ?
Comment on attachment 705884 [details] [diff] [review]
WIP Patch 3

This patch is way obsolete now that we're reaching the point where the different themes can share most of this stuff.
Attachment #705884 - Attachment is obsolete: true
Attachment #705884 - Flags: feedback?(bugs)
(In reply to Guillaume C. [:ge3k0s] from comment #7)
> Does this patch also include new icons for the tabstrip ?

Hey - nope, this is just tab styling.

(In reply to Matthew N. [:MattN] from comment #6)
> Hey Mike, could you update this to build atop the new patch in bug 738491
> which moves the common Australis tab styles to themes/shared/.  You can take
> a look at the WIP OS X patch in bug 823180 which is already using that.

Can do.
Posted patch WIP Patch 4 (obsolete) — Splinter Review
De-bitrotted, and taking advantage of shared tabs code from patch in bug 823180.
Apply on top of WIP Patch 4.

This gives the Linux GTK theme its own images. We're still stealing the Windows tab-active-middle.png, which makes the middle look a bit funky when using certain desktop themes. Waiting on shorlander for a new tab-active-middle.png.
Uploading the right file this time.
Attachment #720852 - Attachment is obsolete: true
Alright, I think I'm ready for review here.
Attachment #720854 - Attachment is obsolete: true
Posted patch Patch v4.1 (obsolete) — Splinter Review
Ok, here's the final patch.
Attachment #719662 - Attachment is obsolete: true
Attachment #723007 - Attachment is obsolete: true
Attachment #723009 - Flags: review?(mnoorenberghe+bmo)
(In reply to Mike Conley (:mconley) from comment #12)
> Created attachment 720854 [details] [diff] [review]
> Interdiff WIP Patch 4 -> WIP Patch 4.1
> 
> Uploading the right file this time.

The patch is missing tabbrowser/tab-active-middle.png and this caused a build error on UX:

RuntimeError: File "tabbrowser/tab-active-middle.png" not found
To be applied on top of Patch v4.1
Comment on attachment 723009 [details] [diff] [review]
Patch v4.1

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

The patch looks good.  I made a list of TODOs as a reminder:

1) Tab background middle still needs to be sorted out.
2) Tab Close button images can be handled in bug 851001.
3) New tab button image still needs to be handled.
4) For the close button and favicon+throbber, the margins/padding may need to be updated: https://people.mozilla.com/~shorlander/files/australis-designSpecs/images-linux/dimensions-tab.png
5) Need stroke images with inner highlight from Stephen IIRC.

I'd like to have the middle of the selected tab blending with the start and end (#1 above) and the TODO in the patch addressed before giving r+.

::: browser/themes/linux/browser.css
@@ +55,5 @@
>  #nav-bar[collapsed=true] + #customToolbars + #PersonalToolbar:not(:-moz-lwtheme),
>  #nav-bar[tabsontop=true],
>  #nav-bar[tabsontop=true][collapsed=true]:not([customizing]) + toolbar,
>  #nav-bar[tabsontop=true][collapsed=true]:not([customizing]) + #customToolbars + #PersonalToolbar {
> +  background-image: linear-gradient(@toolbarHighlight@, rgba(255,255,255,0)); /* TODO */

TODO! Please finish this or file a follow-up.

@@ -1642,5 @@
> -                    radial-gradient(circle farthest-corner at 50% 3px, rgba(233,242,252,1) 3%, rgba(172,206,255,.75) 40%, rgba(87,151,201,.5) 80%, rgba(87,151,201,0));
> -}
> -
> -#tabbrowser-tabs[positionpinnedtabs] > .tabbrowser-tab > .tab-stack > .tab-content[pinned] {
> -  min-height: 18px; /* corresponds to the max. height of non-textual tab contents, i.e. the tab close button */

Did you look into why this existed in the first place to be sure we won't regress some fix?
Attachment #723009 - Flags: review?(mnoorenberghe+bmo)
Attachment #723009 - Flags: feedback+
Posted patch Patch v4.2 (obsolete) — Splinter Review
(In reply to Matthew N. [:MattN] from comment #17)
> Comment on attachment 723009 [details] [diff] [review]
> Patch v4.1
> 
> Review of attachment 723009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks good.  I made a list of TODOs as a reminder:
> 
> 1) Tab background middle still needs to be sorted out.
> 2) Tab Close button images can be handled in bug 851001.
> 3) New tab button image still needs to be handled.
> 4) For the close button and favicon+throbber, the margins/padding may need
> to be updated:
> https://people.mozilla.com/~shorlander/files/australis-designSpecs/images-
> linux/dimensions-tab.png
> 5) Need stroke images with inner highlight from Stephen IIRC.
> 
> I'd like to have the middle of the selected tab blending with the start and
> end (#1 above) and the TODO in the patch addressed before giving r+.

Good eye - I hadn't noticed that the tab middle wasn't blending. Seems to look better now, and I tried it using a few different themes on Ubuntu.  The stroke looks a little dark sometimes, but we're still waiting on correct strokes from shorlander (#5).

> 
> ::: browser/themes/linux/browser.css
> @@ +55,5 @@
> >  #nav-bar[collapsed=true] + #customToolbars + #PersonalToolbar:not(:-moz-lwtheme),
> >  #nav-bar[tabsontop=true],
> >  #nav-bar[tabsontop=true][collapsed=true]:not([customizing]) + toolbar,
> >  #nav-bar[tabsontop=true][collapsed=true]:not([customizing]) + #customToolbars + #PersonalToolbar {
> > +  background-image: linear-gradient(@toolbarHighlight@, rgba(255,255,255,0)); /* TODO */
> 
> TODO! Please finish this or file a follow-up.
> 

It turns out this is fine just the way it is.

> @@ -1642,5 @@
> > -                    radial-gradient(circle farthest-corner at 50% 3px, rgba(233,242,252,1) 3%, rgba(172,206,255,.75) 40%, rgba(87,151,201,.5) 80%, rgba(87,151,201,0));
> > -}
> > -
> > -#tabbrowser-tabs[positionpinnedtabs] > .tabbrowser-tab > .tab-stack > .tab-content[pinned] {
> > -  min-height: 18px; /* corresponds to the max. height of non-textual tab contents, i.e. the tab close button */
> 
> Did you look into why this existed in the first place to be sure we won't
> regress some fix?

Yeah, this existed because on Linux there was some jitter when pinning and unpinning tabs due to the content collapsing and resizing the tab momentarily. I couldn't reproduce this issue using this patch though. I'm not sure this hack applies anymore, though needinfo'ing :dao in case he wants to chime in (since he appears to have added it).
Attachment #723009 - Attachment is obsolete: true
Attachment #723037 - Attachment is obsolete: true
Flags: needinfo?(dao)
Assignee

Updated

6 years ago
Attachment #725479 - Flags: review?(mnoorenberghe+bmo)
> > > -#tabbrowser-tabs[positionpinnedtabs] > .tabbrowser-tab > .tab-stack > .tab-content[pinned] {
> > > -  min-height: 18px; /* corresponds to the max. height of non-textual tab contents, i.e. the tab close button */
> > 
> > Did you look into why this existed in the first place to be sure we won't
> > regress some fix?
> 
> Yeah, this existed because on Linux there was some jitter when pinning and
> unpinning tabs due to the content collapsing and resizing the tab
> momentarily.

I'm not sure what this means. This code was needed to ensure positioned pinned tabs have the same height as normal tabs.
Flags: needinfo?(dao)
(In reply to Dão Gottwald [:dao] from comment #19)
> > > > -#tabbrowser-tabs[positionpinnedtabs] > .tabbrowser-tab > .tab-stack > .tab-content[pinned] {
> > > > -  min-height: 18px; /* corresponds to the max. height of non-textual tab contents, i.e. the tab close button */
> > > 
> > > Did you look into why this existed in the first place to be sure we won't
> > > regress some fix?
> > 
> > Yeah, this existed because on Linux there was some jitter when pinning and
> > unpinning tabs due to the content collapsing and resizing the tab
> > momentarily.
> 
> I'm not sure what this means. This code was needed to ensure positioned
> pinned tabs have the same height as normal tabs.

Perhaps I misunderstood the point of that line of CSS then - I was mainly looking at bug 621481, bug 617522, and bug 593570 for clues.

Anyhow, the tab start and ends appear to be setting minimum heights now. Do you agree that this rule is no longer required?
Flags: needinfo?(dao)
(In reply to Mike Conley (:mconley) from comment #20)
> (In reply to Dão Gottwald [:dao] from comment #19)
> > > > > -#tabbrowser-tabs[positionpinnedtabs] > .tabbrowser-tab > .tab-stack > .tab-content[pinned] {
> > > > > -  min-height: 18px; /* corresponds to the max. height of non-textual tab contents, i.e. the tab close button */
> > > > 
> > > > Did you look into why this existed in the first place to be sure we won't
> > > > regress some fix?
> > > 
> > > Yeah, this existed because on Linux there was some jitter when pinning and
> > > unpinning tabs due to the content collapsing and resizing the tab
> > > momentarily.
> > 
> > I'm not sure what this means. This code was needed to ensure positioned
> > pinned tabs have the same height as normal tabs.
> 
> Perhaps I misunderstood the point of that line of CSS then - I was mainly
> looking at bug 621481, bug 617522, and bug 593570 for clues.

This code was added in bug 579683.

> Anyhow, the tab start and ends appear to be setting minimum heights now. Do
> you agree that this rule is no longer required?

If that min height translates to .tab-content having a height of at least 18px, then this isn't needed anymore.
Flags: needinfo?(dao)
Comment on attachment 725479 [details] [diff] [review]
Patch v4.2

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

r=me!
#4 from my last comment can be handled in bug 826689.

::: browser/themes/linux/browser.css
@@ +12,5 @@
>  %include ../shared/browser.inc
>  %filter substitution
>  %define toolbarHighlight rgba(255,255,255,.3)
> +%define fgTabTexture linear-gradient(transparent 0px, transparent 1px, hsla(0,0%,100%,0.35) 1px, hsla(0,0%,100%,0.35) 2px, hsla(0,0%,100%,0.65) 2px, hsla(0,0%,100%,0.65) 3px, @toolbarHighlight@)
> +%define fgTabBackgroundMiddle @fgTabTexture@, linear-gradient(transparent 0px, transparent 1px, -moz-dialog 2px, -moz-dialog)

s/1px/2px/ for fgTabBackgroundMiddle. This will then match Windows and fixes the stroke colour on the middle with default Fedora theme.
Attachment #725479 - Flags: review?(mnoorenberghe+bmo) → review+
(In reply to Matthew N. [:MattN] from comment #22)
> Comment on attachment 725479 [details] [diff] [review]
> Patch v4.2
> 
> Review of attachment 725479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me!

\o/

> #4 from my last comment can be handled in bug 826689.

Sounds good.

> 
> ::: browser/themes/linux/browser.css
> @@ +12,5 @@
> >  %include ../shared/browser.inc
> >  %filter substitution
> >  %define toolbarHighlight rgba(255,255,255,.3)
> > +%define fgTabTexture linear-gradient(transparent 0px, transparent 1px, hsla(0,0%,100%,0.35) 1px, hsla(0,0%,100%,0.35) 2px, hsla(0,0%,100%,0.65) 2px, hsla(0,0%,100%,0.65) 3px, @toolbarHighlight@)
> > +%define fgTabBackgroundMiddle @fgTabTexture@, linear-gradient(transparent 0px, transparent 1px, -moz-dialog 2px, -moz-dialog)
> 
> s/1px/2px/ for fgTabBackgroundMiddle. This will then match Windows and fixes
> the stroke colour on the middle with default Fedora theme.

Done.
Attachment #725479 - Attachment is obsolete: true
Depends on: 513159
Whiteboard: [depends on Windows implementation] → [fixed-in-ux][depends on Windows implementation]
Assignee

Updated

6 years ago
Depends on: 858089
No longer blocks: 826689
Assignee

Updated

6 years ago
Depends on: 869063
Assignee

Updated

6 years ago
Depends on: 869084
Assignee

Updated

6 years ago
Depends on: 870160
No longer depends on: 513159
Restrict Comments: true
https://hg.mozilla.org/mozilla-central/rev/7d9922f790c4
Status: ASSIGNED → RESOLVED
Closed: 6 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: 940431
Blocks: 613259
Depends on: 994328
You need to log in before you can comment on or make changes to this bug.