865178 - Australis tabs OS X lightweight theme support

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

Lightweight theme styling is needed for OS X similar to bug 823180 for Windows.  The main change involves keeping borders between the navbar and tabs when a theme is applied.

Stephen, since we use a solid color for for the selected tab on OS X, I wasn't sure what color and opacity should be used on the selected tab and toolbars below when lightweight themes were applied. Can you think of other differences from the Windows LWT approach? Thanks! Otherwise I have this mostly working.

Comment 1

[Matthew N. [:MattN]]

That should have been bug 813786 in comment 0 for Windows LWT support in Australis.

Comment 2

[Stephen Horlander [:shorlander]]

Attachment: Persona OSX - Light

It should look pretty much the same WRT to the tab strip.

Comment 3

[Stephen Horlander [:shorlander]]

Attachment: Persona OSX - Dark

Comment 4

[Matthew N. [:MattN]]

IIUC, shorlander said we can use the same color and opacity as Windows for the toolbar and selected tab.

Comment 5

[Matthew N. [:MattN]]

Attachment: v.1 Simplify toolbar borders and prevent overlapping

Note the copy that Splinter will not show properly:
copy from browser/themes/windows/browser-lightweightTheme.css
copy to browser/themes/osx/browser-lightweightTheme.css

I got rid of the overlapping 2px of toolbars because:
* the overlap became visible with the LWT semi-transparent overlay
* it's simpler IMO
* aligns with the Windows implementation

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 745503 [details] [diff] [review]
v.1 Simplify toolbar borders and prevent overlapping

How are the animation perf numbers on OS X?

Comment 7

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

Attachment: Latest patch applied to UX

This patch has bitrotted. After manually de-bitrotting, this is what I'm getting with a lw-theme applied. Something is up with this patch...

Comment 8

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

Comment on attachment 745503 [details] [diff] [review]
v.1 Simplify toolbar borders and prevent overlapping

Please see the screenshot I just posted.

Comment 9

[Matthew N. [:MattN]]

(In reply to Mike Conley (:mconley) from comment #7)
> This patch has bitrotted. After manually de-bitrotting, this is what I'm
> getting with a lw-theme applied. Something is up with this patch...

I'm rebuilding on top of latest UX so I haven't seen the problem from the screenshot yet but the patch applied without fuzz for me.

Comment 10

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

I am definitely getting patch fuzz using a recent pull of UX:

Mikes-MacBook-Pro:ux mikeconley$ hg qimport https://bugzilla.mozilla.org/attachment.cgi?id=745503 -n 865178
adding 865178 to series file

Mikes-MacBook-Pro:ux mikeconley$ hg qpush
applying 865178
patching file browser/themes/osx/browser.css
Hunk #1 FAILED at 0
Hunk #2 FAILED at 61
2 out of 3 hunks FAILED -- saving rejects to file browser/themes/osx/browser.css.rej
patching file browser/themes/osx/jar.mn
Hunk #1 FAILED at 14
1 out of 2 hunks FAILED -- saving rejects to file browser/themes/osx/jar.mn.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 865178
Mikes-MacBook-Pro:ux mikeconley$

Comment 11

[Matthew N. [:MattN]]

Attachment: v.1.1 Fix regression caused by bug 850918

Rebased with a regression fix for bug 850918 which I believe is what was shown in the screenshot.

(In reply to Dão Gottwald [:dao] from comment #6)
> How are the animation perf numbers on OS X?

I'm doing an opt build now and will post the numbers.

Comment 12

[Matthew N. [:MattN]]

Perf numbers (see bottom for OS X):
https://docs.google.com/spreadsheet/pub?key=0Av64yYvszN34dDdZX0FYWEd3MVpEVzdjYXlFcGpUQmc&single=true&gid=4&output=html

Comparing m-c and UX:
Frame intervals are essentially the same across the board. It seems like bug 676241 will help a little bit too.
Paint times increased for tab open without a LWT but decreased slightly for tab open and close with a LWT. It seems like the performance is acceptable to proceed for now. Note that there seems to be more noise than with Windows.

Comment 13

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

Attachment: Dragging a tab on OSX

With this latest patch, dragging the tab doesn't cause anything to be drawn for the background. I'm gonna go out on a limb and assume that this isn't intentional. :)

Comment 14

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

Comment on attachment 746013 [details] [diff] [review]
v.1.1 Fix regression caused by bug 850918

See previous comment and attachment.

Comment 15

[Matthew N. [:MattN]]

Attachment: v.2 Fix tab drag

This fixed the tab drag issue by making OS X work like Windows and Linux do.

I added the new define because we could assume the same gradient worked for both LWT and non-LWT on Windows/Linux but this is not the case on OS X where the gradient is opaque without a LWT.

I tested that this didn't regress Windows with or without a LWT and with scaling.

Comment 16

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

Comment on attachment 747104 [details] [diff] [review]
v.2 Fix tab drag

This looks good to me.

I tested this on Ubuntu, and noticed that the selected tab background is still stretched - but then I also found out that it's that way with or without your patch. So something is still kinda screwy, but it's not this patch's fault.

I'll file a bug for the Linux theme stuff.

Great job on this.

Comment 17

[Dão Gottwald [:dao]]

(In reply to Matthew N. [:MattN] from comment #15)
> I added the new define because we could assume the same gradient worked for
> both LWT and non-LWT on Windows/Linux but this is not the case on OS X where
> the gradient is opaque without a LWT.

Why does the gradient need to be opaque without a LWT?

Comment 18

[Matthew N. [:MattN]]

(In reply to Dão Gottwald [:dao] from comment #17)
> (In reply to Matthew N. [:MattN] from comment #15)
> > I added the new define because we could assume the same gradient worked for
> > both LWT and non-LWT on Windows/Linux but this is not the case on OS X where
> > the gradient is opaque without a LWT.
> 
> Why does the gradient need to be opaque without a LWT?

It doesn't need to be opaque but unlike Windows/Linux, the gradient doesn't need to overlay -moz-dialog. That's also how the gradient was specified in the spec and it seemed easier to maintain as one gradient instead of two layers.

It may be possible to come up with a gradient and base color such that the gradient works for the LWT and non-LWT case but the look may not be desired.

Comment 24

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/39b1b4eee94b