1855028 - Implement "browser.setClientWindowState" command

Status: ASSIGNED

(Remote Protocol :: WebDriver BiDi, task, P3)

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The WebDriver BiDi spec is not done yet. As such we cannot implement it for M9. Lets push onto the backlog for now.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I accidentally took over the points when cloning the former bug. We need to discuss the complexity.

Comment 3

[Dan]

I know this is in not is not directly blocking implementing browser.getClientWindows, but shouldn't implementing this come first? browser.getClientWindows needs the getClientWindowState.

Comment 4

[Dan]

(In reply to Dan from comment #3)

I know this is in not is not directly blocking implementing browser.getClientWindows, but shouldn't implementing this come first? browser.getClientWindows needs the getClientWindowState.

Okay, I looked at the dependency graph again, and this issue is dependent on browser.getClientWindows. browser.getClientWindows does need to return the current state of the client window as part of its return object. So I am not sure if setting it should come first.

Comment 5

[Dan]

Attachment: Bug 1855028 - [webdriver-bidi] Implement "browser.setClientWindowState" command.

Comment 6

[Dan]

Attachment: Bug 1855028 - Migrated GeckoDriver window methods to Window Manager. r?whimboo

Comment 7

[Dan]

Attachment: Bug 1855028 - [wdspec] Implement "browser.setClientWindowState" command tests. r?whimboo

Comment 8

[Phabricator Automation]

Comment on attachment 9445393 [details]
Bug 1855028 - Migrated GeckoDriver window methods to Window Manager. r?whimboo

Revision D233020 was moved to bug 1942849. Setting attachment 9445393 [details] to obsolete.

Comment 9

[Dan]

Attachment: Bug 1855028 - [wdspec] Implement "browser.setClientWindowState" command tests. r?whimboo Differential Revision: https://phabricator.services.mozilla.com/D233065

Comment 10

[Julian Descottes [:jdescottes]]

No update for > 1 month, maybe should be moved to backlog for now.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry that is my fault. I didn't notice that the revision, that we are waiting for, was updated. I'll do the review.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I pushed a try build with the latest modifications but I still see quite a number of test failures across platforms:
https://treeherder.mozilla.org/jobs?repo=try&revision=84b8c7f0343094540cbd5aa7b45abcc1d69411ef

Here some things to ensure:

1. We need metadata files for Android given that all these tests cannot be run and need to be disabled for that platform
2. Transitions between states seem to be fragile - especially minimized to fullscreen seems to fail a lot
3. We probably want to run each test in a new window that we can close at the end of the test to not affect the following tests
4. We need special handling of assertions for Wayland given that we cannot move the window

Dan, are you still interested to continue working on this bug? I'll make sure to more promptly reply, and in case I miss something you can always ping me on Matrix. Please let us know. Thanks.

Comment 13

[Dan]

Hi Henrik. Yes, I am still interested in working on the bug.

Comment 14

[Julian Descottes [:jdescottes]]

Dan: I'm removing it from the milestone because we don't have to track contributions as milestone bugs, but this has no impact on the priority. Feel free to keep working on this and thanks again for your help on this bug!

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hi Dan, I want to check if you may have an update for us regarding this implementation. If you don't have the time please give us a note and someone can take over the remaining work. Thanks!

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hi Dan. I assume that you may not find the time to update this patch. Are you ok when one of us is taking over the remaining work? Thanks for the contribution so far!

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dan, I think that we can pick-up the remaining work to finally get this API landed. For that work we expect 3 points and we could do it in the next milestone starting in October.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Liam will take a look at finishing up the patches. Thanks!

Comment 19

[Liam DeBeasi]

Hey there,

I'm working on getting the tests for this passing. I'm using the work from the previous contributor as a starting point since that has already received reviews from you. Can you clarify the intent behind the stress.py tests? I'd like to better understand what part of the implementation this is testing.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Great and that's a good starting point!

Regarding the stress tests.... in the past we had issues with the stability of the window resizing methods and especially when you change between different modes and sizes side effects were shown. As such we are trying to stress-test Firefox to see that the way how we implemented the related API works as expected. The tests were already present for the WebDriver classic tests and were just ported over.

Comment 21

[Liam DeBeasi]

Attachment: Bug 1855028 - [webdriver-bidi] Add the "browser.setClientWindowState" command.

Comment 22

[Liam DeBeasi]

Attachment: Bug 1855028 - [wdspec] Implement browser.setClientWindowState command tests r=whimboo

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We cannot directly transition from the minimized to the fullscreen state and need to restore the window first if minimized. There is https://github.com/w3c/webdriver-bidi/issues/1035 on file for it.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I've created https://github.com/w3c/webdriver-bidi/pull/1063 to fix the transition issue.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As discussed with Liam I'm going to fix the remaining issue so that we can get this API landed soon. For the remaining work we agreed on 2 points.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I just noticed that the WebDriver tests need to be updated so that we use Javascript to get the actual window state. Right now we only rely on the values as returned by the command, which doesn't mean it's true.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #26)

I just noticed that the WebDriver tests need to be updated so that we use Javascript to get the actual window state. Right now we only rely on the values as returned by the command, which doesn't mean it's true.

Actually there’s no way to retrieve the actual OS-level window state via a web API, so we can’t add those checks; at least not in the public tests. However, I can add Mozilla-specific tests that obtain the window state via chrome JS.

Otherwise I noticed a discrepancy in the current BiDi spec between OS-level fullscreen and document fullscreen. Right now, BiDi is using document fullscreen, which is not equivalent to the OS-level fullscreen window state. Since this command is intended to modify the OS-level window state, I’m not sure why the document fullscreen API was chosen. It seems like this should be changed.

Lets get this discussed in our next triage meeting.