Closed Bug 1236512 Opened 9 years ago Closed 7 years ago

document.hidden is not set to true when window is completely covered by another non-translucent application

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
42 Branch
Platform:
Unspecified
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: mmzeeman, Assigned: edgar)

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 8 obsolete files)

Part 1: Send "occlusionstatechange" custom event when occlusion state in Mac is changed, v2, f=spohl, r=mstange
10.17 KB, patch
edgar
: review+
edgar
: feedback+
Details | Diff | Splinter Review
Part 2: Add isFullyOccluded attribute to ChromeWindow, v2, r=bz
2.98 KB, patch
edgar
: review+
Details | Diff | Splinter Review
Part 3: Set docShellIsActive to false when browser window is fully covered by another application, v3, r=mconley
24.88 KB, patch
edgar
: review+
Details | Diff | Splinter Review
Part 4: Rewrite browser_fullscreen-window-open.js to use separated browser window running fullscreen test, v2, r=mconley
6.12 KB, patch
edgar
: review+
Details | Diff | Splinter Review
Part 5: Wait docshell has been activated again after entering fullscreen mode in panelUINotifications test, v3, r=Gijs
3.37 KB, patch
edgar
: review+
Details | Diff | Splinter Review
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.
Component: Untriaged → DOM
Product: Firefox → Core
> 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.
Summary: document.hidden does not work when window is completely hidden by another application → document.hidden is not set to true when window is completely covered by another non-translucent application
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?
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.
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
Comment 4 should be a separate bug, probably.
Status: UNCONFIRMED → NEW
Ever confirmed: true
See Also: → 646937
Whiteboard: [qf:p1]
Hi Edgar, please help from investigating this! Thank you!
Flags: needinfo?(echen)
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 | ---- | ------
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
Flags: needinfo?(echen) → needinfo?(jmathies)
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.
Flags: needinfo?(jmathies)
Karl, any ideas on GTK support?
Flags: needinfo?(karlt)
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.
Flags: needinfo?(karlt)
Assignee: nobody → echen
> 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.
> 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
Attached patch WIP Patch, v1 (obsolete) — — Splinter Review
For the reference, attach the WIP patch of the MAC implementation.
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/
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?
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
> 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?
Flags: needinfo?(jmathies)
(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?
Flags: needinfo?(jmathies) → needinfo?(spohl.mozilla.bugs)
(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
Flags: needinfo?(spohl.mozilla.bugs)
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.
Attachment #8871682 - Flags: review?(spohl.mozilla.bugs)
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).
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.
Flags: needinfo?(bzbarsky)
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.
Attachment #8871684 - Flags: review?(mconley)
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.
Attachment #8871682 - Flags: review?(spohl.mozilla.bugs)
Attachment #8871682 - Flags: review?(mstange)
Attachment #8871682 - Flags: feedback+
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.
Attachment #8871682 - Flags: review?(mstange) → review+
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!
Flags: needinfo?(bzbarsky)
Attachment #8871683 - Flags: review+
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.
Attachment #8871684 - Flags: review?(mconley) → review-
(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.
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.
Attachment #8871684 - Attachment is obsolete: true
(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
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.
Attachment #8875669 - Flags: review?(mconley)
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
Attachment #8877403 - Flags: review?(mconley)
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.
Attachment #8875669 - Flags: review?(mconley) → review+
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!
Attachment #8877403 - Flags: review?(mconley) → review+
(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.
(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!!
Address review comment #47, - Remove unnecessary changes in head.js - Fix ESLint error - Add comment about what exactly is being tested
Attachment #8875669 - Attachment is obsolete: true
(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.
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.
Attachment #8878400 - Flags: review?(gijskruitbosch+bugs)
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).
Attachment #8878400 - Flags: review?(gijskruitbosch+bugs)
(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.
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.
Attachment #8879445 - Flags: review?(gijskruitbosch+bugs)
Attachment #8879445 - Flags: review?(gijskruitbosch+bugs) → review+
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
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
Flags: needinfo?(echen)
(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.
Flags: needinfo?(echen)
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
I tried to adjust the note to reflect what was actually changed, but please double-check the wording?
OS: Unspecified → Mac OS X
(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
> 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.
No longer depends on: 1396743
Component: DOM → DOM: Core & HTML
See Also: → 1688997
Performance Impact: --- → P1
Whiteboard: [qf:p1]
See Also: → 1779557
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: