Closed Bug 997227 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- wontfix
firefox30 --- verified
firefox31 --- verified

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [Australis:P3+])

Attachments

(1 file)

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.
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).
gavin / sledru - kinda feel like a heel for filing this after yesterday's landing of bug 996148. Sorry!
Screenshot comparisons look good on Mountain Lion. Switching to Snow Leopard now.
Comment on attachment 8407608 [details] [diff] [review]
Patch v1 (to be applied after backout of bug 996148)

Snow Leopard is all good.
Attachment #8407608 - Flags: review?(gijskruitbosch+bugs)
Attachment #8407608 - Flags: review?(MattN+bmo)
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+.
Attachment #8407608 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8407608 - Flags: review?(MattN+bmo) → review+
Gavin said we're cool to eat this bug for 29.
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.
Attachment #8407608 - Flags: approval-mozilla-aurora?
Thanks!

remote:   https://hg.mozilla.org/integration/fx-team/rev/5b8b5f3b200f
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5b8b5f3b200f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
Target Milestone: --- → Firefox 31
Attachment #8407608 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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).
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: petruta.rasa
You need to log in before you can comment on or make changes to this bug.