Closed Bug 1391691 Opened 7 years ago Closed 7 years ago

Make {Fullscreen,Maximize,Minimize} Window commands idempotent

Categories

(Remote Protocol :: Marionette, enhancement)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(10 files)

Bug 1391691 - Rename GeckoDriver#fullscreen to fullscreenWindow.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1391691 - Make WebDriver:MinimizeWindow idempotent.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1391691 - Make WebDriver:MaximizeWindow idempotent.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1391691 - Make WebDriver:FullscreenWindow idempotent.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1391691 - Restore window state using Set Window Rect.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1391691 - Add wdclient documentation for window manipulation.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1391691 - Fix undefined variable error with client.Session#position().
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1391691 - Bringing window out of fullscreen does not restore x/y position.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1391691 - Skip needless assertion.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1391691 - Export WindowState properly.
59 bytes, text/x-review-board-request
automatedtester
: review+
Details 
The WebDriver specification has changed (again) and the {Fullscreen,Maximize,Minimize} Window commands should now be idempotent (should be a no-op when called a second time).
Assignee: nobody → ato
Blocks: webdriver
Status: NEW → ASSIGNED
Depends on: 1391661
Depends on: 1388036
Comment on attachment 8898910 [details]
Bug 1391691 - Make WebDriver:FullscreenWindow idempotent.

https://reviewboard.mozilla.org/r/170264/#review175760

::: testing/marionette/driver.js:3137
(Diff revision 1)
>  GeckoDriver.prototype.fullscreenWindow = async function(cmd, resp) {
>    assert.firefox();
>    const win = assert.window(this.getCurrentWindow());
>    assert.noUserPrompt(this.dialog);
>  
> +  if (!win.fullScreen) {

Why aren't you using WindowState here?
Comment on attachment 8898914 [details]
Bug 1391691 - Bringing window out of fullscreen does not restore x/y position.

https://reviewboard.mozilla.org/r/170272/#review175762

::: testing/web-platform/tests/webdriver/tests/set_window_rect.py:94
(Diff revision 1)
> -    original = session.window.rect
> -
>      # step 10
>      session.window.fullscreen()
>      assert session.window.state == "fullscreen"
>      response = set_window_rect(session, {"width": 400, "height": 400})

That's bad. But we allow to pass null params to `set_window_rect` (see step 4 and 5) which should keep/restore to the original values?
Comment on attachment 8898914 [details]
Bug 1391691 - Bringing window out of fullscreen does not restore x/y position.

https://reviewboard.mozilla.org/r/170272/#review175762

> That's bad. But we allow to pass null params to `set_window_rect` (see step 4 and 5) which should keep/restore to the original values?

Well as later steps show it's invalid to pass `null` values. It means we never can test that leaving maximize, minimize, or fullscreen state will restore the original values.
Comment on attachment 8898914 [details]
Bug 1391691 - Bringing window out of fullscreen does not restore x/y position.

https://reviewboard.mozilla.org/r/170272/#review175762

> Well as later steps show it's invalid to pass `null` values. It means we never can test that leaving maximize, minimize, or fullscreen state will restore the original values.

There is nothing in the HTML specification that says fully exiting fullscreen should restore the window to the same X/Y position it was in before.  This is a trait of the WM and outside the scope of what we can assert.
Comment on attachment 8898907 [details]
Bug 1391691 - Rename GeckoDriver#fullscreen to fullscreenWindow.

https://reviewboard.mozilla.org/r/170258/#review175850
Attachment #8898907 - Flags: review?(dburns) → review+
Comment on attachment 8898908 [details]
Bug 1391691 - Make WebDriver:MinimizeWindow idempotent.

https://reviewboard.mozilla.org/r/170260/#review175862
Attachment #8898908 - Flags: review?(dburns) → review+
Comment on attachment 8898909 [details]
Bug 1391691 - Make WebDriver:MaximizeWindow idempotent.

https://reviewboard.mozilla.org/r/170262/#review175866
Attachment #8898909 - Flags: review?(dburns) → review+
Comment on attachment 8898910 [details]
Bug 1391691 - Make WebDriver:FullscreenWindow idempotent.

https://reviewboard.mozilla.org/r/170264/#review175874

As with whimboo's comment, use window state to be consistent with the other commands
Attachment #8898910 - Flags: review?(dburns) → review-
Comment on attachment 8898911 [details]
Bug 1391691 - Restore window state using Set Window Rect.

https://reviewboard.mozilla.org/r/170266/#review175876
Attachment #8898911 - Flags: review?(dburns) → review+
Comment on attachment 8898912 [details]
Bug 1391691 - Add wdclient documentation for window manipulation.

https://reviewboard.mozilla.org/r/170268/#review175878
Attachment #8898912 - Flags: review?(dburns) → review+
Comment on attachment 8898913 [details]
Bug 1391691 - Fix undefined variable error with client.Session#position().

https://reviewboard.mozilla.org/r/170270/#review175880
Attachment #8898913 - Flags: review?(dburns) → review+
Comment on attachment 8898910 [details]
Bug 1391691 - Make WebDriver:FullscreenWindow idempotent.

https://reviewboard.mozilla.org/r/170264/#review175874

The other commands aren’t using WindowState, but we can do even if
this will get fixed implicitly as part of bug 1311041.
Comment on attachment 8898910 [details]
Bug 1391691 - Make WebDriver:FullscreenWindow idempotent.

https://reviewboard.mozilla.org/r/170264/#review175884
Attachment #8898910 - Flags: review?(dburns) → review+
Comment on attachment 8898915 [details]
Bug 1391691 - Skip needless assertion.

https://reviewboard.mozilla.org/r/170274/#review175886

::: testing/web-platform/tests/webdriver/tests/contexts/maximize_window.py:60
(Diff revision 2)
>  
>      assert before_size != session.window.size
>      assert session.window.state == "maximized"
>  
>  
> +def test_maximize_twice_is_indempotent(session):

I think this test should be in the `maximise() is idempotent` change. also you have misspelled the test. `s/indempotent/idempotent/`
Attachment #8898915 - Flags: review?(dburns) → review+
Comment on attachment 8898914 [details]
Bug 1391691 - Bringing window out of fullscreen does not restore x/y position.

https://reviewboard.mozilla.org/r/170272/#review175890
Attachment #8898914 - Flags: review?(dburns) → review+
Comment on attachment 8898916 [details]
Bug 1391691 - Export WindowState properly.

https://reviewboard.mozilla.org/r/170276/#review175894
Attachment #8898916 - Flags: review?(dburns) → review+
Comment on attachment 8898915 [details]
Bug 1391691 - Skip needless assertion.

https://reviewboard.mozilla.org/r/170274/#review175886

> I think this test should be in the `maximise() is idempotent` change. also you have misspelled the test. `s/indempotent/idempotent/`

You’re right.  I appear to have messed something up when I rebased
and reordered the patches in my last pass.  Will address this.
Comment on attachment 8898914 [details]
Bug 1391691 - Bringing window out of fullscreen does not restore x/y position.

https://reviewboard.mozilla.org/r/170272/#review175762

> There is nothing in the HTML specification that says fully exiting fullscreen should restore the window to the same X/Y position it was in before.  This is a trait of the WM and outside the scope of what we can assert.

Not sure what we need here, the HTML or fullscreen API spec: https://fullscreen.spec.whatwg.org/#exit-fullscreen? It has at least a resize option.

Can you please clarify if that is a webdriver spec issue or not? In any case this can be handled separately.
Comment on attachment 8898914 [details]
Bug 1391691 - Bringing window out of fullscreen does not restore x/y position.

https://reviewboard.mozilla.org/r/170272/#review175762

> Not sure what we need here, the HTML or fullscreen API spec: https://fullscreen.spec.whatwg.org/#exit-fullscreen? It has at least a resize option.
> 
> Can you please clarify if that is a webdriver spec issue or not? In any case this can be handled separately.

We talked about this on IRC, and the WebDriver specification no
longer provides primitives for restoring the window to its previous
state because the Fullscreen Window command is now idempotent.

In any case, restoring a window from fullscreen or iconified state
does not necessarily mean the window’s X/Y position will be the
same as it was before it went into said state because this behaviour
depends on the window manager.  It may incidentally be restored to
the same position, but this is only incidental.

Furthermore, this test uses Set Window Rect to exit fullscreen with
a width/height parameter, which definitely means we should not be
asserting the returned rect’s X/Y fields.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17aeed1f6454
Export WindowState properly. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/6c48daaad075
Rename GeckoDriver#fullscreen to fullscreenWindow. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/649b0e761c87
Add wdclient documentation for window manipulation. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/6306abc0b5e9
Fix undefined variable error with client.Session#position(). r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/fd88b612ac2e
Restore window state using Set Window Rect. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/4653134d01ef
Bringing window out of fullscreen does not restore x/y position. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/5bddbd90ec7c
Skip needless assertion. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/1fd98ace1473
Make WebDriver:MinimizeWindow idempotent. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/46f82e1e2cde
Make WebDriver:MaximizeWindow idempotent. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/666ac679317e
Make WebDriver:FullscreenWindow idempotent. r=automatedtester
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/28b678c9009d
Backed out 10 changesets for wpt failures a=backout
(In reply to Wes Kocher (:KWierso) from comment #45)

> I had to back these out for failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=124591121&repo=autoland

This should’ve been addressed by bug 1388036, but I guess with
Autoland we don’t test changes stacked on top of eachother, but on
their own separate branches rebased against inbound.

So what we have to do here is simply wait for that change to land,
then re-land this.
Flags: needinfo?(ato)
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23b08fa8fad0
Export WindowState properly. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/821016a04a50
Rename GeckoDriver#fullscreen to fullscreenWindow. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/83080d660f56
Add wdclient documentation for window manipulation. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/5e07d579282d
Fix undefined variable error with client.Session#position(). r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/c36b163dd034
Restore window state using Set Window Rect. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/2286cf7d577a
Bringing window out of fullscreen does not restore x/y position. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/9013ea0fc942
Skip needless assertion. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/ee876a24b7e4
Make WebDriver:MinimizeWindow idempotent. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/ef813a1a750f
Make WebDriver:MaximizeWindow idempotent. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/2f25a6f73753
Make WebDriver:FullscreenWindow idempotent. r=automatedtester
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: