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

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Camelia Badau [:cbadau], Desktop QA]

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

[(no longer active)]

Uh-oh, this is bad :(

Mike, any chance you could take a look please?

Comment 2

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

Ugh, yes.

Comment 3

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

Attachment: Patch v1

Comment 4

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

Attachment: Patch v1.1

Comment 5

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

Comment on attachment 8421237 [details] [diff] [review]
Patch v1.1

A pretty embarrassing oversight on my part. :/

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Tracking (regression + important feature + Ehsan sad)

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of pb indicators

Comment 9

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

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.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

I don't think the mask should be taking up 100% of the titlebar height, which the large mask does.

Comment 11

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

(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.

Comment 12

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

Estimating this new patch at p=3, since this adds yet another case that changes which mask we use.

Comment 13

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

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?

Comment 14

[Marco Mucci [:MarcoM]]

Added to Iteration 32.2

Comment 15

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

Attachment: Using short mask in fullscreen mode

Looking at this, I think I'm on jaws' side here. This looks better.

Comment 16

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

Attachment: WIP Patch 2

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.

Comment 17

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

Attachment: Patch v2

Ok, tested on XP (Classic, Luna), Windows 7 (Aero Glass, Aero Basic, Classic), Windows 8.

Comment 18

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

Comment on attachment 8423927 [details] [diff] [review]
Patch v2

What do you think of this, jaws?

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 20

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

(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.

Comment 21

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

Attachment: Patch v2.1 (r+'d by jaws)

Added some documentation.

Comment 22

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/6b3c88b4c146

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6b3c88b4c146

Comment 24

[Camelia Badau [:cbadau], Desktop QA]

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.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Let's get this uplifted asap - please nom.

Comment 26

[Mike Conley (: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.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a336b9b26d09
https://hg.mozilla.org/releases/mozilla-beta/rev/e17059d22a5b

Comment 28

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting back [qa+] so we can verify this on Aurora and Beta as well.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/e17059d22a5b

Comment 30

[Camelia Badau [:cbadau], Desktop 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)