Last Comment Bug 996148 - The PB indicator in full screen mode seems to be cut off
: The PB indicator in full screen mode seems to be cut off
Status: VERIFIED FIXED
[Australis:P3+]
: regression
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: unspecified
: All Mac OS X
-- normal (vote)
: Firefox 31
Assigned To: Mike Conley (:mconley)
:
: Dão Gottwald [:dao]
Mentors:
Depends on: 997227
Blocks: 973694
  Show dependency treegraph
 
Reported: 2014-04-14 12:02 PDT by :Ehsan Akhgari
Modified: 2014-04-18 08:09 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
disabled
+
disabled


Attachments
Screenshot (16.52 KB, image/png)
2014-04-14 12:02 PDT, :Ehsan Akhgari
no flags Details
Patch v1 (3.48 KB, patch)
2014-04-14 18:20 PDT, Mike Conley (:mconley)
MattN+bmo: review+
gavin.sharp: approval‑mozilla‑aurora+
gavin.sharp: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image :Ehsan Akhgari 2014-04-14 12:02:28 PDT
Created attachment 8406315 [details]
Screenshot

I can reproduce this when I put a private window in full screen mode.  mconley could not repro for some reason.
Comment 1 User image :Ehsan Akhgari 2014-04-14 12:04:04 PDT
I have the titlebar turned on btw.
Comment 2 User image Mike Conley (:mconley) 2014-04-14 12:05:25 PDT
Ehsan is also using 10.9.2 (Mavericks). I cannot repro on 10.8.5 (Mountain Lion).
Comment 3 User image Mike Conley (:mconley) 2014-04-14 12:46:56 PDT
Youch - what I _can_ reproduce is a total lack of a private browsing indicator in fullscreen in safe mode. :/

I _think_ this has something to do with the persistence of the widths for the titlebar-placeholder-on-TabsToolbar-for-fullscreen-button element. Ehsan had a width of 16 persisted in his localstore.rdf, and this is why he got the mask partially cut off.

I wonder if perhaps we should just remove the width persistence from the titlebar-placeholder-on-TabsToolbar-for-fullscreen-button element - that could sidestep this issue, and I don't believe will impact our ts_paint or tpaint times (but we'll of course need to check that).
Comment 4 User image Mike Conley (:mconley) 2014-04-14 17:50:52 PDT
Ok, here are some STR:

1) Create a brand new profile
2) Enter customize mode, and enable the titlebar
3) Restart Firefox
4) Open a private browsing window
5) Enter fullscreen mode

AR: https://bugzilla.mozilla.org/attachment.cgi?id=8406315

ER: The mask should be the same distance from the right edge of the window as it is in a normal window.
Comment 5 User image Mike Conley (:mconley) 2014-04-14 17:59:51 PDT
Bonus STR for bonus bug:

Repeat steps 1-2, but instead of just restarting Firefox, enter safe mode.

Then do steps 4 and 5. No mask at all. :(
Comment 6 User image Mike Conley (:mconley) 2014-04-14 18:20:12 PDT
Created attachment 8406542 [details] [diff] [review]
Patch v1

Ok - I'm proposing we set the width on the fullscreen placeholder whether or not tabs in titlebar is enabled. I've tested / confirmed locally that this fixes the problem.

I'm going to push a patch to try to ensure this doesn't introduce a ts_paint or tpaint regression.
Comment 7 User image Mike Conley (:mconley) 2014-04-14 18:20:52 PDT
Marking P3+ because having a missing private browsing indicator is not a good thing at all.
Comment 9 User image Mike Conley (:mconley) 2014-04-14 18:25:36 PDT
Comment on attachment 8406542 [details] [diff] [review]
Patch v1

Hey Matt - it's just a small patch. Can you sanity-check me?
Comment 10 User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-04-15 00:55:43 PDT
Comment on attachment 8406542 [details] [diff] [review]
Patch v1

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

::: browser/base/content/browser.js
@@ +4471,5 @@
>      if (allowed) {
>        // We set the tabsintitlebar attribute first so that our CSS for
>        // tabsintitlebar manifests before we do our measurements.
>        document.documentElement.setAttribute("tabsintitlebar", "true");
>        updateTitlebarDisplay();

Moving the width measurement up will break if any of the the selectors which change the dimensions include [tabsintitlebar] or [chromemargin*] because that change will happen after the measurement. Have you audited all of the existing selectors for #titlebar-secondary-buttonbox and its children to ensure they won't change the width?

If you're confident about this and don't want me to run a screenshot diff tomorrow then rs=me. I think this should bake a few days though and get QA testing.
Comment 11 User image Mike Conley (:mconley) 2014-04-15 07:05:59 PDT
I'll audit the CSS right now for both changes to titlebar-secondary-buttonbox, and its children.

I'll also grab a copy of your screenshot tool and try to do some before / after comparisons. I have Mountain Lion and Snow Leopard with me.
Comment 12 User image Mike Conley (:mconley) 2014-04-15 09:51:01 PDT
I did the CSS audit and confirmed that the widths of the secondary button box and its children are not at risk of changing due to attributes being added within TabsInTitlebar's _update function, so that's good.

It took me a while because Lion's screencapture tool doesn't let me choose a particular window to capture, and so I had to do some hackery to get the screenshot comparisons to work.

Anyhow, I did it, and it doesn't look like there are any differences between a build with this patch, and the reference screenshots from Nightly. So that's also good.

Local testing has also shown that this fixes the bugs exposed by the STR in comment 4 and comment 5.

The 10.6 try push shows no tpaint or ts_paint regressions, so that's good. The 10.8 retriggers are still coming in. I've upped their priority, but they might still be a few hours off.

I'm going to wait for those to come in before landing on fx-team and requesting uplift approvals.
Comment 13 User image Mike Conley (:mconley) 2014-04-15 11:12:22 PDT
10.8 retriggers came in, and compare talos isn't showing any regression on ts_paint or tpaint. Let's roll.

remote:   https://hg.mozilla.org/integration/fx-team/rev/47845138fae1
Comment 14 User image Mike Conley (:mconley) 2014-04-15 14:46:20 PDT
Comment on attachment 8406542 [details] [diff] [review]
Patch v1

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

Bug 973694. :(


User impact if declined: 

OS X users that have configured Firefox to run without tabs in the titlebar run the risk of having _no_ private browsing indicator if they:

1) Go into fullscreen mode in a private browsing window
2) Open a private browsing window in safe mode.


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

Local testing to confirm bug fix, plus a screenshot comparison on both Mountain Lion and Snow Leopard against reference screenshots. Talos testing showed no ts_paint or tpaint regressions.


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

Low. The other alternative is backing out bug 973694, but that would be really sad (and result in super ugly private browsing mode for users without tabs in titlebar).


String or IDL/UUID changes made by this patch:

None.
Comment 15 User image Ryan VanderMeulen [:RyanVM] 2014-04-15 19:20:30 PDT
https://hg.mozilla.org/mozilla-central/rev/47845138fae1
Comment 16 User image Mike Conley (:mconley) 2014-04-15 20:30:08 PDT
Thanks sledru / gavin for letting this one through!


Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/59e0310606e0


Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/5a2fd00c18e0
Comment 17 User image Mike Conley (:mconley) 2014-04-16 10:49:27 PDT
Just FYI, if I have my way, this will get backed out of all channels, and the more robust solution in bug 997227 will be used instead.
Comment 18 User image Mike Conley (:mconley) 2014-04-16 16:49:10 PDT
Backed out of fx-team as: https://hg.mozilla.org/integration/fx-team/rev/c94117f49274
Comment 19 User image Cornel Ionce [QA] (:cornel_ionce) 2014-04-18 05:45:58 PDT
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:30.0) Gecko/20100101 Firefox/30.0

Verified fixed with Fx 29 beta 9 (build ID: 20140417185217) and Latest Aurora (build ID: 20140418004001) on Mac OS X 10.8 and 10.9 using both scenarios:
1. Fullscreen mode in a private browsing window.
2. Open a private browsing window in safe mode.
Comment 20 User image Mike Conley (:mconley) 2014-04-18 08:09:13 PDT
Backed out of mozilla-central: https://hg.mozilla.org/mozilla-central/rev/c94117f49274

Backed out of mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/68daba799a49

Note You need to log in before you can comment on or make changes to this bug.