Closed
Bug 1189749
Opened 9 years ago
Closed 7 years ago
fullScreen has not been implemented
Categories
(Testing :: Marionette Client and Harness, defect)
Testing
Marionette Client and Harness
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: automatedtester, Assigned: automatedtester)
References
(Blocks 1 open bug, )
Details
(Keywords: pi-marionette-client, pi-marionette-server, pi-marionette-spec, Whiteboard: [marionette=1.0])
Attachments
(1 file)
This is not currently implemented and is in the specification
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dburns
Assignee | ||
Comment 2•8 years ago
|
||
This implements the Full Screen method described in the WebDriver specification (http://w3c.github.io/webdriver/webdriver-spec.html#fullscreen-window) as well as tests for it. Review commit: https://reviewboard.mozilla.org/r/63726/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63726/
Attachment #8770201 -
Flags: review?(ato)
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63726/diff/1-2/
Comment 4•8 years ago
|
||
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review60854 ::: testing/marionette/client/marionette_driver/marionette.py:1947 (Diff revision 2) > """ > return self._send_message("maximizeWindow") > + > + def fullscreen(self): > + """ Resize the browser window to go into fullscreen mode. The action > + should be equivalent to the user doing "View > Enter Full Screen" s/should/is/ ::: testing/marionette/driver.js:2516 (Diff revision 2) > > /** > + * Sets the user agent content window to full screen as if the user > + * had done "View > Enter Full Screen" > + */ > +GeckoDriver.prototype.fullScreen = function (cmd, resp) { Use "fullscreen" (all lower case) to match platform API name ("requestFullscreen"). ::: testing/marionette/driver.js:2520 (Diff revision 2) > + */ > +GeckoDriver.prototype.fullScreen = function (cmd, resp) { > + if (this.appName != "Firefox") { > + throw new UnsupportedOperationError(); > + } > + FullScreen.init(); What does this do? ::: testing/marionette/driver.js:2521 (Diff revision 2) > + // Full screen is done at the content level. > + //this.listener.fullScreen(); Debug comment? ::: testing/marionette/harness/marionette/tests/unit/test_window_management.py:107 (Diff revision 2) > + test_html = self.marionette.absolute_url("test_windows.html") > + self.marionette.navigate(test_html) Remove: We don’t need a specific document for this test. ::: testing/marionette/harness/marionette/tests/unit/test_window_management.py:110 (Diff revision 2) > + Wait(self.marionette).until( > + lambda m: m.execute_script("return document.fullscreenElement") is not None) I would make an argument that because WebDriver tries to provide a synchronous API, we should do this wait in the server. ::: testing/marionette/harness/marionette/tests/unit/test_window_management.py:112 (Diff revision 2) > + self.assertTrue(size.has_key('width')) > + self.assertTrue(size.has_key('height')) Assert that returned size is greater than previous size. ::: testing/marionette/harness/marionette/tests/unit/test_window_management.py:115 (Diff revision 2) > + # Clean up just in case > + self.marionette.execute_script("document.exitFullscreen()") As discussed should make _Set Window Size_ bring us out of fullscreen. ::: testing/marionette/listener.js:304 (Diff revision 2) > addMessageListenerId("Marionette:getScreenshotHash", getScreenshotHashFn); > addMessageListenerId("Marionette:addCookie", addCookieFn); > addMessageListenerId("Marionette:getCookies", getCookiesFn); > addMessageListenerId("Marionette:deleteAllCookies", deleteAllCookiesFn); > addMessageListenerId("Marionette:deleteCookie", deleteCookieFn); > + addMessageListenerId("Marionette:fullScreen", fullScreenFn); Let’s use lower-case "fullscreen" to match the platform API name. ::: testing/marionette/listener.js:1511 (Diff revision 2) > + // FullScreen can happen at any element, since this is a generic command > + // we should set the body to fullscreen. > + curContainer.frame.document.body.requestFullscreen(); In that case we should use `document.documentElement` since `document.body` does not exist on all document types (e.g. XHTML or SVG).
Attachment #8770201 -
Flags: review?(ato)
Comment 5•8 years ago
|
||
Why do we want to call this "fullscreen"? When I check the current list of commands I see "takeScreenshot" bug not "takeElementScreenshot". takeScreenshot is able to take both a fullscreen capture and one for an element as root. Shouldn't we add a new method for takeElementScreenshot?
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #5) > Why do we want to call this "fullscreen"? When I check the current list of > commands I see "takeScreenshot" bug not "takeElementScreenshot". > takeScreenshot is able to take both a fullscreen capture and one for an > element as root. Shouldn't we add a new method for takeElementScreenshot? This bug has nothing to do with screenshots. This is about making the browser go fullscreen
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
Comment 7•8 years ago
|
||
Sorry, missread the summary while searching for screen shot and the full option. David, are you going to finish up this bug, or should someone else finish it?
Comment 8•8 years ago
|
||
I think this patch only requires minor fixups. The important bit remaining is making it synchronous. I would suggest implementing it using a promise: GeckoDriver.prototype.fullscreen = function (cmd, resp) { return new Promise(resolve => { addEventListener("fullscreenchange", resolve, {once: true}); // trigger full screen here }); };
Comment 9•8 years ago
|
||
What happens with the event listener in the above case if triggering the full screen mode fails? I think it will stay around until the next time, and then we have two of them registered, which fire once. I think we need to handle a failing case here to also not introduce a hang in Marionette.
Comment 10•8 years ago
|
||
A fullscreenchange event occurs even when erroring, and we can presumably check if the screen is in fullscreen mode when this triggers. Based on whether it succeeds or not, we can reject the promise by throwing an error.
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review101792 ::: testing/marionette/driver.js:2520 (Diff revision 2) > + */ > +GeckoDriver.prototype.fullScreen = function (cmd, resp) { > + if (this.appName != "Firefox") { > + throw new UnsupportedOperationError(); > + } > + FullScreen.init(); This also needs a check for a valid window like it get done on bug 1322383.
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review137648 ::: testing/marionette/driver.js:2891 (Diff revision 3) > + yield wait.until((resolve, reject) => { > + if ((win.screenX != orig.screenX || win.screenY != orig.screenY) || > + (win.outerWidth != orig.width || win.outerHeight != orig.height)) { > + resolve(); > + } else { > + reject(); > + } > + }) I think you ought to be able to use ChromeWindow.windowState to determine if the window is in fullscreen state: https://developer.mozilla.org/en-US/docs/Web/API/Window/windowState You should be be able to just follow the recipe I recently landed for GeckoDriver#maximizeWindow: > GeckoDriver.prototype.maximizeWindow = function* (cmd, resp) { > assert.firefox(); > const win = assert.window(this.getCurrentWindow()); > assert.noUserPrompt(this.dialog); > > yield new Promise(resolve => { > win.addEventListener("resize", resolve, {once: true}); > > if (win.windowState == win.STATE_MAXIMIZED) { > win.restore(); > } else { > win.maximize(); > } > }); > > return { > x: win.screenX, > y: win.screenY, > width: win.outerWidth, > height: win.outerHeight, > }; > }; This avoids polling, which is always a good thing. It’s not clear to me whether you actually need to request fullscreen from an element in web content. Is there no way to request fullscreen from chrome alone? ::: testing/marionette/driver.js:2900 (Diff revision 3) > + } else { > + reject(); > + } > + }) > + > + return this.getWindowRect(cmd, resp); Make sure you return the literal dictionary here so we don’t repeat any of the other steps of getWindowRect. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_management.py:120 (Diff revision 3) > def tearDown(self): > self.close_all_windows() > super(TestNoSuchWindowChrome, self).tearDown() > > > + Superfluous newline. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_management.py:171 (Diff revision 3) > + def test_should_be_able_to_enter_full_screen(self): > + original_size = self.marionette.window_size > + > + size = self.marionette.fullscreen() > + > + self.assertGreater(size['width'], original_size['width']) > + self.assertGreater(size['height'], original_size['height']) > + > + # Clean up just in case > + self.marionette.set_window_size(original_size['width'], original_size['height']) To match test_window_rect.py and test_window_maximize.py, I would put this in a new test_window_fullscreen.py file. I also don’t think the assertions are great. Take a look at test_window_maximize.py for what you need to test. ::: testing/marionette/listener.js:426 (Diff revision 3) > var actionChainFn = dispatch(actionChain); > var multiActionFn = dispatch(multiAction); > var addCookieFn = dispatch(addCookie); > var deleteCookieFn = dispatch(deleteCookie); > var deleteAllCookiesFn = dispatch(deleteAllCookies); > +//var fullscreenFn = dispatch(fullscreen); Please make this use dispatch. ::: testing/marionette/listener.js:1674 (Diff revision 3) > + // fullscreen can happen at any element, since this is a generic command > + // we should set the body to fullscreen. > + curContainer.frame.document.onfullscreenchange = function (event) { > + sendOk(command_id); > + } > + curContainer.frame.document.documentElement.requestFullscreen(); This function should use dispatch and should use a promise that is resolved on onfullscreenchange firing. You also need to attach the DOM event using addEventListener, and you should be using the {once: true} option so it doesn’t linger around.
Attachment #8770201 -
Flags: review?(ato) → review-
Comment 14•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review137648 > I think you ought to be able to use ChromeWindow.windowState to determine if the window is in fullscreen state: https://developer.mozilla.org/en-US/docs/Web/API/Window/windowState > > You should be be able to just follow the recipe I recently landed for GeckoDriver#maximizeWindow: > > > GeckoDriver.prototype.maximizeWindow = function* (cmd, resp) { > > assert.firefox(); > > const win = assert.window(this.getCurrentWindow()); > > assert.noUserPrompt(this.dialog); > > > > yield new Promise(resolve => { > > win.addEventListener("resize", resolve, {once: true}); > > > > if (win.windowState == win.STATE_MAXIMIZED) { > > win.restore(); > > } else { > > win.maximize(); > > } > > }); > > > > return { > > x: win.screenX, > > y: win.screenY, > > width: win.outerWidth, > > height: win.outerHeight, > > }; > > }; > > This avoids polling, which is always a good thing. > > It’s not clear to me whether you actually need to request fullscreen from an element in web content. Is there no way to request fullscreen from chrome alone? > It’s not clear to me whether you actually need to request fullscreen > from an element in web content. Is there no way to request fullscreen > from chrome alone? Yes, so I checked in the Browser Toolbox, and there is a requestFullscreen function available on the ChromeWindow: > window.document.documentElement.requestFullscreen()
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review139628 ::: testing/marionette/client/marionette_driver/marionette.py:2223 (Diff revision 6) > + """ Resize the browser window to go into fullscreen mode. The action > + is equivalent to the user doing "View > Enter Full Screen" This doesn’t explain what the code is doing if the window is already fullscreened. ::: testing/marionette/client/marionette_driver/marionette.py:2228 (Diff revision 6) > + """ Resize the browser window to go into fullscreen mode. The action > + is equivalent to the user doing "View > Enter Full Screen" > + > + :returns: dictionary representation of current window width and height > + """ > + return self._send_message("fullscreen")['value'] See comment below, but ['value'] should be dropped. ::: testing/marionette/driver.js:2894 (Diff revision 6) > * @throws {NoSuchWindowError} > * Top-level browsing context has been discarded. > * @throws {UnexpectedAlertOpenError} > * A modal dialog is open, blocking this operation. > */ > -GeckoDriver.prototype.maximizeWindow = function* (cmd, resp) { > +GeckoDriver.prototype.maximizeWindow = function (cmd, resp) { This function uses a yield, so it must be a generator function. ::: testing/marionette/driver.js:2918 (Diff revision 6) > + * sets the user agent content window to full screen as if the user > + * had done "View > Enter Full Screen Correct typos and explain what happens if window is already fullscreened. ::: testing/marionette/driver.js:2948 (Diff revision 6) > + resp.body.value = { > + x: win.screenX, > + y: win.screenY, > + width: win.outerWidth, > + height: win.outerHeight, > + }; JSON Objects are returned unwrapped in the Marionette protocol, so this shouldn’t do it differently. You can fix this by assigning the object literal to resp.body, dropping "value". ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_fullscreen.py:1 (Diff revision 6) > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > +from marionette_harness import MarionetteTestCase Insert blank line after comment. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_fullscreen.py:17 (Diff revision 6) > + # ensure window is not maximized > + self.marionette.set_window_rect( > + width=self.max["width"] - 100, height=self.max["height"] - 100) > + actual = self.marionette.window_rect > + self.assertNotEqual(actual["width"], self.max["width"]) > + self.assertNotEqual(actual["height"], self.max["height"]) Whether window is maximised or not doesn’t matter in this case. We need to ensure it’s not fullscreened though, presumably. You could probably achieve this using a chrome execute_script call to exitFullscreen. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_fullscreen.py:32 (Diff revision 6) > + fullscreen = self.marionette.execute_script(""" > + return window.fullScreen;""", sandbox=None) > + if fullscreen: > + self.marionette.fullscreen() > + > + def assert_window_fullscreen(self, actual, delta=None): Unused keyword argument ‘delta’. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_fullscreen.py:33 (Diff revision 6) > + self.assertGreater(actual["width"], self.max["width"]) > + self.assertGreater(actual["height"], self.max["height"]) Not sure how this behaves across different platforms, but wouldn’t availHeight/availWidth be equivalent to fullscreen? Not raising an issue about this, as I assume you’ve checked this. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_fullscreen.py:50 (Diff revision 6) > + def test_fullscreen(self): > + rect = self.marionette.fullscreen() > + self.assert_window_rect(rect) > + size = self.marionette.window_size > + self.assertEqual(size, rect) You should probably check that window.fullScreen is set here. Maybe that assertion should be part of assert_window_fullscreen? ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_management.py:120 (Diff revision 6) > def tearDown(self): > self.close_all_windows() > super(TestNoSuchWindowChrome, self).tearDown() > > > + Superfluous newline added. ::: testing/marionette/listener.js:507 (Diff revision 6) > var executeFn = dispatch(execute); > var executeInSandboxFn = dispatch(executeInSandbox); > var executeSimpleTestFn = dispatch(executeSimpleTest); > var sendKeysToElementFn = dispatch(sendKeysToElement); > > + Another newline.
Attachment #8770201 -
Flags: review?(ato) → review-
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review139630 This looks much better FWIW.
Assignee | ||
Comment 20•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review139628 > This function uses a yield, so it must be a generator function. This is fixed with my generator patch, thought I had reverted this already :/ > Not sure how this behaves across different platforms, but wouldn’t availHeight/availWidth be equivalent to fullscreen? > > Not raising an issue about this, as I assume you’ve checked this. I have checked it. `availHeight` and `availWidth` return the max size the window (not browser chrome) can return. We are returning `outerHeight` and since fullscreen is now clearing a lot of the browser chrome to make it fullscreen it goes larger than `availheight` and `availWidth`.
Comment hidden (mozreview-request) |
Comment 22•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review139628 > This is fixed with my generator patch, thought I had reverted this already :/ This doesn’t appear to have been fixed in your patch though. You probably have to revert this change, then rebase this.
Comment 23•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review139628 > I have checked it. `availHeight` and `availWidth` return the max size the window (not browser chrome) can return. We are returning `outerHeight` and since fullscreen is now clearing a lot of the browser chrome to make it fullscreen it goes larger than `availheight` and `availWidth`. > I have checked it. availHeight and availWidth return the max size the > window (not browser chrome) can return. We are returning outerHeight > and since fullscreen is now clearing a lot of the browser chrome to > make it fullscreen it goes larger than availheight and availWidth. Sounds good.
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review140006 ::: testing/marionette/driver.js:2919 (Diff revision 7) > }; > }; > > /** > + * Synchronously sets the user agent window to full screen as if the user > + * had done "View > Enter Full Screen", or restores it if it is already Nit: Two spaces before “or”. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_management.py:120 (Diff revision 7) > def tearDown(self): > self.close_all_windows() > super(TestNoSuchWindowChrome, self).tearDown() > > > + Blank line. ::: testing/marionette/listener.js:524 (Diff revision 7) > var executeFn = dispatch(execute); > var executeInSandboxFn = dispatch(executeInSandbox); > var executeSimpleTestFn = dispatch(executeSimpleTest); > var sendKeysToElementFn = dispatch(sendKeysToElement); > > + Blank line.
Attachment #8770201 -
Flags: review?(ato) → review-
Comment 25•7 years ago
|
||
There also appears to be many test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d17fcb70e2c&selectedJob=96540260
Assignee | ||
Comment 26•7 years ago
|
||
(In reply to Andreas Tolfsen ‹:ato› from comment #25) > There also appears to be many test failures: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=0d17fcb70e2c&selectedJob=96540260 these are because of the generator patches not applied, I need a new try
Comment hidden (mozreview-request) |
Comment 28•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review139628 > This doesn’t appear to have been fixed in your patch though. You probably have to revert this change, then rebase this. When I look at the complete diff range in https://reviewboard.mozilla.org/r/63726/diff/, this is still being turned from a generator function to a regular function as part of this patch.
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review140104
Attachment #8770201 -
Flags: review?(ato) → review-
Comment hidden (mozreview-request) |
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review140160 Looks good now. Push when confident.
Attachment #8770201 -
Flags: review?(ato) → review+
Assignee | ||
Comment 32•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8770201 [details] Bug 1189749: Implement full screen support in Marionette https://reviewboard.mozilla.org/r/63726/#review139628 > When I look at the complete diff range in https://reviewboard.mozilla.org/r/63726/diff/, this is still being turned from a generator function to a regular function as part of this patch. I have removed it, committed it and added it back and committed it back
Comment 33•7 years ago
|
||
Pushed by dburns@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7fd514958035 Implement full screen support in Marionette r=ato
Comment 34•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7fd514958035
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•1 year ago
|
Product: Testing → Remote Protocol
Comment 35•1 year ago
|
||
Moving bugs for Marionette client due to component changes.
Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in
before you can comment on or make changes to this bug.
Description
•