Maas-Maarten Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_1) AppleWebKit/601.2.7 (KHTML, like Gecko) Version/9.0.1 Safari/601.2.7

Steps to reproduce:

When you go to http://jsfiddle.net/0GiS0/cAG5N/ you can show and log visibility changes for document.hidden.




Actual results:

This works as expected for changing tabs, minimizing the window, but not when the browser window is completely hidden by another window. 


Expected results:

The visibility api spec:

On getting, the hidden attribute MUST return true if the Document contained by the top level browsing context (root window in the browser's viewport) [HTML5] is not visible at all. The attribute MUST return false if the Document contained by the top level browsing context is at least partially visible on at least one screen.

This is also what chrome and safari do.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 8 years ago

> This is also what chrome and safari do.

Which "this"?

In general, detecting window occlusion like this is quite tricky, especially for the case of translucent windows.  During the spec discussion for the visibility API specification, it was quite clear that occlusion did NOT fall into the set of things that have to cause document.hidden to become true (though it's allowed to if UAs want to go to the trouble, as long as they handle all the translucency edge cases correctly).

It's possible that Mac OS has a simple API for detecting this sort of thing, of course.  It's not clear to me whether other platforms do, and whether we'd want to use this even if it did; we generally don't want to throttle web pages that are in foreground tabs (which is what setting .hidden would pretty much involve) just because they're occluded, because it would mess with things like audio playback.

	Andrew Overholt [:overholt]
	Comment 2 • 8 years ago

Given comment 1 I'm inclined to split this bug into widget-specific bugs but do we want to go to the trouble of dealing with translucent "covering" windows?

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 8 years ago

If the OS provides an API that deals for it with us, we might as well.  If not, we do not want to go anywhere near this mess, imo.

	danny
	Comment 4 • 8 years ago

This also doesn't work on OSX if you have Firefox set to fullscreen mode, then switch to another fullscreen application, or if you have Firefox windowed and switch to another desktop. This is a pain because one of the main uses of desktop notifications is if the webpage isn't visible, but a lot of the time that it isn't visible, document.hidden still returns false on Firefox

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 8 years ago

Comment 4 should be a separate bug, probably.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 6 • 7 years ago

Hi Edgar, please help from investigating this! Thank you!

	Edgar Chen [:edgar] Assignee
	Comment 7 • 7 years ago

Test results for each OS and different Browser:
         
Does document.hidden change to true when the browser window is completely covered by another non-translucent application,

            | Firefox | Google Chrome | Edge | Safari
------------+---------+---------------+------+--------
 Windows 10 |    No   |       No      |  No  | ------
------------+---------+---------------+------+--------
 Mac        |    No   |       Yes     | ---- |  Yes
------------+---------+---------------+------+--------
 Linux      |    No   |       No      | ---- | ------

	Edgar Chen [:edgar] Assignee
	Comment 8 • 7 years ago

Not sure other browsers don't support on Window 10 and Linux is because they don't implement or the OS doesn't provide such information (Mac provides API to manage occlusion state [1]).

Hi :jimm, do you know if Windows or GTK provides such information? or do you know who might know this?

Thank you.
	

[1] https://developer.apple.com/reference/appkit/nswindowdelegate/1419424-windowdidchangeocclusionstate?language=objc

	Jim Mathies [:jimm]
	Comment 9 • 7 years ago

Here's a Chrome proposal related to this for OSX, not sure how old it is:
https://www.chromium.org/developers/design-documents/mac-occlusion

I'm not aware of a Windows API or event that exposes occlusion state.

	Jim Mathies [:jimm]
	Comment 10 • 7 years ago

Karl, any ideas on GTK support?

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 11 • 7 years ago

:jerry told me this API is available after Windows 8 https://msdn.microsoft.com/zh-tw/library/windows/desktop/hh768892(v=vs.85).aspx

	Karl Tomlinson (:karlt)
	Comment 12 • 7 years ago

Most users of the GTK port will have compositing window managers, where the VisibilityNotify events are not provided (unless the window manager goes to special effort to emulate, but I've never heard of that), so I don't think implementation would be practical in the GTK port.

	Edgar Chen [:edgar] Assignee
	Comment 13 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23b3286d7101440dc201c0fe35624cae545b6ca2

	Edgar Chen [:edgar] Assignee
	Comment 14 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6015624dbd6026c45ff0389f5e011c255b425267

	Edgar Chen [:edgar] Assignee
	Comment 15 • 7 years ago

> https://msdn.microsoft.com/zh-tw/library/windows/desktop/hh768892(v=vs.85).aspx

The windows' API doesn't exactly meet what we need here. The definition of "An app is occluded" in this API is:
- All the monitors are off.
- The session that the app runs in is disconnected.
- The app runs in full screen exclusively on a single monitor configuration.
- The app runs on a different desktop than the active desktop and does not have permission to render on the active desktop.

And I also did some tests in Windows 8; the notification isn't filed for "fully covered by other application" case.

	Edgar Chen [:edgar] Assignee
	Comment 16 • 7 years ago

> https://developer.apple.com/reference/appkit/nswindowdelegate/1419424-windowdidchangeocclusionstate?language=objc

The Mac's API is only available after SDK macOS 10.9, but our release build is still using SDK macOS 10.7 [1] at this moment.

[1] The build target in about:buildconfig is x86_64-apple-darwin11.2.0

	Edgar Chen [:edgar] Assignee
	Comment 17 • 7 years ago

For the reference, attach the WIP patch of the MAC implementation.

	Chris Peterson [:cpeterson]
	Comment 18 • 7 years ago

Here is a convenient test case for the Page Visibility API. The page plays a video with sound that will be paused when the page is hidden so you can hear whether the page received the "visibilitychange" notification.

http://daniemon.com/tech/webapps/page-visibility/

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 19 • 7 years ago

According to comment 15 and comment 16, I am wondering how we are going to proceed this bug? Do we still consider this as qf:p1? Mind providing suggestions here, Boris?

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 7 years ago

I don't have any great ideas.

Can we runtime-detect the Mac thing?

On Windows, if the notification is not fired when there's another window that's maximized, then I don't see how we can do much about this there.

	Edgar Chen [:edgar] Assignee
	Comment 21 • 7 years ago

> Can we runtime-detect the Mac thing?

I am not sure if runtime-detection is possible in widget code. :jimm, is it possible doing that?

	Jim Mathies [:jimm]
	Comment 22 • 7 years ago

(In reply to Edgar Chen [:edgar] from comment #21)
> > Can we runtime-detect the Mac thing?
> 
> I am not sure if runtime-detection is possible in widget code. :jimm, is it
> possible doing that?

Good question, we do this on windows all the time, not sure about OSX. Stephen, any idea?

	Stephen A Pohl [:spohl]
	Comment 23 • 7 years ago

(In reply to Jim Mathies [:jimm] from comment #22)
> (In reply to Edgar Chen [:edgar] from comment #21)
> > > Can we runtime-detect the Mac thing?
> > 
> > I am not sure if runtime-detection is possible in widget code. :jimm, is it
> > possible doing that?
> 
> Good question, we do this on windows all the time, not sure about OSX.
> Stephen, any idea?

We only support Mavericks (10.9) and above, so it can be assumed that this API will be available at runtime (see bug 1282251 and [1]). For runtime detection in other cases we use the helper functions in nsCocoaFeatures.mm [2].

[1] https://www.mozilla.org/en-US/firefox/52.0/system-requirements/
[2] https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsCocoaFeatures.mm

	Edgar Chen [:edgar] Assignee
	Comment 24 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=757d755a9dc06c612b8b2c1ec43e4573f5e3e9c3

	Edgar Chen [:edgar] Assignee
	Comment 25 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83283d30237eee7dcc94e6017b20289f6b4d9257

	Edgar Chen [:edgar] Assignee
	Comment 26 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a2d0f9f800d469bc9c07be6b73d9741ed0c83cc

	Edgar Chen [:edgar] Assignee
	Comment 30 • 7 years ago

Comment on attachment 8871682 [details] [diff] [review]
Part 1: Send "occlusionstatechange" custom event when occlusion state in Mac is changed, v1

This patch implements occlusion state API on Mac.
- https://developer.apple.com/reference/appkit/nswindowdelegate/1419424-windowdidchangeocclusionstate?language=objc
- https://developer.apple.com/reference/appkit/nswindow/1419321-occlusionstate?language=objc

We do run-time detect here since these APIs are available after SDK 10.9.
And then dispatch "occlusionstatechange" custom event when occlustion state is changed.

spohl, could you help to review this patch? Thank you.

	Edgar Chen [:edgar] Assignee
	Comment 31 • 7 years ago

Comment on attachment 8871682 [details] [diff] [review]
Part 1: Send "occlusionstatechange" custom event when occlusion state in Mac is changed, v1

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

::: widget/cocoa/nsCocoaWindow.mm
@@ +1983,5 @@
>  
>  void
> +nsCocoaWindow::DispatchOcclusionEvent()
> +{
> +  if (!mWindow || ![mWindow respondsToSelector:@selector(occlusionState)]) {

We probably can remove `![mWindow respondsToSelector:@selector(occlusionState)]` since we only support Mavericks (10.9) and above, we can assume that "occlustionState" will be always available at runtime (per comment #23).

	Edgar Chen [:edgar] Assignee
	Comment 32 • 7 years ago

Comment on attachment 8871683 [details] [diff] [review]
Part 2: Add isFullyOccluded attribute to ChromeWindow, v1

This patch adds isFullyOccluded attribute to ChromeWindow for tabbrowser accessing. And for now, only Mac (after 10.9) will possibly return true; other platform always returns false, since only Mac support reporting occlusion state.

bz, could you help to review this patch? Thank you.

	Edgar Chen [:edgar] Assignee
	Comment 33 • 7 years ago

Comment on attachment 8871684 [details] [diff] [review]
Part 3: Set docShellIsActive to false when browser window is fully covered by another application, v1

This patch contains three things,

1. Set browser.docShellIsActive to false when window.isFullyOccluded reports true which is only possibly happened on Mac (after 10.9) since only Mac support reporting occlusion state.

2. Add test only be run on Mac.

3. I found tabbrowser changes break some existing tests for fullscreen, browser_domFullscreen_fullscreenMode.js and browser_bug1236512.js.
The root cause: switching fullscreen mode possibly trigger docshell being set to non-activate and then set to activate back again, but test tries to request fullscreen before docshell backs to active, then fullscreen request is denied.
So those test should wait docshell being activated again before starting next test. 

mconley, could you help to review this? Thank you.

	Edgar Chen [:edgar] Assignee
	Comment 34 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=855efa1b52976b2de5b5009bc8ab7d8b79b8ef1d&filter-tier=1&group_state=expanded

	Stephen A Pohl [:spohl]
	Comment 35 • 7 years ago

Comment on attachment 8871682 [details] [diff] [review]
Part 1: Send "occlusionstatechange" custom event when occlusion state in Mac is changed, v1

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

The Cocoa parts look good to me, but I would like for Markus to take a look as well. Thanks!

::: widget/cocoa/nsCocoaWindow.mm
@@ +1983,5 @@
>  
>  void
> +nsCocoaWindow::DispatchOcclusionEvent()
> +{
> +  if (!mWindow || ![mWindow respondsToSelector:@selector(occlusionState)]) {

Yes, this can be dropped.

@@ +2737,5 @@
>    NS_OBJC_END_TRY_ABORT_BLOCK;
>  }
>  
> +// This method is on NSWindowDelegate starting with 10.9
> +- (void)windowDidChangeOcclusionState:(NSNotification *)aNotification

nit: no space between NSNotification and *

@@ +2740,5 @@
> +// This method is on NSWindowDelegate starting with 10.9
> +- (void)windowDidChangeOcclusionState:(NSNotification *)aNotification
> +{
> +  if (mGeckoWindow)
> +    mGeckoWindow->DispatchOcclusionEvent();

Please wrap this in {} braces.

	Markus Stange [:mstange]
	Comment 36 • 7 years ago

Comment on attachment 8871682 [details] [diff] [review]
Part 1: Send "occlusionstatechange" custom event when occlusion state in Mac is changed, v1

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

The Cocoa parts look good, with Stephen's comments addressed.

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 7 years ago

Comment on attachment 8871683 [details] [diff] [review]
Part 2: Add isFullyOccluded attribute to ChromeWindow, v1

> Right now, only Mac (after 10.9) will possible return true; other platform is always

s/possible/possibly/ and s/platform is/platforms/

r=me.  Thank you for doing this!

	Mike Conley (:mconley) (:⚙️)
	Comment 38 • 7 years ago

Comment on attachment 8871684 [details] [diff] [review]
Part 3: Set docShellIsActive to false when browser window is fully covered by another application, v1

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

This looks good! I have enough feedback though that I think I'd like another look when another draft is up. Thanks!

::: browser/base/content/tabbrowser.xml
@@ +4090,5 @@
>                }
>  
>                // Show or hide the spinner as needed.
>                let needSpinner = this.getTabState(showTab) != this.STATE_LOADED &&
> +                                !this.minimizedOrFullyOccluded &&

If I recall correctly, there's a similar check above for shouldBeBlank that we also need to update:

http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/browser/base/content/tabbrowser.xml#4136

@@ +5038,2 @@
>                if (aEvent.target == window && !this._switcher) {
>                  this.mCurrentBrowser.preserveLayers(window.windowState == window.STATE_MINIMIZED);

Should we preserve layers here if we're fully occluded as well?

::: browser/base/content/test/general/browser_domFullscreen_fullscreenMode.js
@@ +81,5 @@
>      let sizemodeChanged = false;
>      function tryResolve() {
>        if ((!(aFlags & FS_CHANGE_DOM) || fullscreenData) &&
>            (!(aFlags & FS_CHANGE_SIZE) || sizemodeChanged)) {
> +        // In the platform that support reporting occlusion state (e.g. Mac),

Nit: "platform" -> "platforms"

@@ +84,5 @@
>            (!(aFlags & FS_CHANGE_SIZE) || sizemodeChanged)) {
> +        // In the platform that support reporting occlusion state (e.g. Mac),
> +        // enter/exit fullscreen mode will trigger docshell being set to
> +        // non-activate and then set to activate back again.
> +        // For those platform, we should wait docshell being activated again,

Nit: "we should wait docshell being activated again" -> "we should wait until the docshell has been activated again"

@@ +85,5 @@
> +        // In the platform that support reporting occlusion state (e.g. Mac),
> +        // enter/exit fullscreen mode will trigger docshell being set to
> +        // non-activate and then set to activate back again.
> +        // For those platform, we should wait docshell being activated again,
> +        // otherwise fullscreen request would possible be denied in next test.

Nit: "otherwise fullscreen request would possible be denied" -> "otherwise, the fullscreen request might be denied"

::: dom/html/test/test_fullscreen-api.html
@@ +105,5 @@
>        // OS X Lion before we run the test.
>        testWindow.addEventListener("load", function() {
>          SimpleTest.waitForFocus(function() {
> +          SimpleTest.waitForFocus(function() {
> +            // For the platform that support reporting occlusion state (e.g. Mac),

Please copy the nit corrections from the browser_domFullscreen_fullscreenMode.js to here as well.

::: dom/tests/browser/browser_bug1236512.js
@@ +1,1 @@
> +/**

Please add "use strict"; under the license.

@@ +4,5 @@
> + */
> +
> +const testPageURL = "http://mochi.test:8888/browser/dom/tests/browser/dummy.html";
> +
> +function* testContentVisibilityState(aIsHidden, aBrowser) {

In general, I think since bug 1353542, we've been trying to have mochitest-browser tests use async functions and await instead of tasks and yield. Can you switch these over?

@@ +40,5 @@
> +  let winTest = yield BrowserTestUtils.openNewBrowserWindow({ width: 500,
> +                                                              height: 500,
> +                                                              left: 200,
> +                                                              top: 200 });
> +  let browser = winTest.gBrowser;

"browser" is an unfortunately overloaded term. To avoid confusion, we should probably call this otherTabBrowser or something.

@@ +51,5 @@
> +  yield testContentVisibilityState(false /* isHidden */, browser);
> +
> +  info("test window should report 'hidden' if it is fully covered by another " +
> +       "window");
> +  window.focus();

You might need to use waitForFocus here - same for the other callers. I can't remember if window focusing is synchronous or not, but waitForFocus suggests no.

@@ +57,5 @@
> +
> +  info("test window should still report 'hidden' since it is still fully covered "
> +       + "by another window");
> +  browser.selectedTab = browser.addTab();
> +  browser.removeCurrentTab();

Probably best to use BrowserTestUtils.removeTab instead.

@@ +66,5 @@
> +  winTest.focus();
> +  yield waitContentVisibilityChange(false /* isHidden */, browser);
> +
> +  info("closing test window");
> +  winTest.close();

Should use BrowserTestUtils.closeWindow.

	Edgar Chen [:edgar] Assignee
	Comment 39 • 7 years ago

(In reply to Mike Conley (:mconley) from comment #38)
> Comment on attachment 8871684 [details] [diff] [review]
> Part 3: Set docShellIsActive to false when browser window is fully covered
> by another application, v1
> 
> Review of attachment 8871684 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: browser/base/content/tabbrowser.xml
> @@ +4090,5 @@
> >                // Show or hide the spinner as needed.
> >                let needSpinner = this.getTabState(showTab) != this.STATE_LOADED &&
> > +                                !this.minimizedOrFullyOccluded &&
> 
> If I recall correctly, there's a similar check above for shouldBeBlank that

Thank you for catching this!!.

> 
> @@ +5038,2 @@
> >                if (aEvent.target == window && !this._switcher) {
> >                  this.mCurrentBrowser.preserveLayers(window.windowState == window.STATE_MINIMIZED);
> 
> Should we preserve layers here if we're fully occluded as well?

Yeah, we should preserve layers when we're fully occluded as well. Will fix this.

> ::: dom/tests/browser/browser_bug1236512.js
> @@ +4,5 @@
> > + */
> > +
> > +const testPageURL = "http://mochi.test:8888/browser/dom/tests/browser/dummy.html";
> > +
> > +function* testContentVisibilityState(aIsHidden, aBrowser) {
> 
> In general, I think since bug 1353542, we've been trying to have
> mochitest-browser tests use async functions and await instead of tasks and
> yield. Can you switch these over?

Sure. Will switch to async functions and await.

	Edgar Chen [:edgar] Assignee
	Comment 40 • 7 years ago

Address review comment #35.

	Edgar Chen [:edgar] Assignee
	Comment 41 • 7 years ago

Address review comment #37.

	Edgar Chen [:edgar] Assignee
	Comment 42 • 7 years ago

Address review comment #38,
- Update shouldBeBlank check
- Preserve layers if we're fully occluded
- Fix nits
- Add "use strict"
- Switch to async functions and await
- s/browser/browserTest/
- Use waitForFocus indead
- Use the utilities of BrowserTestUtils instead.

I found browser/base/content/test/general/browser_minimize.js will get failed intermittently [1] on Mac after applying these patches. And the failure can not be reproduced on my Mac, it seems it can only be reproduced in try (not super sure). I am still debugging it.

[1] the try result in comment #34.

	Edgar Chen [:edgar] Assignee
	Comment 43 • 7 years ago

(In reply to Edgar Chen [:edgar] from comment #42)
> I found browser/base/content/test/general/browser_minimize.js will get
> failed intermittently [1] on Mac after applying these patches. And the
> failure can not be reproduced on my Mac, it seems it can only be reproduced
> in try (not super sure). I am still debugging it.
> 
> [1] the try result in comment #34.

I found in failure case, browser_fullscreen-window-open.js makes test window stay in fully occluded state. And browser_minimize.js fails in checking initial condition. If we skip browser_fullscreen-window-open.js test, we don't hit this failure any more [1]. It looks like browser_fullscreen-window-open.js sometimes puts test window in some werid state. Not sure what's happened, investigating ...

I tried adding waitForFocus() at the beginning of browser_minimize.js, but it doesn't help.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd93c9a03c59797ae9dc8e2d90484cefd806d3dd&group_state=expanded

	Edgar Chen [:edgar] Assignee
	Comment 44 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fff3a98d2386ee2cfe7f8cf7fa9b8602a77df359&filter-tier=1&filter-tier=2&filter-tier=3&group_state=expanded

	Edgar Chen [:edgar] Assignee
	Comment 45 • 7 years ago

Comment on attachment 8875669 [details] [diff] [review]
Part 3: Set docShellIsActive to false when browser window is fully covered by another application, v2

(In reply to Edgar Chen [:edgar] from comment #42)
> Address review comment #38,
> - Update shouldBeBlank check
> - Preserve layers if we're fully occluded
> - Fix nits
> - Add "use strict"
> - Switch to async functions and await
> - s/browser/browserTest/
> - Use waitForFocus indead
> - Use the utilities of BrowserTestUtils instead.

mconley, may I have your review again? Thank you.

	Edgar Chen [:edgar] Assignee
	Comment 46 • 7 years ago

Comment on attachment 8877403 [details] [diff] [review]
Part 4: Rewrite browser_fullscreen-window-open.js to use separated browser window runing fullscreen test, v1

(In reply to Edgar Chen [:edgar] from comment #43)
> I found in failure case, browser_fullscreen-window-open.js makes test window
> stay in fully occluded state. And browser_minimize.js fails in checking
> initial condition. If we skip browser_fullscreen-window-open.js test, we
> don't hit this failure any more [1]. It looks like
> browser_fullscreen-window-open.js sometimes puts test window in some werid
> state. Not sure what's happened, investigating ...

It looks like test window is not on the "primary" desktop sometimes after runing browser_fullscreen-window-open.js [1]. I tried adding waitForFocus() at the beginning of browser_minimize.js, but it doesn't help. So I rewrite the browser_fullscreen-window-open.js a bit to run the tests on a separated browser window, and I don't see the intermittent failure in browser_minimize.js any more [2]. mconley, could you help to review this change? Thank you.

[1] from the screenshot of failure case, http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/aa84f35f4d334bf5048377145e96f7db7bb44e99176a3b101474e915898d71026f95906f756c56196b43d17cd79a7a40955e85241e1cf13aea1ed25e6ad884fd
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fff3a98d2386ee2cfe7f8cf7fa9b8602a77df359&filter-tier=1&group_state=expanded

	Mike Conley (:mconley) (:⚙️)
	Comment 47 • 7 years ago

Comment on attachment 8875669 [details] [diff] [review]
Part 3: Set docShellIsActive to false when browser window is fully covered by another application, v2

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

This looks great to me, thanks Edgar. I have a few smaller questions / notes (and I hope you'll run this through ESLint before landing). Thanks!

::: browser/base/content/test/general/head.js
@@ +121,5 @@
>    }, 100);
>    var moveOn = function() { clearInterval(interval); nextTest(); };
>  }
>  
> +function promiseWaitForCondition(aConditionFn, retryTimes) {

I'm confused. Why do we need this? None of your patches in this bug seem to use this capability...

::: dom/tests/browser/browser_bug1236512.js
@@ +8,5 @@
> +const testPageURL = "http://mochi.test:8888/browser/dom/tests/browser/dummy.html";
> +
> +async function testContentVisibilityState(aIsHidden, aBrowser) {
> +  await ContentTask.spawn(aBrowser.selectedBrowser, aIsHidden, (aIsHidden) => {
> +    is(content.document.hidden, aIsHidden, "document.hidden");

Does this pass ESLint? I suspect ESLint will complain that you're shadowing the aIsHidden.

You can get around this by calling the argument something else inside the ContentTask function.

@@ +16,5 @@
> +}
> +
> +async function waitContentVisibilityChange(aIsHidden, aBrowser) {
> +  await ContentTask.spawn(aBrowser.selectedBrowser, aIsHidden,
> +                          async function (aIsHidden) {

Same as above, re: ESLint and shadowing.

@@ +34,5 @@
> +    });
> +  });
> +}
> +
> +add_task(async function () {

Can you please document in a block comment above this test what exactly is being tested?

I also suspect ESLint is going to flag you for function () instead of function(). Make sure to run ESLint on your patch before landing.

	Mike Conley (:mconley) (:⚙️)
	Comment 48 • 7 years ago

Comment on attachment 8877403 [details] [diff] [review]
Part 4: Rewrite browser_fullscreen-window-open.js to use separated browser window runing fullscreen test, v1

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

Seems like a reasonable solution to me. Probably a good idea to do a few retriggers on Try to make sure the problem is actually gone. Thanks!

	Edgar Chen [:edgar] Assignee
	Comment 49 • 7 years ago

(In reply to Mike Conley (:mconley) from comment #47)
> Comment on attachment 8875669 [details] [diff] [review]
> Part 3: Set docShellIsActive to false when browser window is fully covered
> by another application, v2
> 
> Review of attachment 8875669 [details] [diff] [review]:
> ::: browser/base/content/test/general/head.js
> @@ +121,5 @@
> > +function promiseWaitForCondition(aConditionFn, retryTimes) {
> 
> I'm confused. Why do we need this? None of your patches in this bug seem to
> use this capability...

My fault, this is for testing and I missed to remove it.

> ::: dom/tests/browser/browser_bug1236512.js
> @@ +8,5 @@
> > +const testPageURL = "http://mochi.test:8888/browser/dom/tests/browser/dummy.html";
> > +
> > +async function testContentVisibilityState(aIsHidden, aBrowser) {
> > +  await ContentTask.spawn(aBrowser.selectedBrowser, aIsHidden, (aIsHidden) => {
> > +    is(content.document.hidden, aIsHidden, "document.hidden");
> 
> Does this pass ESLint? I suspect ESLint will complain that you're shadowing
> the aIsHidden.
> 
> You can get around this by calling the argument something else inside the
> ContentTask function.

Will fix the shadowing error.

I run eslint locally. I got,

$ ./mach eslint dom/tests/browser/browser_bug1236512.js 
/Volumes/workspace/mercurial/mozilla-central/dom/tests/browser/browser_bug1236512.js
  None  warning  File ignored because of a matching ignore pattern. Use "--no-ignore" to override.  (eslint)

I think that is why ESLint test on try doesn't complain the error.

>  
> @@ +34,5 @@
> > +add_task(async function () {
> 
> Can you please document in a block comment above this test what exactly is
> being tested?

Sure, will do.

	Edgar Chen [:edgar] Assignee
	Comment 50 • 7 years ago

(In reply to Mike Conley (:mconley) from comment #48)
> Comment on attachment 8877403 [details] [diff] [review]
> Part 4: Rewrite browser_fullscreen-window-open.js to use separated browser
> window runing fullscreen test, v1
> 
> Review of attachment 8877403 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems like a reasonable solution to me. Probably a good idea to do a few
> retriggers on Try to make sure the problem is actually gone. Thanks!

I have retriggered 20+ on Try and the result looks good. Thanks for the review!!

	Edgar Chen [:edgar] Assignee
	Comment 51 • 7 years ago

Address review comment #47,
- Remove unnecessary changes in head.js
- Fix ESLint error
- Add comment about what exactly is being tested

	Edgar Chen [:edgar] Assignee
	Comment 52 • 7 years ago

s/runing/running/ in commit message.

	Edgar Chen [:edgar] Assignee
	Comment 53 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86670badcea942095188a2402a45dc433c522b96&filter-tier=1&group_state=expanded

	Edgar Chen [:edgar] Assignee
	Comment 54 • 7 years ago

(In reply to Edgar Chen [:edgar] from comment #53)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=86670badcea942095188a2402a45dc433c522b96&filter-
> tier=1&group_state=expanded

Rebase patches and run Try again, I got a new failure on a new-added fullscreen test, browser_panelUINotifications_fullscreen_noAutoHideToolbar.js [1]. The failure reason is same as 3) of Comment 33. Fixing ..

[1] that is introduced in bug 1369899.

	Edgar Chen [:edgar] Assignee
	Comment 55 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1cdf01c134b6a91de1e70fcf3ad38ea038ba6ed&filter-tier=1&group_state=expanded

	Edgar Chen [:edgar] Assignee
	Comment 56 • 7 years ago

Comment on attachment 8878400 [details] [diff] [review]
Part 5: Wait docshell has been activated again after entering fullscreen mode in panelUINotifications test, v1

In this bug, we add support to deactivate docshell when browser window is fully covered by anther window. And entering fullscreen mode will trigger docshell being set to non-activate and then set to activate back again. We need to wait until the docshell has been actiavted again after entering fullscreen mode, otherwise, the following dom fullscreen request might be denied. Gijs, could you help to review this change? Thank you.

	:Gijs (he/him)
	Comment 57 • 7 years ago

Comment on attachment 8878400 [details] [diff] [review]
Part 5: Wait docshell has been activated again after entering fullscreen mode in panelUINotifications test, v1

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

::: browser/components/customizableui/test/browser_panelUINotifications_fullscreen_noAutoHideToolbar.js
@@ +4,5 @@
>  
> +async function waitForDocshellActivated() {
> +  await ContentTask.spawn(gBrowser.selectedBrowser, null, async function() {
> +    if (!content.document.hidden &&
> +        content.document.visibilityState === "visible") {

Based on what you were saying this might race if it runs early, in that it could return when document.hidden will be false, but after that, the docshell will be deactivated and then reactivated. What guarantees that this doesn't happen? Is just waiting for the fullscreen event (as the only consumer does) enough? Then it might make more sense to make this dependency explicit by putting the waiting for the fullscreen event and this code in the same helper, rather than implying that this helper on its own makes sense.

More generally, why does this happen? Can we detect that we're entering full screen mode and just not set the document as hidden? That seems like it would be a better solution than sending all these effectively spurious events to the document...

I'm also a bit confused about the mix between checking hidden/visibilityState and "docshell activated", because docshell has a concept of "active docshell" (ie document.docShell.isActive), and it's not clear if this is what you're interested in (but you're not using it...) or if you mean something else (but you're describing it with similar terms).

	Edgar Chen [:edgar] Assignee
	Comment 58 • 7 years ago

(In reply to :Gijs from comment #57)
> Comment on attachment 8878400 [details] [diff] [review]
> Part 5: Wait docshell has been activated again after entering fullscreen
> mode in panelUINotifications test, v1
> 
> Review of attachment 8878400 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Then it might make more sense to make this dependency
> explicit by putting the waiting for the fullscreen event and this code in
> the same helper, rather than implying that this helper on its own makes
> sense.

Will put it and the waiting for the fullscreen event in the same helper.

> More generally, why does this happen? Can we detect that we're entering full
> screen mode and just not set the document as hidden? That seems like it
> would be a better solution than sending all these effectively spurious
> events to the document...

It could be possible that we are in fullscreen mode, but isFullyOccluded reports true (e.g. the focused desktop is not the fullscreen app). It is a bit hard to decide when to suppress the event.

> I'm also a bit confused about the mix between checking
> hidden/visibilityState and "docshell activated", because docshell has a
> concept of "active docshell" (ie document.docShell.isActive), and it's not
> clear if this is what you're interested in (but you're not using it...) or
> if you mean something else (but you're describing it with similar terms).

Setting docshell activated/deactivated will trigger visibility state changes to relevant state ("visible" or "hidden"). AFAIK, there is no such event notifying docshell is being activated, so I use "visibilitychange" event rather than polling the state. But yeah, I think it will make it clearer if I use "document.docShell.isActive" as condition for "visibilitychange" event check function.

	Edgar Chen [:edgar] Assignee
	Comment 59 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39cde0a3e0f2389680353e740bbd691abc181ca5&group_state=expanded&filter-tier=1

	Edgar Chen [:edgar] Assignee
	Comment 60 • 7 years ago

Comment on attachment 8879445 [details] [diff] [review]
Part 5: Wait docshell has been activated again after entering fullscreen mode in panelUINotifications test, v2

Address review comment #57,
- Put the waiting for docshell activated and the waiting for fullscreen event in the same helper.
- Use docShell.isActive as condition of check function for waiting "visibilitychange" event.

Gijs, could you help to take a look again? Thank you.

	Pulsebot
	Comment 61 • 7 years ago

Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/844d46609652
Part 1: Send "occlusionstatechange" custom event when occlusion state in Mac is changed; f=spohl; r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/62999c1db7ee
Part 2: Add isFullyOccluded attribute to ChromeWindow; r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9147bef9fc6
Part 3: Set docShellIsActive to false when browser window is fully covered by another application; r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f5c86248074
Part 4: Rewrite browser_fullscreen-window-open.js to use separated browser window running fullscreen test; r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/f10d7fb82ded
Part 5: Wait docshell has been activated again after entering fullscreen mode in panelUINotifications test; r=Gijs

	Carsten Book [:Tomcat]
	Comment 62 • 7 years ago

sorry had to backout for test failures in the test you added like https://treeherder.mozilla.org/logviewer.html#?job_id=108504603&repo=mozilla-inbound

	Pulsebot
	Comment 63 • 7 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6880c565b951
Backed out changeset f10d7fb82ded 
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9c6ed5c20db
Backed out changeset 6f5c86248074 
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b11fa8ae8e
Backed out changeset f9147bef9fc6 
https://hg.mozilla.org/integration/mozilla-inbound/rev/df2506045dfb
Backed out changeset 62999c1db7ee 
https://hg.mozilla.org/integration/mozilla-inbound/rev/53250ef2a84c
Backed out changeset 844d46609652 for test failures in own test

	Edgar Chen [:edgar] Assignee
	Comment 64 • 7 years ago

(In reply to Carsten Book [:Tomcat] from comment #62)
> sorry had to backout for test failures in the test you added like
> https://treeherder.mozilla.org/logviewer.html#?job_id=108504603&repo=mozilla-
> inbound

My fault, we don't need to wait docshell thing on non-Mac. Will fix and run full try again before push.

	Edgar Chen [:edgar] Assignee
	Comment 65 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1042a5c88b2fe7012e921d1c9d15552c6174a05&filter-tier=1&group_state=expanded

	Pulsebot
	Comment 66 • 7 years ago

Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/683bf7d44d1a
Part 1: Send "occlusionstatechange" custom event when occlusion state in Mac is changed; f=spohl; r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/c067db7f1d31
Part 2: Add isFullyOccluded attribute to ChromeWindow; r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/92a367cfee2f
Part 3: Set docShellIsActive to false when browser window is fully covered by another application; r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/31e29228f8ba
Part 4: Rewrite browser_fullscreen-window-open.js to use separated browser window running fullscreen test; r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/e573f37405ba
Part 5: Wait docshell has been activated again after entering fullscreen mode in panelUINotifications test; r=Gijs

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 67 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/683bf7d44d1a
https://hg.mozilla.org/mozilla-central/rev/c067db7f1d31
https://hg.mozilla.org/mozilla-central/rev/92a367cfee2f
https://hg.mozilla.org/mozilla-central/rev/31e29228f8ba
https://hg.mozilla.org/mozilla-central/rev/e573f37405ba

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 68 • 7 years ago

Thanks!

	Sebastian Zartner [:sebo]
	Comment 69 • 7 years ago

Added a note about this to https://developer.mozilla.org/en-US/docs/Web/API/Document/hidden#Browser_compatibility.

Sebastian

	Boris Zbarsky [:bzbarsky]
	Comment 70 • 7 years ago

I tried to adjust the note to reflect what was actually changed, but please double-check the wording?

	Sebastian Zartner [:sebo]
	Comment 71 • 7 years ago

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #70)
> I tried to adjust the note to reflect what was actually changed, but please
> double-check the wording?

I didn't realize it was just on Mac. Thanks for the correction, Boris! Looks good.

Sebastian

	Boris Zbarsky [:bzbarsky]
	Comment 72 • 7 years ago

> I didn't realize it was just on Mac. Thanks for the correction, Boris!

Looks like https://developer.mozilla.org/en-US/Firefox/Releases/56#DOM was wrong too...  I've fixed it as well.