Closed Bug 947854 Opened 6 years ago Closed 5 years ago

Exiting DOM fullscreen also exits window fullscreen mode

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
mozilla40
Iteration:
40.3 - 11 May
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 --- wontfix
firefox40 --- verified
firefox-esr31 --- wontfix
firefox-esr38 --- wontfix

People

(Reporter: phlsa, Assigned: xidorn)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [qx:spec])

Attachments

(5 files, 10 obsolete files)

5.74 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.69 KB, patch
dao
: review+
Details | Diff | Splinter Review
10.76 KB, patch
dao
: review+
Details | Diff | Splinter Review
11.51 KB, patch
dao
: review+
Details | Diff | Splinter Review
2.80 KB, patch
dao
: review+
Details | Diff | Splinter Review
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.
Flags: firefox-backlog? → firefox-backlog+
Whiteboard: p=5
Duplicate of this bug: 940841
Duplicate of this bug: 997724
Duplicate of this bug: 745520
Not marking bug 807395 as a duplicate of this one, since I'm not sure if this bug is platform specific or not.
See Also: → 807395
Duplicate of this bug: 1075385
Points: --- → 5
Whiteboard: p=5
Depends on: 1121280
Blocks: 1121280
No longer depends on: 1121280
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.
OS: Mac OS X → All
Hardware: x86 → All
Summary: [Mac] Exiting full screen video playback (HTML5) also exits window full screen mode → Exiting full screen video playback (HTML5) also exits window full screen mode
Version: 28 Branch → Trunk
Depends on: 1123170
[Tracking Requested - why for this release]:
Flags: qe-verify+
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.
Summary: Exiting full screen video playback (HTML5) also exits window full screen mode → Exiting DOM fullscreen also exits window fullscreen mode
Assignee: nobody → quanxunzhen
See Also: 807395
Duplicate of this bug: 807395
Attached patch patch 1 - simplify code (obsolete) — Splinter Review
Attachment #8589954 - Flags: review?(dao)
Attached patch patch 2 - fix this bug (obsolete) — Splinter Review
Attachment #8589956 - Flags: review?(dao)
Attachment #8589956 - Flags: review?(bugs)
Attachment #8589956 - Flags: feedback?(cpearce)
Attached patch patch 3 - testSplinter Review
Attachment #8589957 - Flags: review?(bugs)
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
Attachment #8589956 - Flags: review?(bugs) → review+
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).
Attachment #8589957 - Flags: review?(bugs) → review+
Attached patch patch 4 - browser mochitest (obsolete) — Splinter Review
I suspect this test would probably fail on Mac...
Attachment #8590553 - Flags: review?(dao)
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
Attachment #8590553 - Flags: review?(dao)
Attachment #8589954 - Flags: review?(dao)
Attachment #8589956 - Flags: review?(dao)
Comment on attachment 8589954 [details] [diff] [review]
patch 1 - simplify code

This doesn't seem to cause the problems, hence request for review again.
Attachment #8589954 - Flags: review?(dao)
Attached patch patch 2 - fix this bug, r=smaug (obsolete) — Splinter Review
Attachment #8589956 - Attachment is obsolete: true
Attachment #8589956 - Flags: feedback?(cpearce)
Attachment #8591477 - Flags: review?(dao)
Attachment #8591477 - Flags: feedback?(cpearce)
Attached patch patch 4 - browser mochitest (obsolete) — Splinter Review
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?
Flags: needinfo?(cpearce)
Depends on: 1154479
(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.
Flags: needinfo?(cpearce)
Finally! \o/ https://treeherder.mozilla.org/#/jobs?repo=try&revision=8966ed960679

Will submit the revised patch tomorrow.
Attachment #8590553 - Attachment is obsolete: true
Attachment #8591484 - Attachment is obsolete: true
Attachment #8593637 - Flags: review?(dao)
(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
Attachment #8589954 - Flags: review?(dao)
(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.
Attached patch patch 1 - simplify code (obsolete) — Splinter Review
Attachment #8589954 - Attachment is obsolete: true
Attachment #8596252 - Flags: review?(dao)
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...
Attachment #8596252 - Flags: review?(dao) → review-
Is there any concern if I remove the fullscreen toggler and use MousePosTracker instead?
Flags: needinfo?(dao)
Attached patch patch 1 - simplify code (obsolete) — Splinter Review
Attachment #8596252 - Attachment is obsolete: true
Flags: needinfo?(dao)
Attachment #8596976 - Flags: review?(dao)
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'.
Attachment #8596976 - Flags: review?(dao)
(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.
Attached patch patch 1 - simplify code (obsolete) — Splinter Review
Attachment #8596976 - Attachment is obsolete: true
Attachment #8597051 - Flags: review?(dao)
(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.
Attachment #8597051 - Flags: review?(dao) → review-
(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.
Attachment #8591477 - Flags: review?(dao)
Attached patch patch 1 - simplify code (obsolete) — Splinter Review
Attachment #8597051 - Attachment is obsolete: true
Attachment #8597114 - Flags: review?(dao)
My fault. I forgot I've updated this patch locally before for a change from the trunk.
Attachment #8591477 - Attachment is obsolete: true
Attachment #8591477 - Flags: feedback?(cpearce)
Attachment #8597119 - Flags: review?(dao)
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
Attachment #8597114 - Flags: review?(dao)
Attached patch patch 1 - simplify code (obsolete) — Splinter Review
Oh, ok, thanks. Interesting. I didn't know that.
Attachment #8597114 - Attachment is obsolete: true
Attachment #8597128 - Flags: review?(dao)
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
Attachment #8597128 - Flags: review?(dao) → review-
Hmmm... JavaScript has added a lot of interesting things.
Attachment #8597128 - Attachment is obsolete: true
Attachment #8597210 - Flags: review?(dao)
Attachment #8597210 - Flags: review?(dao) → review+
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.
Attachment #8593637 - Flags: review?(dao) → review+
Attachment #8597119 - Flags: review?(dao) → review+
(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?
Flags: needinfo?(dao)
(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.
Flags: needinfo?(dao) → needinfo?(jaws)
This patch fix the perm-fail on e10s bc1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37be1a734144
Flags: needinfo?(jaws)
Attachment #8601410 - Flags: review?(dao)
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?
Attachment #8601410 - Flags: review?(dao) → review+
Iteration: --- → 40.3 - 11 May
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.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
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
Depends on: 1162752
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.
Blocks: 1105939
Whiteboard: [qx] → [qx:spec]
Depends on: 1176519
Depends on: 1179123
Duplicate of this bug: 1182845
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.
Status: RESOLVED → VERIFIED
Depends on: 1226297
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.