The default bug view has changed. See this FAQ.

fullscr-toggler element needs to be hidden in DOM fullscreen

RESOLVED FIXED in Firefox 35

Status

()

Firefox
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: vlad, Assigned: dao)

Tracking

Trunk
Firefox 35
Points:
3
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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

Comment 1

3 years ago
Created attachment 8493984 [details] [diff] [review]
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.)
Attachment #8493984 - Flags: review?(mconley)
Attachment #8493984 - Flags: review?(dao)
(Reporter)

Comment 2

3 years ago
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?
(Assignee)

Comment 3

3 years ago
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.
(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
(Assignee)

Comment 5

3 years ago
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).
Attachment #8493984 - Flags: review?(mconley)
Attachment #8493984 - Flags: review?(dao)
Attachment #8493984 - Flags: review-
(Assignee)

Updated

3 years ago
OS: Windows 8 → All
Hardware: x86_64 → All
(Reporter)

Comment 6

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: vladimir → dao
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
Summary: fullscr-toggler element needs to be gone completely in DOM fullscreen → fullscr-toggler element needs to be hidden in DOM fullscreen
Version: unspecified → Trunk
(Assignee)

Comment 7

3 years ago
Created attachment 8495084 [details] [diff] [review]
fullscrtoggler.patch
Attachment #8493984 - Attachment is obsolete: true
Attachment #8495084 - Flags: review?(mconley)
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Component: Tabbed Browser → General

Updated

3 years ago
Iteration: --- → 35.2
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.
(Assignee)

Comment 9

3 years ago
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.
Attachment #8495084 - Attachment is obsolete: true
Attachment #8495084 - Flags: review?(mconley)
Attachment #8496118 - Flags: review?(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;| ?
(Assignee)

Comment 11

3 years ago
Oh yeah, I guess it would, but I still see no benefit in such a setup. It seems unnecessarily confusing and fragile.
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!
Attachment #8496118 - Flags: review?(mconley) → review+
(Assignee)

Comment 13

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

Comment 14

3 years ago
https://hg.mozilla.org/mozilla-central/rev/cfbe5fc74ea2
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
(Assignee)

Comment 15

3 years ago
Vlad, should this ride trains as usual or would you like it to be uplifted?
Flags: needinfo?(vladimir)
(Assignee)

Updated

3 years ago
Flags: needinfo?(vladimir)
Depends on: 931445
You need to log in before you can comment on or make changes to this bug.