823176 - (australis-tabs-linux) Australis tabs styling for Linux

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement)

Description

[Matthew N. [:MattN]]

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

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1

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.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 2

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

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 3

This fixes the tab labels in tabs-on-bottom mode.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 6

[Matthew N. [:MattN]]

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.

Comment 7

[u428464]

Does this patch also include new icons for the tabstrip ?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 4

De-bitrotted, and taking advantage of shared tabs code from patch in bug 823180.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: Interdiff WIP Patch 4 -> WIP Patch 4.1

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.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Interdiff WIP Patch 4 -> WIP Patch 4.1

Uploading the right file this time.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Interdiff WIP Patch 4 -> Patch 4.1

Alright, I think I'm ready for review here.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4.1

Ok, here's the final patch.

Comment 15

[Matthew N. [:MattN]]

(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

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Follow-up - missing tab-active-middle.png

To be applied on top of Patch v4.1

Comment 17

[Matthew N. [:MattN]]

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?

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4.2

(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).

Comment 19

[Dão Gottwald [:dao]]

> > > -#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.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

(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?

Comment 21

[Dão Gottwald [: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.

Comment 22

[Matthew N. [:MattN]]

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.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4.3 (r+'d by MattN)

(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.

Comment 24

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/7d9922f790c4