Steps to reproduce: - Open a page with HTML5 video (e.g. Air Mozilla) in a full screen window - Toggle full screen mode on the video - Exit full screen mode of the video (press ESC) Result: The entire window exits full screen mode Expected result: Just the video should exit full screen mode while the window itself retains full screen.

To be honest, I don't remember a time when this wasn't an issue. Ever since Firefox started using the OS X default fullscreen behavior, I've seen this happening. So I'm not even sure if it is a regression or a less-than-optimal design decision.

If that's the case, and this is Mac-specific (small percentage of overall users) it's not a release blocker but if there's a low risk fix made available, please go ahead with uplift nominations and we can take it up to channels depending on timing & risk.

Not marking bug 807395 as a duplicate of this one, since I'm not sure if this bug is platform specific or not.

This is not a Mac-only problem. It happens on Windows as well. Given the current code for DOM fullscreen highly relies on the fullscreen mode code, I reasonably assume it is a problem affects all platforms. To solve this, we have to make those two parts work (mostly) independently.

[Tracking Requested - why for this release]:

This applies not only to video, but also on HTML5 Canvas. True all platforms? My friends did not see the problem.

(In reply to RblSb@nm.ru from comment #16) > This applies not only to video, but also on HTML5 Canvas. True all > platforms? My friends did not see the problem. All platforms. He is probably using flash to play video, which is not affected by this bug. Flash has its own fullscreen mechanism.

Comment on attachment 8589956 [details] [diff] [review] patch 2 - fix this bug >+ nsRefPtr<AsyncEventDispatcher> asyncDispatcher = new AsyncEventDispatcher( >+ this, NS_LITERAL_STRING("MozExitedDomFullscreen"), true, true); Nit, 2 space indentation, please

Comment on attachment 8589957 [details] [diff] [review] patch 3 - test So this tests the events. Don't we want also actual browser chrome test too (which dao should probably review).

I suspect this test would probably fail on Mac...

Comment on attachment 8590553 [details] [diff] [review] patch 4 - browser mochitest need to find out why this test cause failures first... https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf2c153849cb

Comment on attachment 8589954 [details] [diff] [review] patch 1 - simplify code This doesn't seem to cause the problems, hence request for review again.

The only left problems are that this test fails on Linux without e10s, and Windows & Mac with e10s: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b6822a36c7c I really have no idea what I can do to fix those failures. Any suggestions?

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #28) > Created attachment 8591484 [details] [diff] [review] > patch 4 - browser mochitest > > The only left problems are that this test fails on Linux without e10s, and > Windows & Mac with e10s: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b6822a36c7c > > I really have no idea what I can do to fix those failures. Any suggestions? Dunno. You must have a race somewhere.

Finally! \o/ https://treeherder.mozilla.org/#/jobs?repo=try&revision=8966ed960679 Will submit the revised patch tomorrow.

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #31) > Created attachment 8593637 [details] [diff] [review] > patch 4 - browser mochitest Man, I love youuuuu. Looked today the latest nightly build, where the problem is still there. When you will see a fix? Really want :3

(In reply to RblSb@nm.ru from comment #32) > (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #31) > > Created attachment 8593637 [details] [diff] [review] > > patch 4 - browser mochitest > > Man, I love youuuuu. Looked today the latest nightly build, where the > problem is still there. When you will see a fix? Really want :3 When I get my patches granted from :dao.

I'll try to get to this tomorrow.

Comment on attachment 8589954 [details] [diff] [review] patch 1 - simplify code >--- a/browser/base/content/browser-fullScreen.js >+++ b/browser/base/content/browser-fullScreen.js >@@ -68,25 +68,21 @@ var FullScreen = { > > if (enterFS) { > document.addEventListener("keypress", this._keyToggleCallback, false); > document.addEventListener("popupshown", this._setPopupOpen, false); > document.addEventListener("popuphidden", this._setPopupOpen, false); > // We don't animate the toolbar collapse if in DOM full-screen mode, > // as the size of the content area would still be changing after the > // mozfullscreenchange event fired, which could confuse content script. >- this._shouldAnimate = !document.mozFullScreen; >- this.mouseoverToggle(false); >+ this._shouldAnimate = true; >+ this.hideNavToolbox(document.mozFullScreen); The above comment was referring to this._shouldAnimate = !document.mozFullScreen. Now it doesn't seem to make sense anymore. >+ hideNavToolbox: function(forceHide) >+ { While you're at it, please put these on one line: hideNavToolbox: function(forceHide) { >+#main-window[inDOMFullscreen] #navigator-toolbox { >+ display: none; >+} I'm not sure what this is about. You're still calling hideNavToolbox for DOM fullscreen mode anyway, aren't you? >+#main-window[inFullscreen] #navigator-toolbox[shouldAnimate] { >+ transition: 1.5s margin-top ease-out; >+} * the descendent selector isn't needed here, as the inFullscreen is also present on #navigator-toolbox * please rename shouldAnimate to something more fullscreen-specific * will this disable the margin-left and margin-right transitions when entering customize mode from fullscreen? http://hg.mozilla.org/mozilla-central/annotate/946ac85af8f4/browser/base/content/browser.css#l1087

(In reply to Dão Gottwald [:dao] from comment #35) > Comment on attachment 8589954 [details] [diff] [review] > patch 1 - simplify code > >+#main-window[inDOMFullscreen] #navigator-toolbox { > >+ display: none; > >+} > > I'm not sure what this is about. You're still calling hideNavToolbox for DOM > fullscreen mode anyway, aren't you? Yes, but I want to ensure that the toolbox won't appear accidently. > >+#main-window[inFullscreen] #navigator-toolbox[shouldAnimate] { > >+ transition: 1.5s margin-top ease-out; > >+} > > * please rename shouldAnimate to something more fullscreen-specific What about adding a prefix, say fullscreenShouldAnimate? > * will this disable the margin-left and margin-right transitions when > entering customize mode from fullscreen? > http://hg.mozilla.org/mozilla-central/annotate/946ac85af8f4/browser/base/ > content/browser.css#l1087 Probably yes in some case. Normally it won't. I guess I can just add an event listener to remove that attribute once the transition finishes.

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #36) > (In reply to Dão Gottwald [:dao] from comment #35) > > Comment on attachment 8589954 [details] [diff] [review] > > patch 1 - simplify code > > >+#main-window[inDOMFullscreen] #navigator-toolbox { > > >+ display: none; > > >+} > > > > I'm not sure what this is about. You're still calling hideNavToolbox for DOM > > fullscreen mode anyway, aren't you? > > Yes, but I want to ensure that the toolbox won't appear accidently. That would mean the code is broken, wouldn't it? We should fix that, if needed, rather than having two pieces of code doing the same thing... > > >+#main-window[inFullscreen] #navigator-toolbox[shouldAnimate] { > > >+ transition: 1.5s margin-top ease-out; > > >+} > > > > * please rename shouldAnimate to something more fullscreen-specific > > What about adding a prefix, say fullscreenShouldAnimate? ok.

(In reply to Dão Gottwald [:dao] from comment #37) > (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #36) > > (In reply to Dão Gottwald [:dao] from comment #35) > > > Comment on attachment 8589954 [details] [diff] [review] > > > patch 1 - simplify code > > > >+#main-window[inDOMFullscreen] #navigator-toolbox { > > > >+ display: none; > > > >+} > > > > > > I'm not sure what this is about. You're still calling hideNavToolbox for DOM > > > fullscreen mode anyway, aren't you? > > > > Yes, but I want to ensure that the toolbox won't appear accidently. > > That would mean the code is broken, wouldn't it? We should fix that, if > needed, rather than having two pieces of code doing the same thing... Well, actually just a double assurance. I think it should be fine to just remove the CSS code here without breaking anything.

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #36) > (In reply to Dão Gottwald [:dao] from comment #35) > > Comment on attachment 8589954 [details] [diff] [review] > > patch 1 - simplify code > > * will this disable the margin-left and margin-right transitions when > > entering customize mode from fullscreen? > > http://hg.mozilla.org/mozilla-central/annotate/946ac85af8f4/browser/base/ > > content/browser.css#l1087 > > Probably yes in some case. Normally it won't. I guess I can just add an > event listener to remove that attribute once the transition finishes. I guess it is not necessary. Is it possible to enter customize mode from fullscreen when the toolbox is hidden? Only that case could cause the transition be disabled. When the toolbox is shown, that attribute is never set.

Comment on attachment 8596252 [details] [diff] [review] patch 1 - simplify code >+#main-window[inFullscreen] #navigator-toolbox[fullscreenShouldAnimate] { >+ transition: 1.5s margin-top ease-out; >+} As mentioned in comment 35, please get rid of the descendent selector. This patch appears to make the "fullscreen toggler" (the thin black bar for unhiding toolbars) visible while the toolbox animates...

Is there any concern if I remove the fullscreen toggler and use MousePosTracker instead?

Comment on attachment 8596976 [details] [diff] [review] patch 1 - simplify code >+ if (this._shouldAnimate && !forceHide) { >+ gNavToolbox.setAttribute("fullscreenShouldAnimate", true); >+ this._shouldAnimate = false; >+ // Hide the fullscreen toggler until the transition ends. >+ gNavToolbox.addEventListener("transitionend", function listener() { >+ gNavToolbox.removeEventListener("transitionend", listener, true); >+ if (self._isChromeCollapsed) >+ self._fullScrToggler.hidden = false; >+ }, true); >+ self._fullScrToggler.hidden = true; >+ } Please use an arrow function: let listener = event => { [...] }; gNavToolbox.addEventListener("transitionend", listener, true); ... and use 'this' instead of 'self'.

(In reply to Dão Gottwald [:dao] from comment #44) > Comment on attachment 8596976 [details] [diff] [review] > patch 1 - simplify code > > >+ if (this._shouldAnimate && !forceHide) { > >+ gNavToolbox.setAttribute("fullscreenShouldAnimate", true); > >+ this._shouldAnimate = false; > >+ // Hide the fullscreen toggler until the transition ends. > >+ gNavToolbox.addEventListener("transitionend", function listener() { > >+ gNavToolbox.removeEventListener("transitionend", listener, true); > >+ if (self._isChromeCollapsed) > >+ self._fullScrToggler.hidden = false; > >+ }, true); > >+ self._fullScrToggler.hidden = true; > >+ } > > Please use an arrow function: > > let listener = event => { > [...] > }; > gNavToolbox.addEventListener("transitionend", listener, true); > > ... and use 'this' instead of 'self'. I don't think 'this' can be used here. If you don't like 'self', then probably 'FullScreen' should be used instead.

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #45) > (In reply to Dão Gottwald [:dao] from comment #44) > > Please use an arrow function: > > > > let listener = event => { > > [...] > > }; > > gNavToolbox.addEventListener("transitionend", listener, true); > > > > ... and use 'this' instead of 'self'. > > I don't think 'this' can be used here. It can.

Comment on attachment 8597051 [details] [diff] [review] patch 1 - simplify code This patch also seems to contain a bunch of debug logging that we don't want to land.

(In reply to Dão Gottwald [:dao] from comment #48) > Comment on attachment 8597051 [details] [diff] [review] > patch 1 - simplify code > > This patch also seems to contain a bunch of debug logging that we don't want > to land. Sorry about forgetting to remove the debug loggings. But those loggings are easy to remove. Please do not block on things which can be fixed simply. Could you tell me if there is any other problem? If no, could you continue to review the remining patches?

Comment on attachment 8591477 [details] [diff] [review] patch 2 - fix this bug, r=smaug This doesn't properly apply on top of the current patch 1. It will probably need an update once patch 1 is done. That's why I was waiting with the review of this patch.

My fault. I forgot I've updated this patch locally before for a change from the trunk.

Comment on attachment 8597114 [details] [diff] [review] patch 1 - simplify code >+ let listener = () => { >+ gNavToolbox.removeEventListener("transitionend", listener, true); >+ if (FullScreen._isChromeCollapsed) >+ FullScreen._fullScrToggler.hidden = false; >+ }; 'this' works perfectly fine here. That's actually the only reason why I asked you to use an arrow function. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions#Lexical_this

Oh, ok, thanks. Interesting. I didn't know that.

Comment on attachment 8597128 [details] [diff] [review] patch 1 - simplify code >+ showNavToolbox: function(trackMouse) { How about making trackMouse the default so that callers can opt out only when they really want that: showNavToolbox: function(trackMouse = true) { > function focusAndSelectUrlBar() { > if (gURLBar) { > if (window.fullScreen) >- FullScreen.mouseoverToggle(true); >+ FullScreen.showNavToolbox(); > > gURLBar.select(); > if (document.activeElement == gURLBar.inputField) > return true; > } > return false; > } Not tracking the mouse here seems like a bad behavior change. STR: * Go fullscreen, wait for toolbars to hide * Press Accel+L * Unfocus the location bar by clicking in the content area * Move the mouse to the top of the screen and back over the content area

Hmmm... JavaScript has added a lot of interesting things.

Comment on attachment 8593637 [details] [diff] [review] patch 4 - browser mochitest >+ window.focus(); >+ browser.focus(); I don't think these are needed, the window and the browser being focused is the expected default behavior here.

(In reply to Dão Gottwald [:dao] from comment #57) > Comment on attachment 8593637 [details] [diff] [review] > patch 4 - browser mochitest > > >+ window.focus(); > >+ browser.focus(); > > I don't think these are needed, the window and the browser being focused is > the expected default behavior here. IIRC, without these calls, the test may intermittent timeout in some platforms. But I'll push a try without them to see whether it works fine.

Hmmmm.... A perm-fail in another bc test with Linux debug builds on e10s...

I cannot reproduce that failure in my local linux debug build, hence I have no clear idea how to fix that failure. However, I do see some weird behavior when it runs after the test before it, which is browser_885052_customize_mode_observers_disabed.js. The test for 885052 is related to fullscreen. When those tests are run sequentially, the windows created in test 885530 (the failing test above) may be maximized, which are in normal mode when it is run individually. This behavior can be seen without this patchset, hence not actually a regression from my patches. But I suspect that the weird behavior may relate to the failure, and it seems disabling test 885052 on linux e10s does make things work: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48cbc6d990a1 So could I disable that, and leave it to be investigated later?

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #61) > I cannot reproduce that failure in my local linux debug build, hence I have > no clear idea how to fix that failure. However, I do see some weird behavior > when it runs after the test before it, which is > browser_885052_customize_mode_observers_disabed.js. > > The test for 885052 is related to fullscreen. When those tests are run > sequentially, the windows created in test 885530 (the failing test above) > may be maximized, which are in normal mode when it is run individually. This > behavior can be seen without this patchset, hence not actually a regression > from my patches. > > But I suspect that the weird behavior may relate to the failure, and it > seems disabling test 885052 on linux e10s does make things work: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=48cbc6d990a1 > > So could I disable that, and leave it to be investigated later? Deferring to Jared who wrote that test.

This patch fix the perm-fail on e10s bc1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37be1a734144

I was sending the request to the original reviewer of that test, :Unfocused. However it seems he's currently not reviewing patches (and blocks review requests...) So, dao, could you review that?

https://hg.mozilla.org/integration/mozilla-inbound/rev/373a497c69ca https://hg.mozilla.org/integration/mozilla-inbound/rev/f1cd052c7fb7 https://hg.mozilla.org/integration/mozilla-inbound/rev/4ac24fe93a88 https://hg.mozilla.org/integration/mozilla-inbound/rev/e40fca3b4a54 https://hg.mozilla.org/integration/mozilla-inbound/rev/f5c29147aa66

https://hg.mozilla.org/mozilla-central/rev/373a497c69ca https://hg.mozilla.org/mozilla-central/rev/f1cd052c7fb7 https://hg.mozilla.org/mozilla-central/rev/4ac24fe93a88 https://hg.mozilla.org/mozilla-central/rev/e40fca3b4a54 https://hg.mozilla.org/mozilla-central/rev/f5c29147aa66

I guess I probably should have landed it in fx-team instead of mozilla-inbound. I'm not sure what's the difference between those two repos. It doesn't seem to be a big issue, though.

Noticed this landed while working on something else. I suspect this may have fixed some other issues with this transition. Setting needinfo so I can look later.

After exiting fullscreen site set to full screen and hides all the panels Firefox, but you can show the panel by pointing the mouse up. Interesting bug :) OSX 10.9, last FF build

Can not you fix it at least FF v.39? In this case, we have to wait another month v.40 and v.41 some even (for example I D:). The FF v.38 on YouTube completely removed flash player. Use html5 intolerable in any version of the browser -_- I drink vodka and crying

(In reply to RblSb@nm.ru from comment #71) > Can not you fix it at least FF v.39? In this case, we have to wait another > month v.40 and v.41 some even (for example I D:). The FF v.38 on YouTube > completely removed flash player. Use html5 intolerable in any version of the > browser -_- I drink vodka and crying This patchset is non-trivial, and also caused regression (bug 1162752). It is probably too risky to get the approval for uplifting them to Beta. Sorry.

I was able to reproduce this issue on Firefox 40.0a1 (2015-05-01) under Windows 7 64-bit. Verified fixed on Firefox 40 Beta 4 (20150713153304) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5.

This bug still exists in Linux (Firefox 60.0.2 - Fedora 28 x64 xfce) if the title bar is hidden (customize option). The entire window exits full screen mode when exiting a fullscreen html5 video.

(In reply to didli from comment #78) > This bug still exists in Linux (Firefox 60.0.2 - Fedora 28 x64 xfce) if the > title bar is hidden (customize option). The entire window exits full screen > mode when exiting a fullscreen html5 video. Please file a separate bug with more details, and please check it continues to reproduce on 61 (current release) and, ideally, nightly (63, https://nightly.mozilla.org/ ). Thank you!

Problem still persists in version 63 (64-bit) for Ubuntu.

Please file a new bug.