1071821 - fullscr-toggler element needs to be hidden in DOM fullscreen

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

When we have DOM fullscreen active, the fullscr-toggler element needs to be display:none (or similar), to avoid creating a 1px-tall bar at the top of the screen.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: set display:none when in DOM fullscreen

This seems to work, and simplifies the code some.  The event listeners can be added once, and they'll only do anything if the element is actually present.  The current code was also adding the event listener multiple times -- harmless, but unnecessary.

(Also, I'd like to rename "exitDomFullScreen" to something like "doExitDomFullScreen", since it doesn't have the same meaning as "enterDomFullScreen".  The enter function is called when dom FS is entered; the current exit function is called in order to trigger an exit.)

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Oh, also, this patch keeps the toggler as display:none unless we're actually in browser fullscreen mode.  I think the current state is that it's always visible, maybe even in a normal browser window?

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8493984 [details] [diff] [review]
set display:none when in DOM fullscreen

>@@ -15,13 +15,22 @@ var FullScreen = {
>     window.addEventListener("fullscreen", this, true);
>     window.messageManager.addMessageListener("MozEnteredDomFullscreen", this);
> 
>+    this._fullScrToggler.addEventListener("mouseover", this._expandCallback, false);
>+    this._fullScrToggler.addEventListener("dragenter", this._expandCallback, false);
>+    this._fullScrToggler.style.display = "none";

This is equivalent in XUL:

this._fullScrToggler.hidden = true;

... however, now there's some code using collapsed=true and some code using hidden=true, which is confusing. In fact this element is already collapsed by default. Can you refactor this a bit more such that we use only hidden=true to hide fullscr-toggler?

>   uninit: function() {
>     window.messageManager.removeMessageListener("MozEnteredDomFullscreen", this);
>+
>     this.cleanup();
>+
>+    this._fullScrToggler.removeEventListener("mouseover", this._expandCallback, false);
>+    this._fullScrToggler.removeEventListener("dragenter", this._expandCallback, false);
>+    this._fullScrToggler.style.removeProperty("display");

I don't think these three lines are needed.

Comment 4

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

(In reply to Dão Gottwald [:dao] from comment #3)
> Comment on attachment 8493984 [details] [diff] [review]
> set display:none when in DOM fullscreen
> 
> >@@ -15,13 +15,22 @@ var FullScreen = {
> >     window.addEventListener("fullscreen", this, true);
> >     window.messageManager.addMessageListener("MozEnteredDomFullscreen", this);
> > 
> >+    this._fullScrToggler.addEventListener("mouseover", this._expandCallback, false);
> >+    this._fullScrToggler.addEventListener("dragenter", this._expandCallback, false);
> >+    this._fullScrToggler.style.display = "none";
> 
> This is equivalent in XUL:
> 
> this._fullScrToggler.hidden = true;
> 
> ... however, now there's some code using collapsed=true and some code using
> hidden=true, which is confusing. In fact this element is already collapsed
> by default. Can you refactor this a bit more such that we use only
> hidden=true to hide fullscr-toggler?

Hooo boy, this browser-fullScreen.js code needs a refactor.

vlad - I was wrong in IRC. Toggling DOM Fullscreen does, obviously now, call toggle - nsGlobalWindow bubbles up a fullscreen event when we enter DOM Fullscreen[1].

The toggler is collapsed by default, but the call to this.mouseoverToggle(false); near where vlad is making changes is causing it to uncollapse.

mouseoverToggle(false) is, however, currently the thing that's hiding the toolbox.

The purpose of mouseoverToggle is, as I read it, to do the job of actually switching between the toggler and the toolbox - that's the collapse and marginTop stuff happening towards the end there, and the first boolean parameter is for whether or not we're showing the toolbox.

Perhaps we can make mouseoverToggle more intelligent - such that if mouseoverToggle(false) is called with document.mozFullscreen set to true, that we skip showing the toggler?

[1]: http://hg.mozilla.org/mozilla-central/file/5e704397529b/dom/base/nsGlobalWindow.cpp#l5976

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8493984 [details] [diff] [review]
set display:none when in DOM fullscreen

Let me know if you can't / don't want to spend more time on this, I could pick it up (Mike presumably too).

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hmm, I'm not sure I understand why mouseoverToggle() is needed in this case at all; the element should just stay collapsed if DOM fullscreen is active (I thought that "collapsed" was the 1px-tall state, but guess not).  I can spend more time on this, but I suspect that one of you two will know overall what needs to be done more directly instead of using me as remote-hands to write the code :)  I'm happy to do it if you don't think you'll have time in the next week or so though, let me know.

Comment 7

[Dão Gottwald [:dao]]

Attachment: fullscrtoggler.patch

Comment 8

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

Comment on attachment 8495084 [details] [diff] [review]
fullscrtoggler.patch

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

::: browser/base/content/browser-fullScreen.js
@@ +10,5 @@
>      // called when we go into full screen, even if initiated by a web page script
>      window.addEventListener("fullscreen", this, true);
>      window.messageManager.addMessageListener("MozEnteredDomFullscreen", this);
>  
> +    this._fullScrToggler = document.getElementById("fullscr-toggler");

Why not keep the lazy getter, and add the event listeners in there while we're at it? I know a getElementById is basically O(1), but even if it's infinitesimally small, it's still a non-zero amount of work we're adding to start-up.

Comment 9

[Dão Gottwald [:dao]]

Attachment: fullscrtoggler.patch

moved the _fullScrToggler initialization to FullScreen.toggle. Adding the event listeners in a smart getter doesn't make sense, because no code would otherwise touch this._fullScrToggler early enough for the event listeners to get added when they're needed.

Comment 10

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

Comment on attachment 8496118 [details] [diff] [review]
fullscrtoggler.patch

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

(In reply to Dão Gottwald [:dao] from comment #9)
> Created attachment 8496118 [details] [diff] [review]
> fullscrtoggler.patch
> 
> moved the _fullScrToggler initialization to FullScreen.toggle. Adding the
> event listeners in a smart getter doesn't make sense, because no code would
> otherwise touch this._fullScrToggler early enough for the event listeners to
> get added when they're needed.


Wouldn't |this._fullScrToggler.hidden = aShow || document.mozFullScreen;| ?

Comment 11

[Dão Gottwald [:dao]]

Oh yeah, I guess it would, but I still see no benefit in such a setup. It seems unnecessarily confusing and fragile.

Comment 12

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

Comment on attachment 8496118 [details] [diff] [review]
fullscrtoggler.patch

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

Alright, I'll defer to your best judgement here. This tests just fine. Thanks Dao!

Comment 13

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/cfbe5fc74ea2

Comment 14

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/cfbe5fc74ea2

Comment 15

[Dão Gottwald [:dao]]

Vlad, should this ride trains as usual or would you like it to be uplifted?