Closed Bug 1189749 Opened 9 years ago Closed 7 years ago

fullScreen has not been implemented

Categories

(Testing :: Marionette Client and Harness, defect)

Product:
Testing
Component:
Marionette Client and Harness
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: automatedtester, Assigned: automatedtester)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: pi-marionette-client, pi-marionette-server, pi-marionette-spec, Whiteboard: [marionette=1.0])

Attachments

(1 file)

Bug 1189749: Implement full screen support in Marionette
58 bytes, text/x-review-board-request
ato
: review+
Details 
This is not currently implemented and is in the specification
Assignee: nobody → dburns
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)
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 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)
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)
(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)
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?
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
      });
   };
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.
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 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 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 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()
Depends on: 1361987
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 on attachment 8770201 [details]
Bug 1189749: Implement full screen support in Marionette

https://reviewboard.mozilla.org/r/63726/#review139630

This looks much better FWIW.
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 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 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 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-
(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 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 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 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+
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
Pushed by dburns@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7fd514958035
Implement full screen support in Marionette r=ato
https://hg.mozilla.org/mozilla-central/rev/7fd514958035
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Testing → Remote Protocol

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.

Attachment

General

Creator:
Created:
Updated:
Size: