Closed Bug 996148 Opened 10 years ago Closed 10 years ago

The PB indicator in full screen mode seems to be cut off

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 + verified
firefox30 + disabled
firefox31 + disabled

People

(Reporter: ehsan.akhgari, Assigned: mconley)

References

Details

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

Attachments

(2 files)

Attached image Screenshot
I can reproduce this when I put a private window in full screen mode.  mconley could not repro for some reason.
I have the titlebar turned on btw.
Ehsan is also using 10.9.2 (Mavericks). I cannot repro on 10.8.5 (Mountain Lion).
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).
Keywords: regression
Hardware: x86 → All
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.
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. :(
Attached patch Patch v1Splinter Review
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.
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Marking P3+ because having a missing private browsing indicator is not a good thing at all.
Whiteboard: [Australis:P3+]
Comment on attachment 8406542 [details] [diff] [review]
Patch v1

Hey Matt - it's just a small patch. Can you sanity-check me?
Attachment #8406542 - Flags: review?(MattN+bmo)
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.
Attachment #8406542 - Flags: review?(MattN+bmo) → review+
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.
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.
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
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]
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.
Attachment #8406542 - Flags: approval-mozilla-beta?
Attachment #8406542 - Flags: approval-mozilla-aurora?
Attachment #8406542 - Flags: approval-mozilla-beta?
Attachment #8406542 - Flags: approval-mozilla-beta+
Attachment #8406542 - Flags: approval-mozilla-aurora?
Attachment #8406542 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/47845138fae1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
Target Milestone: --- → Firefox 31
Depends on: 997227
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.
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.