Over the course of tabs-in-titlebar on OS X, we've accumulated quite a few magic numbers in the code that handles how to draw the tabs in titlebar (or how to draw things when tabs _aren't_ in the titlebar). That technical debt is starting to become pretty expensive. For one thing, it's making fixing bug 973694 difficult, because it doesn't allow me to easily position the private browsing indicator at the top of the window in a nice way. I think we should try to simplify the OS X titlebar handling stuff a bit.

This will want a const introduced in bug 946987.

Extracted from the original patch I wrote in bug 973694.

(In reply to Mike Conley (:mconley) from comment #1) > This will want a const introduced in bug 946987. I was already planning on splitting that out today and now I had another reason to do so :) That define is now added in bug 990384 for your review.

Minor tweaks.

Comment on attachment 8401913 [details] [diff] [review] Patch v1.1 Here we go. Hopefully we can get this in before we do the mozscreenshot thing.

Comment on attachment 8401913 [details] [diff] [review] Patch v1.1 Review of attachment 8401913 [details] [diff] [review]: ----------------------------------------------------------------- Some prelinary notes from a screenshot diff on 10.9 in case you're still around. Some of these may be acceptable but I haven't investigated yet: * Note that the fullscreen button's vertical position shifted slightly when we are bring it down. I need to check if it is aligned with the other three * This causes a regression that the customization mode texture doesn't appear at the top of the window. (feedback- for this) * This patch changes the horizontal position of the the fullscreen button when tabs are not in the titlebar and a LWT is applied

(In reply to Matthew N. [:MattN] from comment #6) > * Note that the fullscreen button's vertical position shifted slightly when > we are bring it down. I need to check if it is aligned with the other three In what way? They look lined up to me: https://www.dropbox.com/s/onby2wt922vpftb/Screenshot%202014-04-07%2016.21.35.png > * This causes a regression that the customization mode texture doesn't > appear at the top of the window. (feedback- for this) Ok, I'll see what I can do about that. > * This patch changes the horizontal position of the the fullscreen button > when tabs are not in the titlebar and a LWT is applied Good catch.

(In reply to Mike Conley (:mconley) from comment #7) > (In reply to Matthew N. [:MattN] from comment #6) > > * Note that the fullscreen button's vertical position shifted slightly when > > we are bring it down. I need to check if it is aligned with the other three > > In what way? They look lined up to me: > https://www.dropbox.com/s/onby2wt922vpftb/Screenshot%202014-04-07%2016.21.35. > png See attached. The fullscreen button should move down 1px.

Comment on attachment 8401913 [details] [diff] [review] Patch v1.1 Review of attachment 8401913 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/osx/browser.css @@ +3143,5 @@ > } > > +/** Begin titlebar **/ > + > +#titlebar-buttonbox > .titlebar-button { Nit: Could you move this whole block to the top near line 45 (where some of this was before)? * It would simplify the diff as some code was just moved. This makes reviewing easier. * I personally prefer for CSS to have some parallel to the document structure so having the titlebar stuff at the top is nicer IMO. * It doesn't seem to be related to the nearby code (feel free to move the tabHeight define [and perhaps the others in tabs.inc.css] to browser.inc or just leave the one rule which depends on it below) @@ +3195,5 @@ > +} > + > +#main-window[tabsintitlebar][customize-entered] > #titlebar { > + -moz-appearance: none; > + background-image: linear-gradient(to bottom, rgb(233, 233, 233), rgb(178, 178, 178) 40px); Nit: removes the spaces between the RGB components. I think this is related to point 2 from my feedback. Perhaps this rule can be added to the "#main-window[customize-entered] > #tab-view-deck" rule to use the same properties?

Comment on attachment 8403667 [details] [diff] [review] Patch v1.2 (In reply to Matthew N. [:MattN] from comment #6) > * Note that the fullscreen button's vertical position shifted slightly when > we are bring it down. I need to check if it is aligned with the other three Fixed - a rule that was supposed to be in the private-browsing patch slipped into this one. > * This causes a regression that the customization mode texture doesn't > appear at the top of the window. (feedback- for this) Fixed - but I want to make sure that we're not somehow regressing CART here. I'll push to try in a second and do a comparison. > * This patch changes the horizontal position of the the fullscreen button > when tabs are not in the titlebar and a LWT is applied Actually, I think the new positioning is _more_ correct, since it leaves the fullscreen button in the position it would normally be if there was no LWT applied - so I think I've fixed a bonus bug here. (In reply to Matthew N. [:MattN] from comment #9) > Comment on attachment 8401913 [details] [diff] [review] > Patch v1.1 > > Review of attachment 8401913 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/themes/osx/browser.css > @@ +3143,5 @@ > > } > > > > +/** Begin titlebar **/ > > + > > +#titlebar-buttonbox > .titlebar-button { > > Nit: Could you move this whole block to the top near line 45 (where some of > this was before)? > * It would simplify the diff as some code was just moved. This makes > reviewing easier. > * I personally prefer for CSS to have some parallel to the document > structure so having the titlebar stuff at the top is nicer IMO. > * It doesn't seem to be related to the nearby code (feel free to move the > tabHeight define [and perhaps the others in tabs.inc.css] to browser.inc or > just leave the one rule which depends on it below) I've moved the block up, and moved the tabHeight constant to browser.inc. > > @@ +3195,5 @@ > > +} > > + > > +#main-window[tabsintitlebar][customize-entered] > #titlebar { > > + -moz-appearance: none; > > + background-image: linear-gradient(to bottom, rgb(233, 233, 233), rgb(178, 178, 178) 40px); > > Nit: removes the spaces between the RGB components. > Not necessary anymore since I've removed that rule. > I think this is related to point 2 from my feedback. Perhaps this rule can > be added to the "#main-window[customize-entered] > #tab-view-deck" rule to > use the same properties? Yep - that works for me. Like I said though, going to ensure this doesn't somehow regress CART. Try builds coming next.

Baseline: https://tbpl.mozilla.org/?tree=Try&rev=b87f455ae010 Patch: https://tbpl.mozilla.org/?tree=Try&rev=e2ead5bd718b Compare-talos: http://compare-talos.mattn.ca/?oldRevs=b87f455ae010&newRev=e2ead5bd718b&server=graphs.mozilla.org&submit=true

(In reply to Mike Conley (:mconley) from comment #11) > > * It doesn't seem to be related to the nearby code (feel free to move the > > tabHeight define [and perhaps the others in tabs.inc.css] to browser.inc or > > just leave the one rule which depends on it below) > > I've moved the block up, and moved the tabHeight constant to browser.inc. It shouldn't be in browser.inc, at least not without a more careful name. browser.css using it like you are is essentially a bug. In reality, that number is something like the tabs' min-height and using it as if it were the tab bar's static height is bogus.

My understanding is that we decided months ago that we don't support anything other than a height of @tabHeight@ on OS X which is why I was fine with it (I thought of this issue too).

browser.inc is cross-platform. I can already see the patches making use of tabHeight elsewhere. As for OS X, my understanding is that the current situation is a compromise but nonetheless a bug that we'd fix if feasible.

Alright, so how do you recommend that I proceed? Should I drop using @tabHeight@ entirely?

Or do you recommend we rename tabHeight to something else?

(In reply to Mike Conley (:mconley) from comment #12) > Baseline: https://tbpl.mozilla.org/?tree=Try&rev=b87f455ae010 > Patch: https://tbpl.mozilla.org/?tree=Try&rev=e2ead5bd718b > > Compare-talos: > http://compare-talos.mattn.ca/ > ?oldRevs=b87f455ae010&newRev=e2ead5bd718b&server=graphs.mozilla. > org&submit=true Good news - no CART regressions here.

Comment on attachment 8403667 [details] [diff] [review] Patch v1.2 Review of attachment 8403667 [details] [diff] [review]: ----------------------------------------------------------------- There were no changes in the 88 screenshots for the following sets: Tabs WindowSize Toolbars TabsInTitlebar LightweightThemes CustomizeMode (In reply to Dão Gottwald [:dao] from comment #13) > (In reply to Mike Conley (:mconley) from comment #11) > > I've moved the block up, and moved the tabHeight constant to browser.inc. > > It shouldn't be in browser.inc, at least not without a more careful name. > browser.css using it like you are is essentially a bug. In reality, that > number is something like the tabs' min-height and using it as if it were the > tab bar's static height is bogus. I would also use this in https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/tab-selected.svg?raw=1 for the two @height attributes and in that case "tabHeight" is fairly accurate. (In reply to Dão Gottwald [:dao] from comment #15) > browser.inc is cross-platform. I can already see the patches making use of > tabHeight elsewhere. As for OS X, my understanding is that the current > situation is a compromise but nonetheless a bug that we'd fix if feasible. Perhaps, but I don't think it's a priority, this bug isn't introducing the problem, and I don't see us fixing it in the next week for 29 so I don't think we need to fix it now. I'm okay with renaming the define to tabMinHeight in browser.inc (of course then I wouldn't use it in tab-selected.svg). (In reply to Mike Conley (:mconley) from comment #16) > Alright, so how do you recommend that I proceed? I think we can proceed with using the define since the only alternative I know of is to hard-code the same value which makes it less-obvious where the number came from. ::: browser/themes/osx/browser.css @@ +15,1 @@ > %define toolbarButtonPressed :hover:active:not([disabled="true"]):not([cui-areatype="menu-panel"]) Nit: I would prefer these remain alphabetical (ignoring forwardTransitionLength). Feel free to ignore me if you disagree. @@ +96,5 @@ > + margin-top: @windowButtonMarginTop@; > +} > + > +#main-window[tabsintitlebar][customize-entered] > #titlebar { > + -moz-appearance: none; I thought that this was something that should be avoided because the default -moz-appearance tells OS X where to anchor sheets IIRC. I guess we already do this for LWT though and in my testing with "window.alert('foo')" from the browser console we sheet just appears at the very top edge of the window instead which is not ideal. Since we're already doing this for LWT and we haven't seen reports of other issues, I think it's fine to do in customize mode for now unless you have a way to avoid this. Ideally we'd fix LWT too but we can do both of these in a follow-up.

(In reply to Mike Conley (:mconley) from comment #16) > Alright, so how do you recommend that I proceed? Should I drop using > @tabHeight@ entirely? Rename it to @tabMinHeight@ or move it back to tabs.inc.css or both. Either way the outcome should be that it's not so tempting to use it all over the place in browser.css.

Alright, so here's how I'm going to proceed: 1) I'm going to address the two issues that MattN brought up, and then land this. 2) I'm going to file a follow-up bug to rename the tabHeight define to tabMinHeight.

(In reply to Mike Conley (:mconley) from comment #21) > 2) I'm going to file a follow-up bug to rename the tabHeight define to > tabMinHeight. Your patch moves tabHeight to browser.inc and it shouldn't -- please rename it here or just leave it in tabs.inc.css.

(In reply to Matthew N. [:MattN] from comment #19) > Comment on attachment 8403667 [details] [diff] [review] > Patch v1.2 > > Review of attachment 8403667 [details] [diff] [review]: > ----------------------------------------------------------------- > > There were no changes in the 88 screenshots for the following sets: Tabs > WindowSize Toolbars TabsInTitlebar LightweightThemes CustomizeMode \o/ Thanks for the thorough check! > > ::: browser/themes/osx/browser.css > @@ +15,1 @@ > > %define toolbarButtonPressed :hover:active:not([disabled="true"]):not([cui-areatype="menu-panel"]) > > Nit: I would prefer these remain alphabetical (ignoring > forwardTransitionLength). Feel free to ignore me if you disagree. > Sounds good. Done. > @@ +96,5 @@ > > + margin-top: @windowButtonMarginTop@; > > +} > > + > > +#main-window[tabsintitlebar][customize-entered] > #titlebar { > > + -moz-appearance: none; > > I thought that this was something that should be avoided because the default > -moz-appearance tells OS X where to anchor sheets IIRC. I guess we already > do this for LWT though and in my testing with "window.alert('foo')" from the > browser console we sheet just appears at the very top edge of the window > instead which is not ideal. Since we're already doing this for LWT and we > haven't seen reports of other issues, I think it's fine to do in customize > mode for now unless you have a way to avoid this. Ideally we'd fix LWT too > but we can do both of these in a follow-up. Without -moz-appearance: none, the texture doesn't stretch up into the titlebar, and it looks wonky when the window is in the background. I'll file a follow-up bug to deal with it. Maybe we can tag a visibility: hidden pseudoelement onto the titlebar in the customizing / LWT case. Thanks for the (as usual) thorough review!

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

(In reply to Dão Gottwald [:dao] from comment #22) > (In reply to Mike Conley (:mconley) from comment #21) > > 2) I'm going to file a follow-up bug to rename the tabHeight define to > > tabMinHeight. > > Your patch moves tabHeight to browser.inc and it shouldn't -- please rename > it here or just leave it in tabs.inc.css. I'll file the follow-up bugs now, and have a patch up to rename that define today.

Bug 994758 and bug 994824 filed.

Comment on attachment 8404754 [details] [diff] [review] Patch v1.3 (r+'d by MattN) [Approval Request Comment] Bug caused by (feature/regressing bug #): Australis. User impact if declined: None directly - except that this patch is a prerequisite for bug 973694, which _does_ have user impact. I'll request approval on that patch once it's had a night to bake on central. Testing completed (on m-c, etc.): Lots of local testing, of course. A night or two on m-c, plus, MattN did a smash-up reviewing job using his screenshot comparison tool to make sure this didn't introduce any regressions (in fact, it fixes a few extra things instead!). Risk to taking this patch (and alternatives if risky): Given MattN's superior reviewing techniques, I'd say pretty darn low. String or IDL/UUID changes made by this patch: None.

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/d59b6d7df5ba Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/163d2250a03e