1404716 - Hide developer toolbar in video fullscreen

Status: VERIFIED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Nicolas Frandeboeuf]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170928180207

Steps to reproduce:

I display the developer toolbar, go to youtube and put a video in fullscreen.


Actual results:

The developer toolbar is not hidden. It was the case with firefox < 57


Expected results:

The developer toolbar is hidden.

That's probably a preference that should be available : to be able to hide the toolbar in video fullscreen mode.

Comment 1

[Nicolas Frandeboeuf]

By developer toolbar I mean the toolbar appearing when pressign SHIFT + F2.

Comment 2

[Henri Kemppainen]

For some background, prior to the landing of bug 1343947, there was no way to see or interact with GCLI in fullscreen mode.  Now it's a fullscreentoolbar, which is consistent with the rest of developer tools (which are also visible during fullscreen video).

However, unlike developer toolbox, the gcli cannot be moved to a separate window.  So it's always there or you need to toggle it off.

I see that having the option to hide GCLI with fullscreen video may be desirable.  Perhaps it could even hide automatically, as e.g. the history sidebar does (it's visible in fullscreen mode, but not with fullscreen video), even if that's not consistent with the developer toolbox's behavior?

Comment 3

[Henri Kemppainen]

Attachment: hide-domfullscreen-gcli.patch

So adding a config option seems overkill.  We can just make the toolbar collapse in DOM Fullscreen mode, which is exactly what the sidebar does.

In case someone wants to force it visible, it's possible e.g. with a userChrome.css oneliner:

#developer-toolbar { visibility: visible !important; }

Comment 4

[:Gijs (he/him)]

Comment on attachment 8915331 [details] [diff] [review]
hide-domfullscreen-gcli.patch

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

I'm not a good reviewer for this because it's basically a devtools patch (despite the filepath you're modifying :-) ). I believe devtools are trying to make their CSS and so on self-contained, so I suspect they may want this to live in one of their CSS files. Hopefully Patrick can advise (or suggest someone to advise).

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8915331 [details] [diff] [review]
hide-domfullscreen-gcli.patch

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

Looks good to me, but you'll need a browser peer to give the final R+ for this.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8915331 [details] [diff] [review]
hide-domfullscreen-gcli.patch

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

If Patrick is happy with this, so am I.

Comment 8

[Henri Kemppainen]

I can't edit keywords on this bug so someone else would have to flag it for checkin.  Thanks.

Comment 9

[Mike Taylor [:miketaylr]]

Adding checkin-needed per Comment #8

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0936cbb593b9
Hide GCLI in DOM fullscreen mode. r=Gijs, r=pbrosset

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/0936cbb593b9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Looks like a low-risk fix that would be beneficial for our DevEdition 57 users. Please request Beta approval on this patch when you get a chance.

Comment 13

[Henri Kemppainen]

Comment on attachment 8915331 [details] [diff] [review]
hide-domfullscreen-gcli.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1343947
[User impact if declined]: Users of gcli + fullscreen elements (e.g. video) find this regression very distracting.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: This one-liner simply adds a specific css selector to an existing rule.  Should affect gcli & fullscreen mode only.
[String changes made/needed]: None.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8915331 [details] [diff] [review]
hide-domfullscreen-gcli.patch

UI Polish, beta57+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0e67208dfe24

Comment 16

[Sajedul Islam]

I have reproduced this bug with Nightly 58.0a1 (2017-10-01) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Beta & Nightly !

Build   ID    20171009192146
User Agent    Mozilla/5.0 (Windows NT 10.0; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

Build   ID    20171012105833
User Agent    Mozilla/5.0 (Windows NT 10.0; WOW64; rv:58.0) Gecko/20100101 Firefox/58.0

Comment 17

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 58.0a1 (2017-10-01) (64-bit) on Ubuntu 16.04 LTS!

This bug's fix is Verified with latest Beta & latest Nightly!

Build   ID    20171009192146
User Agent    Mozilla/5.0 (X11; Linuxx86_64; rv:57.0) Gecko/20100101 Firefox/57.0

Build   ID    20171012105833
User Agent    Mozilla/5.0(X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0

[bugday-20171011]

Comment 18

[Sajedul Islam]

as par comment 16 & comment 17 , I am marking this bug as verified fixed.