The tab and navBar in inactive windows seems too dark compared to other inactive windows.

I would suggest something closer to this.

Slightly darker border.

Agreed, we should be matching the platform as close as possible. Thanks for the catch, Stephen.

The active tab border is going to be a bit tricky. Does there appear to be a better solution than having UX create separate png files I'm not sure what the performance implications of swapping out background images are either. I'd like to work on this depending on the answer to that question. :)

If my last comment was confusing, that's because there was supposed to be a question mark at the end of "png files". > Does there appear to be a better solution than having the UX team create separate png files?

(In reply to Brandon Cheng from comment #4) > The active tab border is going to be a bit tricky. Does there appear to be a > better solution than having UX create separate png files? I imagine we'll need updated versions for the tab-* images in http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/tabbrowser/ , yes. > I'm not sure what > the performance implications of swapping out background images are either. That shouldn't be too bad, they'll be shipped in omni.jar and so disk access should be relatively fast. I think we pre-cache some of them elsewhere on the window, we could add the new files to that set. > I'd like to work on this depending on the answer to that question. :) Mm... I'm guessing we should get images first. Stephen?

(In reply to :Gijs Kruitbosch from comment #6) > Mm... I'm guessing we should get images first. Stephen? I can do that next week. Leaving needinfo.

This is particularly noticeable now the active window tab strip has vibrancy and is therefore much lighter in appearance than it was before. So the active tab strip can now be just as light as the inactive one, while looking less "solid". Maybe the *toolbar* of inactive windows should lighten too, to provide more visual differentiation?

Patch with updated assets and values for inactive windows. There might be a better way to do this and it should maybe use preprocess-tab-svgs.py to create the inactive tab images, but that was a little out of my depth.

Can you use overrides for the "normal" images (see bottom of that jar.mn file) to reduce the amount of extra OS-specific CSS necessary here? :-)

(In reply to :Gijs Kruitbosch from comment #10) > Can you use overrides for the "normal" images (see bottom of that jar.mn > file) to reduce the amount of extra OS-specific CSS necessary here? :-) Done. I think I did it right.

Comment on attachment 8666204 [details] [diff] [review] adjust-inactive-window-appearance.patch Review of attachment 8666204 [details] [diff] [review]: ----------------------------------------------------------------- Almost! ::: browser/themes/osx/browser.css @@ +64,5 @@ > } > > +@media (-moz-mac-yosemite-theme) { > + #navigator-toolbox::after { > + background-image: linear-gradient(to top, hsla(0,0%,0%,.1), hsla(0,0%,0%,.1) 1px, hsla(0,0%,100%,0) 1px, hsla(0,0%,100%,0) 2px, transparent 3px); Is this the toolbox's bottom border? Also for active windows? @@ +238,5 @@ > z-index: 1; > } > + > + #main-window[tabsintitlebar] #TabsToolbar:not([collapsed="true"]) + #nav-bar:-moz-window-inactive:not(:-moz-lwtheme) { > + border-top: 1px solid hsla(0,0%,0%,.15); nit: border-top-color: hsla(...) @@ +2575,5 @@ > + none; > + } > + > + .tab-background-start[visuallyselected=true]:-moz-window-inactive:-moz-locale-dir(ltr)::after, > + .tab-background-end[visuallyselected=true]:-moz-locale-dir(rtl)::after { Pretty sure the RTL styles also need to have -moz-window-inactive ? (here and below) ::: browser/themes/osx/tabbrowser/tab-selected-end-inactive.svg @@ +5,5 @@ > + <defs> > + <style> > + #tab-background-fill { > + background-color: transparent; > + background-image: linear-gradient(transparent, transparent 2px, hsl(0,0%,99%) 2px, hsl(0,0%,97%)); Please preprocess these files (* in the jar.mn) and include shared.inc so you can reuse the definition from there. @@ +12,5 @@ > + width: 100%; > + } > + </style> > + > + <svg:clipPath id="tab-curve-clip-path-end" clipPathUnits="objectBoundingBox"> The default namespace here is SVG, so you don't need the prefix here or below (and you can omit the xmlns:svg= attribute on the root)

Taking this based on discussion with Stephen.

Bug 1088763 - fix yosemite inactive window style, r?mconley

Comment on attachment 8680069 [details] MozReview Request: Bug 1088763 - fix yosemite inactive window style, r?mconley (In reply to :Gijs Kruitbosch from comment #13) > ::: browser/themes/osx/tabbrowser/tab-selected-end-inactive.svg > @@ +5,5 @@ > > + <defs> > > + <style> > > + #tab-background-fill { > > + background-color: transparent; > > + background-image: linear-gradient(transparent, transparent 2px, hsl(0,0%,99%) 2px, hsl(0,0%,97%)); > > Please preprocess these files (* in the jar.mn) and include shared.inc so > you can reuse the definition from there. I fixed everything else, I think, but this suggestion turns out to not be that simple because we need a different preprocessing character (%), which requires a bunch of python and stuff, which seemed too complicated to be worth it. Also, I ended up changing the navbar color to match better, which meant making it still lighter. Stephen, can you ui-review that change? There'll be a build from https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5c12c9a21e9 with this change.

Not sure when, or why, this change happened. But the window background blur is missing and the tabstrip border appears darker. So now the updated tab image is wrong in the opposite direction (i.e. the tab border is lighter than the toolbar border).

(In reply to Stephen Horlander [:shorlander] from comment #17) > Created attachment 8680398 [details] > Changed Tab Strip Background > > Not sure when, or why, this change happened. But the window background blur > is missing and the tabstrip border appears darker. So now the updated tab > image is wrong in the opposite direction (i.e. the tab border is lighter > than the toolbar border). I found the where and why, but it means this will need new tab images.

Comment on attachment 8680069 [details] MozReview Request: Bug 1088763 - fix yosemite inactive window style, r?mconley (In reply to Stephen Horlander [:shorlander] from comment #18) > (In reply to Stephen Horlander [:shorlander] from comment #17) > > Created attachment 8680398 [details] > > Changed Tab Strip Background > > > > Not sure when, or why, this change happened. But the window background blur > > is missing and the tabstrip border appears darker. So now the updated tab > > image is wrong in the opposite direction (i.e. the tab border is lighter > > than the toolbar border). > > I found the where and why, but it means this will need new tab images. OK. I can't create those images, so I'm going to flip the needinfo on its head so we get new images here... :-)

(In reply to :Gijs Kruitbosch from comment #19) > Comment on attachment 8680069 [details] > MozReview Request: Bug 1088763 - fix yosemite inactive window style, > r?mconley > > (In reply to Stephen Horlander [:shorlander] from comment #18) > > (In reply to Stephen Horlander [:shorlander] from comment #17) > > > Created attachment 8680398 [details] > > > Changed Tab Strip Background > > > > > > Not sure when, or why, this change happened. But the window background blur > > > is missing and the tabstrip border appears darker. So now the updated tab > > > image is wrong in the opposite direction (i.e. the tab border is lighter > > > than the toolbar border). > > > > I found the where and why, but it means this will need new tab images. > > OK. I can't create those images, so I'm going to flip the needinfo on its > head so we get new images here... :-) Looked at this a little more closely (http://cl.ly/image/3k2H1Z2L2k3w). Now that we have reverted the background vibrancy I don't see any mismatch between the existing tab image border and the toolbar border. So I think we can just keep the current tab-stroke-start, tab-stroke-end and tab-active-middle images. Will still need the new inactive images.

(In reply to Stephen Horlander [:shorlander] from comment #20) > Will still need the new inactive images. Alright, going to keep the needinfo going for those, then. :-)

(In reply to :Gijs Kruitbosch from comment #21) > (In reply to Stephen Horlander [:shorlander] from comment #20) > > Will still need the new inactive images. > > Alright, going to keep the needinfo going for those, then. :-) Oh, sorry, I meant that the inactive images already in the patch are good to go.

Comment on attachment 8680069 [details] MozReview Request: Bug 1088763 - fix yosemite inactive window style, r?mconley Bug 1088763 - fix yosemite inactive window style, r?mconley

Comment on attachment 8680069 [details] MozReview Request: Bug 1088763 - fix yosemite inactive window style, r?mconley Builds: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd49941e8f7b

Comment on attachment 8680069 [details] MozReview Request: Bug 1088763 - fix yosemite inactive window style, r?mconley https://reviewboard.mozilla.org/r/23555/#review21561 Looks good to me. Thanks for finishing this!

Comment on attachment 8680069 [details] MozReview Request: Bug 1088763 - fix yosemite inactive window style, r?mconley Oops. Used review instead of ui-r.

Comment on attachment 8680069 [details] MozReview Request: Bug 1088763 - fix yosemite inactive window style, r?mconley https://reviewboard.mozilla.org/r/23555/#review21665 Tentative r=me assuming the issues I've brought up aren't a problem. ::: browser/themes/osx/jar.mn:274 (Diff revision 2) > + skin/classic/browser/yosemite/tab-selected-end-inactive.svg (tabbrowser/tab-selected-end-inactive.svg) > + skin/classic/browser/yosemite/tab-selected-start-inactive.svg (tabbrowser/tab-selected-start-inactive.svg) I guess we don't care these files to have yosemite in the name? ::: browser/themes/osx/shared.inc:6 (Diff revision 2) > -%define fgTabTexture linear-gradient(transparent 2px, hsla(0,0%,100%,.6) 2px, hsla(0,0%,100%,.6) 3px, hsl(0,0%,99%) 3px, hsl(0,0%,93%)) > +%define fgTabTexture linear-gradient(transparent 2px, hsl(0,0%,99%) 2px, hsl(0,0%,93%)) This is going to affect non-Yosemite as well. Intentional?

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #27) > Comment on attachment 8680069 [details] > MozReview Request: Bug 1088763 - fix yosemite inactive window style, > r?mconley > > https://reviewboard.mozilla.org/r/23555/#review21665 > > Tentative r=me assuming the issues I've brought up aren't a problem. > > ::: browser/themes/osx/jar.mn:274 > (Diff revision 2) > > + skin/classic/browser/yosemite/tab-selected-end-inactive.svg (tabbrowser/tab-selected-end-inactive.svg) > > + skin/classic/browser/yosemite/tab-selected-start-inactive.svg (tabbrowser/tab-selected-start-inactive.svg) > > I guess we don't care these files to have yosemite in the name? We can add that, it's a good suggestion. > ::: browser/themes/osx/shared.inc:6 > (Diff revision 2) > > -%define fgTabTexture linear-gradient(transparent 2px, hsla(0,0%,100%,.6) 2px, hsla(0,0%,100%,.6) 3px, hsl(0,0%,99%) 3px, hsl(0,0%,93%)) > > +%define fgTabTexture linear-gradient(transparent 2px, hsl(0,0%,99%) 2px, hsl(0,0%,93%)) > > This is going to affect non-Yosemite as well. Intentional? hm, good point. I don't know. Per comment #20, I would expect we might be able to actually just keep this as-is? I'll leave it as-is, doublecheck it doesn't look horrible, and check it in, then if it's still not right Stephen can comment and we can revisit if necessary.

(In reply to :Gijs Kruitbosch from comment #16) > Comment on attachment 8680069 [details] > MozReview Request: Bug 1088763 - fix yosemite inactive window style, > r?mconley > > (In reply to :Gijs Kruitbosch from comment #13) > > ::: browser/themes/osx/tabbrowser/tab-selected-end-inactive.svg > > @@ +5,5 @@ > > > + <defs> > > > + <style> > > > + #tab-background-fill { > > > + background-color: transparent; > > > + background-image: linear-gradient(transparent, transparent 2px, hsl(0,0%,99%) 2px, hsl(0,0%,97%)); > > > > Please preprocess these files (* in the jar.mn) and include shared.inc so > > you can reuse the definition from there. > > I fixed everything else, I think, but this suggestion turns out to not be > that simple because we need a different preprocessing character (%), which > requires a bunch of python and stuff, which seemed too complicated to be > worth it. Can you explain why the approach from https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/tab-selected.svg?raw=1 wouldn't work? It's already using "%". The point of the preprocessing was so we don't duplicate the shape or styles but now we are…

See also https://mxr.mozilla.org/mozilla-central/source/browser/themes/preprocess-tab-svgs.py and https://mxr.mozilla.org/mozilla-central/source/browser/themes/tab-svgs.mozbuild

(In reply to Matthew N. [:MattN] from comment #32) > See also > https://mxr.mozilla.org/mozilla-central/source/browser/themes/preprocess-tab- > svgs.py and > https://mxr.mozilla.org/mozilla-central/source/browser/themes/tab-svgs. > mozbuild These seem more relevant than the link to tab-selected.svg... In any case, it seemed like a lot of hassle to set up all the requisite python, and the duplication is not that bad. I also didn't write the SVG files in question and it was not clear to me that they can be deduplicated. They are different from the tab-selected one, and it is equally not clear to me how all the build goop hangs together here. That and these files were already problematic for the build system (bug 1215526) and I wasn't about to make it worse. If you feel strongly then please file a followup bug.

Review commit: https://reviewboard.mozilla.org/r/28897/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/28897/

Comment on attachment 8701135 [details] MozReview Request: Backout 28e3554983f6 on aurora only for causing bug 1223550. a=Sylvestre Approval Request Comment [Feature/regressing bug #]: This patch causes bug 1223550 [User impact if declined]: Yosemite users will see a flash of unstyled tabs during startup, which is pretty ugly. [Describe test coverage new/current, TreeHerder]: This is a backout of a style change, so we're going back to code that we've shipped for several releases. [Risks and why]: Low risk backout, imo. [String/UUID change made/needed]: None.

Mike, next time, please open a new bug for the backout. It makes our life easier and the bug is easier to read.

https://hg.mozilla.org/releases/mozilla-aurora/rev/97e5908d03d6 Setting 45 to 'fixed', I guess.

This is still causing bug 1223550. I'm going to back this out and re-open the bug so that bug 1223550 doesn't slip into Aurora.

https://hg.mozilla.org/integration/fx-team/rev/fd55305662594a886ca5bf3ad77f3afbe4d53034 Backout 28e3554983f6 (bug 1088763) for causing bug 1223550

Backed out and re-opening.

As I noted on bug 1223550, I do not have the expertise to fix that bug, and so there is no point keeping me assigned here as I can't drive this.

I originally missed the 46 merge cutoff, so I had to back this out of aurora too: remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/af58efa23494

Backout made it to m-c: https://hg.mozilla.org/mozilla-central/rev/fd5530566259

Comment on attachment 8680069 [details] MozReview Request: Bug 1088763 - fix yosemite inactive window style, r?mconley Kicking this out of my queue for checkin.

(In reply to :Gijs Kruitbosch (back May 3rd) from comment #41) > As I noted on bug 1223550, I do not have the expertise to fix that bug, and > so there is no point keeping me assigned here as I can't drive this. mstange, might you have any ideas on what to do here?

(In reply to aleth [:aleth] from comment #45) > (In reply to :Gijs Kruitbosch (back May 3rd) from comment #41) > > As I noted on bug 1223550, I do not have the expertise to fix that bug, and > > so there is no point keeping me assigned here as I can't drive this. > > mstange, might you have any ideas on what to do here? (Note bug 1223550 comment 32)

mstange figured out the issue for the unstyled flash in bug 1223550 comment 35. Anybody feel like driving this patch through to completion? aleth?

I can do it, I have the patches.

Review commit: https://reviewboard.mozilla.org/r/56984/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56984/

We need to preload both the inactive and the active images. All our image loads need to be kicked off before the window document's load event. While the window is hidden, it is inactive. So we need the preloading hack to kick off the loads for the active images. Review commit: https://reviewboard.mozilla.org/r/56986/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56986/

Comment on attachment 8758878 [details] MozReview Request: Bug 1088763 - Preload yosemite tab images. r?mconley https://reviewboard.mozilla.org/r/56986/#review53746 If this does it, LGTM. Thanks! ::: browser/themes/osx/browser.css:2506 (Diff revision 1) > +@media (-moz-mac-yosemite-theme) { > +} Why the dead styling block?

Comment on attachment 8758878 [details] MozReview Request: Bug 1088763 - Preload yosemite tab images. r?mconley Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56986/diff/1-2/

Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/33544460e0b9 Fix yosemite inactive window style, r=mconley, ui-r=shorlander https://hg.mozilla.org/integration/mozilla-inbound/rev/81bfec47b65d Preload yosemite tab images. r=mconley

https://hg.mozilla.org/mozilla-central/rev/33544460e0b9 https://hg.mozilla.org/mozilla-central/rev/81bfec47b65d

(In reply to :Gijs Kruitbosch (no reviews until June 6) from comment #13) > > +@media (-moz-mac-yosemite-theme) { > > + #navigator-toolbox::after { > > + background-image: linear-gradient(to top, hsla(0,0%,0%,.1), hsla(0,0%,0%,.1) 1px, hsla(0,0%,100%,0) 1px, hsla(0,0%,100%,0) 2px, transparent 3px); > > Is this the toolbox's bottom border? Also for active windows? Yes, this also affected active windows. Was that intentional after all? It wasn't clear to me from reading the bug. Before: https://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-central/sha512/a7ae72f474fa84a7c5a7a8ec613244e2d57f4aeec18829dee61aacbd3a2999632265d90ece23f51ab7fcce8a80f3981fc6a586e490ada7363b3166d52462e1e1 After: https://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-central/sha512/56050d46b97f39616ca987b8f217bfb537d4353a9c3bb4bf54171ff2b8836577b0b478245df55b3e5868e65ccdbe287a246a30c753e4f4649a24882a7fa01130 Comparison (ignore the selected tab noise): https://screenshots.mattn.ca/comparisons/mozilla-central/111970c738234569c8c180319155327316335deb/mozilla-central/34a8be4346a9231e472fc36b1d7c0531e0fbf7c5/osx-10-10/primaryUI_001_tabsInTitlebar_fiveTabs_maximized_onlyNavBar_noLWT.png Full results: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=111970c738234569c8c180319155327316335deb&newProject=mozilla-central&newRev=34a8be4346a9231e472fc36b1d7c0531e0fbf7c5&filter=%5Eosx-10-10_primaryUI

Yeah changing the active windows bottom border was not intended.

Argh, sorry I missed that. :(

I'm fixing that in bug 1277638.

Should this be uplifted before the merge?

Comment on attachment 8758877 [details] MozReview Request: Bug 1088763 - Fix yosemite inactive window style, r=mconley, ui-r=shorlander Good idea. Approval Request Comment [Feature/regressing bug #]: Mac OS X Yosemite (10.10) styling [User impact if declined]: Inactive windows look half-active on 10.10 and 10.11 (inactive tab bar background, active toolbar styles) [Describe test coverage new/current, TreeHerder]: none (no tests for visuals), unless you want to count MattN's manually run comparison viewer [Risks and why]: very low, just tweaking CSS and adding a few images [String/UUID change made/needed]: none

Bug 1277638 should be uplifted at the same time as it's a followup to fix a regression this introduced.

Is this and the patch in bug 1277638 aimed at 48? If so I can change the approval request to beta. Looks like this is already fixed in aurora (the uplift didn't happen before the merge)

Not sure - Gijs, can you double-check which changesets made it into what branches? I'm on PTO this week.

Approval Request Comment [Feature/regressing bug #]: n/a [User impact if declined]: yosemite styling looks suboptimal [Describe test coverage new/current, TreeHerder]: nope, styling only [Risks and why]: low-risk, minor styling/imaging changes only, has baked for a considerable time on 49 already [String/UUID change made/needed]: no. (NB: this patch contains all 2 changesets from this bug + 1 changeset from bug 1277638 - you might still need to add approval strings (a=foo) to the commit messages of the individual changesets )

Comment on attachment 8763866 [details] [diff] [review] Patch for beta Improve the l&f, taking it. Should be in 48 beta 3

patch failed to apply, can you take a look ?

(In reply to Carsten Book [:Tomcat] from comment #67) > patch failed to apply, can you take a look ? That's odd, I exported it from beta, and it worked for me when I just reimported it... anyway: remote: https://hg.mozilla.org/releases/mozilla-beta/rev/a209701fec49 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/fd92b3537dd6 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/6a6dc3f2075a

Reproduced the issue on FX 48b1 OS X 10.11. Verified fixed FX 48b5, 49.0a2 (2016-07-01).