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

VERIFIED FIXED in Firefox 34

Status

()

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 35
x86
Mac OS X
Points:
3
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox32 unaffected, firefox33 unaffected, firefox34 verified, firefox35 verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 8487242 [details]
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. :-(
(Assignee)

Comment 1

4 years ago
This could also be fixed by fixing bug 1016742, which is probably simpler.
(Assignee)

Comment 2

4 years ago
(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. :-(
(Assignee)

Updated

4 years ago
status-firefox32: --- → unaffected
status-firefox33: --- → unaffected
status-firefox34: --- → ?
status-firefox35: --- → affected

Updated

4 years ago
Flags: qe-verify+
Flags: firefox-backlog+
(Assignee)

Comment 3

4 years ago
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
(Assignee)

Comment 4

4 years ago
(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. :-\
(Assignee)

Comment 5

4 years ago
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?
Flags: needinfo?(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.
Flags: needinfo?(mconley)
Flags: needinfo?(jmaher)
Flags: needinfo?(avihpit)
(Assignee)

Comment 7

4 years ago
(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.
(Assignee)

Comment 8

4 years ago
Realistically, I guess I own this now, so I might as well actually take it... Marco, can you work your backlog magic? :-)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 35.2
Flags: needinfo?(mmucci)

Comment 9

4 years ago
(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.
Flags: needinfo?(avihpit)

Updated

4 years ago
Flags: needinfo?(jmaher)
Added to IT 35.2
Flags: needinfo?(mmucci)
(Assignee)

Comment 11

4 years ago
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.
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.
(Assignee)

Comment 13

4 years ago
(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.
(Assignee)

Comment 14

4 years ago
Created attachment 8497436 [details] [diff] [review]
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...
Attachment #8497436 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Attachment #8495241 - Attachment is obsolete: true
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!
Attachment #8497436 - Flags: review?(mconley) → review+

Updated

4 years ago
Iteration: 35.2 → 35.3
https://hg.mozilla.org/mozilla-central/rev/dcf3bfa8deae
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
QA Contact: catalin.varga
(Assignee)

Comment 18

4 years ago
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
Attachment #8497436 - Flags: approval-mozilla-aurora?
Verified as fixed using the following environment:

FF 35
Build Id:20141001030205
OS: Mac Os X 10.10
Status: RESOLVED → VERIFIED
status-firefox35: affected → verified
status-firefox34: ? → affected
Comment on attachment 8497436 [details] [diff] [review]
fix tabbar border on yosemite,

Aurora+
Attachment #8497436 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed:
FF34
Build Id: 20141002004001
OS: Mac Os X 10.10
status-firefox34: fixed → verified
(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?
(Assignee)

Comment 24

4 years ago
(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.
You need to log in before you can comment on or make changes to this bug.