997227 - Entering and exiting tabview makes items in tabs toolbar collide with fullscreen button

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Grrrrr - this is fallout from bug 996148. :(

STR:

1) In a new profile, use Cmd-Shift-E to enter tab view
2) Create a new tab group by dragging a tab into a new group
3) Use Cmd-Shift-E to exit tab view

AR:

The tabview button, which is automatically added to the tabs toolbar, is collapsed into the fullscreen button:

https://people.mozilla.org/~axel/tabview.png

ER:

There should be a 7px gap between the fullscreen button icon and any icons to its left.

Matt - it might be a good idea to add tabview toggling to the mozscreenshot tool addon, to get this sort of thing under coverage.

Comment 1

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

Attachment: Patch v1 (to be applied after backout of bug 996148)

So I think the best approach here is a backout of bug 996148, and a slightly different solution that operates on the same principles.

Basically, we have to ensure that we sample the width of the secondary button box _after_ updateTitlebarDisplay is called. The problem was that we were attempting to sample that width when the whole titlebar was hidden from tabview mode.

This patch needs to be applied on top of a backout of bug 996148.

Let me do the screenshot exercise again, and ensure that the STR from bug 996148 cannot be reproduced. I'll also do another try push to ensure no ts_paint or tpaint regressions (although I can't see how there'd be one).

Comment 2

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

gavin / sledru - kinda feel like a heel for filing this after yesterday's landing of bug 996148. Sorry!

Comment 3

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

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

Comment 4

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

Screenshot comparisons look good on Mountain Lion. Switching to Snow Leopard now.

Comment 5

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

Comment on attachment 8407608 [details] [diff] [review]
Patch v1 (to be applied after backout of bug 996148)

Snow Leopard is all good.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8407608 [details] [diff] [review]
Patch v1 (to be applied after backout of bug 996148)

I'm going to trust your judgment in terms of the screenshots looking good, and the code makes sense logic-wise, so here's an r+.

Comment 7

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

Gavin said we're cool to eat this bug for 29.

Comment 8

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

Comment on attachment 8407608 [details] [diff] [review]
Patch v1 (to be applied after backout of bug 996148)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Bug 996148


User impact if declined: 

Users who enter tabview mode on OS X may find the toolbar items in their tabstrip colliding with the fullscreen button (see screenshot in comment 0).


Testing completed (on m-c, etc.): 

Local testing, plus a battery of screenshot reference comparisons. Confirmed that this fixes this bug, and doesn't re-open bug 996148. Also confirmed that this doesn't regress our tpaint or ts_paint talos scores.


Risk to taking this patch (and alternatives if risky): 

Low risk. OS X only.


String or IDL/UUID changes made by this patch:

None.

Comment 9

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

Thanks!

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b8b5f3b200f

Comment 11

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

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/dca76b8434e5

Comment 14

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed on Firefox 30 beta 2 (20140505140302) and Aurora 31.0a2 (20140505004003) using Mac OSX 10.7.5 (Lyon) and 10.9.2 (Mavericks).