1065429 - [10.10] Update selected tab border to match the toolbar top border / titlebar bottom border

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[:Gijs (he/him)]

Attachment: Screenshot

The titlebar was updated for Yosemite, but that meant its border changed, and now the selected tab border no longer matches the border of the toolbar/titlebar - see attached screenshot.

This is a little more complicated still because IIRC we render our own border in lightweight themes which probably hasn't changed, nor has the border on 10.9 and earlier. :-(

Comment 1

[:Gijs (he/him)]

This could also be fixed by fixing bug 1016742, which is probably simpler.

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #1)
> This could also be fixed by fixing bug 1016742, which is probably simpler.

... but will have perf impact. :-(

Comment 3

[:Gijs (he/him)]

Attachment: fix tabbar border on yosemite,

I expect this should be enough, but I'm not 100% sure about perf, so I try-pushed it... baseline: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2552f16dfa2d ; with patch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1efe42fbf4ea

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> Created attachment 8495241 [details] [diff] [review]
> fix tabbar border on yosemite,
> 
> I expect this should be enough, but I'm not 100% sure about perf, so I
> try-pushed it... baseline:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2552f16dfa2d ;
> with patch:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1efe42fbf4ea

Looks like I really want a lighter color for the solid color, at least on Yosemite, which is odd because I sampled the color out of the current border on mavericks. :-\

Comment 5

[:Gijs (he/him)]

http://compare-talos.mattn.ca/?oldRevs=2552f16dfa2d&newRev=1efe42fbf4ea&server=graphs.mozilla.org&submit=true looks reasonable. tpaint and ts_paint are more or less the same. The only worrying thing here is a 6% tart regression on 10.6. With the 10.8 regression being only 0.4% (and within noise levels) and 10.6 now unsupported by Apple since the end of last year, I don't think this should stop us from the relevant code simplification here. Mike, would you agree?

Comment 6

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

Glad to hear that 10.8 behaves nicely.

Out of curiosity, how terrible do you think it'd be to special-case 10.6 to not draw our own border? We could probably pull it off with jar.mn overrides, but I'm wondering if it's even worth it.

I'm personally fine with the 6% TART regression if it's restricted to 10.6, but we should probably get avih and jmaher on board as well.

Comment 7

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #6)
> Glad to hear that 10.8 behaves nicely.
> 
> Out of curiosity, how terrible do you think it'd be to special-case 10.6 to
> not draw our own border? We could probably pull it off with jar.mn
> overrides, but I'm wondering if it's even worth it.

We could use a media query (@media(-moz-mac-lion-theme)), which would make me feel less awful about the complexity. Still not sure that's worth it though.

Comment 8

[:Gijs (he/him)]

Realistically, I guess I own this now, so I might as well actually take it... Marco, can you work your backlog magic? :-)

Comment 9

[Avi Halachmi (:avih)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #6)
> ...
> I'm personally fine with the 6% TART regression if it's restricted to 10.6,
> but we should probably get avih and jmaher on board as well.

That's fine.

we agreed to ignore 10.6-only regressions unless they're very big or result from GFX changes - at which case they may hint at a GFX issue which later versions can out-optimize. See bug 990490.

Comment 10

[Marco Mucci [:MarcoM]]

Added to IT 35.2

Comment 11

[:Gijs (he/him)]

This is harder than I thought because:

1) we can't stop the native border from drawing (unless we forgo native titlebar drawing entirely, which seems like a bad idea)
2) we can't use an opaque border color (which is what the patch I trypushed did) because we draw the top border of the nav-bar which overlaps the background tabs, and their separators have light highlights which should overlap/break up the border. This is annoying because...
3) the native border is dark (potentially too dark) on 10.9, and too light on 10.10, so using a partially transparent border to make 10.10 darker then makes things definitely too dark on 10.9.
4) I worry that the semi-transparent borders will have different perf consequences than the opaque one I tested in the try run.

This makes me think unifying the different borders (per bug 1016742) isn't really a good way forward to fix this particular bug (you also get fun times like the border is supposed to be different on inactive windows etc.), and we should just fix this on 10.10.

Comment 12

[Markus Stange [:mstange]]

What do you think of hiding the native border by making the native titlebar one pixel higher than necessary, and thus hiding the border under the navbar background? The native border would then be 1px lower than the CSS border (but nobody would see it anyway), and the CSS border would blend against the native titlebar gradient. The only disadvantage of this that I can think of is that the gradient would be slightly stretched, and the darkest visible gradient color would be slightly lighter than what we have at the moment, but I don't think anybody would notice.
I also don't see anything wrong with using OS X version media queries to specify different hardcoded border colors.

Comment 13

[:Gijs (he/him)]

(In reply to Markus Stange [:mstange] from comment #12)
> What do you think of hiding the native border by making the native titlebar
> one pixel higher than necessary, and thus hiding the border under the navbar
> background? The native border would then be 1px lower than the CSS border
> (but nobody would see it anyway), and the CSS border would blend against the
> native titlebar gradient.

In principle, I suspect this would work. My frustration with this bug is mostly that I was hoping we could simplify the code here and fix the bug, and this solution doesn't really achieve that. :-(

> The only disadvantage of this that I can think of
> is that the gradient would be slightly stretched, and the darkest visible
> gradient color would be slightly lighter than what we have at the moment,
> but I don't think anybody would notice.

Right.

> I also don't see anything wrong with using OS X version media queries to
> specify different hardcoded border colors.

Yeah. I suspect the simplest thing that could possibly work here is just a 10.10-only border "addition" (semi-transparent) in the CSS, and moving on.

Comment 14

[:Gijs (he/him)]

Attachment: fix tabbar border on yosemite,

Like this? Simplest thing that could possibly work? Not try-pushing because this is a no-op outside of yosemite...

Comment 15

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

Comment on attachment 8497436 [details] [diff] [review]
fix tabbar border on yosemite,

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

I'm afraid I don't have a Yosemite machine to test this on, so going by inspection only. The CSS looks OK, so I'll trust your testing. :)

Thanks Gijs!

Comment 16

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/dcf3bfa8deae

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dcf3bfa8deae

Comment 18

[:Gijs (he/him)]

Comment on attachment 8497436 [details] [diff] [review]
fix tabbar border on yosemite,

Approval Request Comment
[Feature/regressing bug #]: Yosemite
[User impact if declined]: sad titlebar/tabs in yosemite (rumored to be released October)
[Describe test coverage new/current, TBPL]: nope, styling only
[Risks and why]: very low, this change only affects yosemite
[String/UUID change made/needed]: none

Comment 19

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 35
Build Id:20141001030205
OS: Mac Os X 10.10

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8497436 [details] [diff] [review]
fix tabbar border on yosemite,

Aurora+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/045b5012a963

Comment 22

[Catalin Varga [QA][:VarCat]]

Verified as fixed:
FF34
Build Id: 20141002004001
OS: Mac Os X 10.10

Comment 23

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #9)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #6)
> > ...
> > I'm personally fine with the 6% TART regression if it's restricted to 10.6,
> > but we should probably get avih and jmaher on board as well.
> 
> That's fine.
> 
> we agreed to ignore 10.6-only regressions unless they're very big or result
> from GFX changes - at which case they may hint at a GFX issue which later
> versions can out-optimize. See bug 990490.

BTW, why would we have 10.6 regressions when the patch should only modify Yosemite?

Comment 24

[:Gijs (he/him)]

(In reply to Avi Halachmi (:avih) from comment #23)
> (In reply to Avi Halachmi (:avih) from comment #9)
> > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #6)
> > > ...
> > > I'm personally fine with the 6% TART regression if it's restricted to 10.6,
> > > but we should probably get avih and jmaher on board as well.
> > 
> > That's fine.
> > 
> > we agreed to ignore 10.6-only regressions unless they're very big or result
> > from GFX changes - at which case they may hint at a GFX issue which later
> > versions can out-optimize. See bug 990490.
> 
> BTW, why would we have 10.6 regressions when the patch should only modify
> Yosemite?

Because the previous version of this patch didn't "just" modify Yosemite. See comment 1 / bug 1016742.