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.

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

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

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

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

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 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;| ?

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!

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