The private browsing indicator isn't displayed while on full screen

VERIFIED FIXED in Firefox 30

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: cbadau, Assigned: mconley)

Tracking

({regression})

Trunk
Firefox 32
x86_64
Windows 7
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox29 wontfix, firefox30+ verified, firefox31+ verified, firefox32+ verified)

Details

(Whiteboard: p=3 s=it-32c-31a-30b.2 [qa!])

Attachments

(3 attachments, 4 obsolete attachments)

Reproducible on the latest Nightly (BuildID: 20140509030227) 
Mozilla/5.0 (Windows NT 6.1; rv:32.0) Gecko/20100101 Firefox/32.0
Reproducible on the latest Aurora (BuildID: 20140509004003)
Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Firefox/31.0
Reproducible on Beta 30.0b3 (buildID: 20140508121358) 
Mozilla/5.0 (Windows NT 6.1; rv:30.0) Gecko/20100101 Firefox/30.0


Steps to reproduce: 
1. Launch Firefox. 
4. Open a new private window.
5. Go to full screen (press F11).
6. Observe the private browsing indicator. 

Actual results: The private browsing indicator isn't displayed while on full screen.

Expected results: The private browsing indicator is correctly displayed while on full screen. 

Notes: 1. This issue is reproducible also on Windows 8 32bit and on Windows 7 64bit.

Comment 1

5 years ago
Uh-oh, this is bad :(

Mike, any chance you could take a look please?
Component: Private Browsing → Theme
Flags: needinfo?(mconley)
Ugh, yes.
Flags: needinfo?(mconley) → firefox-backlog+
Posted patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Posted patch Patch v1.1 (obsolete) — Splinter Review
Attachment #8421232 - Attachment is obsolete: true
Comment on attachment 8421237 [details] [diff] [review]
Patch v1.1

A pretty embarrassing oversight on my part. :/
Attachment #8421237 - Flags: review?(jaws)
Tracking (regression + important feature + Ehsan sad)
Comment on attachment 8421237 [details] [diff] [review]
Patch v1.1

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

The icon being used for fullscreen mode is too tall, we should be using the smaller one. I'll upload a screenshot.
Attachment #8421237 - Flags: review?(jaws) → review-
Are you sure we should be using the small one in the fullscreen case? The smaller one is taller than the fullscreen window buttons, so that'll look awkward too.

I do notice from your screenshot, however, that there's a 1px break at the top when I use the large mask, which isn't great. That'll need to be fixed.

I think I want some shorlander input here, to make sure we know what mask we want to use.
Flags: needinfo?(shorlander)
I don't think the mask should be taking up 100% of the titlebar height, which the large mask does.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> I don't think the mask should be taking up 100% of the titlebar height,
> which the large mask does.

In this case, the titlebar is the tabstrip, but yeah, from the user's perspective, there's no difference.

Well, let me put together a patch that uses the smaller mask, and we can compare.
Estimating this new patch at p=3, since this adds yet another case that changes which mask we use.
Whiteboard: [p=3]
Hey Marco - especially considering this regression is on Beta, I think we should put this into the priority backlog.

Tentatively assigned to me because I guess I'm responsible for the regression?
Flags: needinfo?(mmucci)
Added to Iteration 32.2
Flags: needinfo?(mmucci)
Whiteboard: [p=3] → p=3 s=it-32c-31a-30b.2 [qa?]
Looking at this, I think I'm on jaws' side here. This looks better.
Assignee

Updated

5 years ago
Attachment #8421237 - Attachment is obsolete: true
Posted patch WIP Patch 2 (obsolete) — Splinter Review
Tested on Windows 7 (Aero Glass, Aero Basic and Classic). Still need to test this on Windows XP and 8.

Clearing needinfo on shorlander, since I think this is how we're gonna roll.
Flags: needinfo?(shorlander)
Posted patch Patch v2 (obsolete) — Splinter Review
Ok, tested on XP (Classic, Luna), Windows 7 (Aero Glass, Aero Basic, Classic), Windows 8.
Attachment #8423906 - Attachment is obsolete: true
Comment on attachment 8423927 [details] [diff] [review]
Patch v2

What do you think of this, jaws?
Attachment #8423927 - Flags: review?(jaws)
Comment on attachment 8423927 [details] [diff] [review]
Patch v2

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

::: browser/themes/windows/browser.css
@@ +2884,5 @@
>      height: 28px;
>    }
> +
> +  #main-window[inFullscreen] #TabsToolbar > .private-browsing-indicator {
> +    background-image: url("chrome://browser/skin/privatebrowsing-mask-titlebar-XPVista7.png");

Maybe add a comment here (and in the other places) stating that the use of the titlebar graphic in the TabsToolbar is intentional?
Attachment #8423927 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)
> Comment on attachment 8423927 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8423927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/windows/browser.css
> @@ +2884,5 @@
> >      height: 28px;
> >    }
> > +
> > +  #main-window[inFullscreen] #TabsToolbar > .private-browsing-indicator {
> > +    background-image: url("chrome://browser/skin/privatebrowsing-mask-titlebar-XPVista7.png");
> 
> Maybe add a comment here (and in the other places) stating that the use of
> the titlebar graphic in the TabsToolbar is intentional?

Thanks, good plan - will do.
Added some documentation.
Attachment #8423927 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8424043 - Attachment description: Patch v2.1 → Patch v2.1 (r+'d by jaws)
Attachment #8424043 - Flags: review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/6b3c88b4c146
Whiteboard: p=3 s=it-32c-31a-30b.2 [qa?] → p=3 s=it-32c-31a-30b.2 [qa?][fixed-in-fx-team]
Whiteboard: p=3 s=it-32c-31a-30b.2 [qa?][fixed-in-fx-team] → p=3 s=it-32c-31a-30b.2 [qa+][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6b3c88b4c146
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: p=3 s=it-32c-31a-30b.2 [qa+][fixed-in-fx-team] → p=3 s=it-32c-31a-30b.2 [qa+]
Target Milestone: --- → Firefox 32
QA Contact: camelia.badau
Verified fixed on Windows 7 32bit, 64 bit, Windows 8 32bit and Windows XP using latest Nightly 32.0a1 (buildID: 20140518030203). The private browsing indicator is displayed now while on full screen.
Status: RESOLVED → VERIFIED
QA Contact: camelia.badau
Whiteboard: p=3 s=it-32c-31a-30b.2 [qa+] → p=3 s=it-32c-31a-30b.2 [qa!]
QA Contact: camelia.badau
Let's get this uplifted asap - please nom.
Flags: needinfo?(mconley)
Comment on attachment 8424043 [details] [diff] [review]
Patch v2.1 (r+'d by jaws)

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

Bug 940509, missing the fullscreen case was an oversight on my part.


User impact if declined: 

Windows users who use private browsing windows will not have any UI indication that they're in a private browsing window if they're in fullscreen mode.


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

A few days of successful baking on m-c. Local testing.


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

Very low - this is CSS only, and the CSS only impacts the fullscreen mode.


String or IDL/UUID changes made by this patch:

None.
Attachment #8424043 - Flags: approval-mozilla-beta?
Attachment #8424043 - Flags: approval-mozilla-aurora?
Flags: needinfo?(mconley)
Attachment #8424043 - Flags: approval-mozilla-beta?
Attachment #8424043 - Flags: approval-mozilla-beta+
Attachment #8424043 - Flags: approval-mozilla-aurora?
Attachment #8424043 - Flags: approval-mozilla-aurora+
Setting back [qa+] so we can verify this on Aurora and Beta as well.
Whiteboard: p=3 s=it-32c-31a-30b.2 [qa!] → p=3 s=it-32c-31a-30b.2 [qa+]
Verified fixed on Windows 7 32bit, 64 bit, Windows 8 32bit and Windows XP using:
- latest Aurora 31.0a2 (buildID: 20140521004003)
- Fx 30 beta 6 (build ID: 20140520115057)
Whiteboard: p=3 s=it-32c-31a-30b.2 [qa+] → p=3 s=it-32c-31a-30b.2 [qa!]
You need to log in before you can comment on or make changes to this bug.