Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setWindowRect should be matched as a capability #1793

Open
gsnedders opened this issue Feb 14, 2024 · 7 comments
Open

setWindowRect should be matched as a capability #1793

gsnedders opened this issue Feb 14, 2024 · 7 comments

Comments

@gsnedders
Copy link
Member

c.f. https://matrix.to/#/!zvtoFTlOBeOwHrBlEK:matrix.org/$yumW_YPZ6kAsKlD3SB_muMP9ImAUoF893INwdNegTEw?via=matrix.org&via=mozilla.org&via=igalia.com

Nowhere does https://w3c.github.io/webdriver/#dfn-matching-capabilities match setWindowRect, so it effectively is ignored when passed as a capability (see: #1792).

However, per @whimboo:

In Firefox we match that capability and fail if the value is not appropriate

I have not tested what other implementations do.

@whimboo
Copy link
Contributor

whimboo commented Feb 15, 2024

Matching on our side takes place as:

https://searchfox.org/mozilla-central/rev/9e5b13e92b50c61e60a42bdc2cb2e5966ae28795/remote/shared/webdriver/Capabilities.sys.mjs#560-573

Note scenarios like window manipulation tests that want to position or resize browser windows. If you would run these tests on desktop it should basically work in nearly all the cases (exception would be Wayland) but on Android it should basically fail.

With matching clients can request that this feature is present to actually run all the tests on that given platform and if it's not supported the session will not be created.

wpt.fyi shows the difference between Firefox on Android and Chrome on Android. For Chrome an assertion only fails without any details that the command actually doesn't work, while for Firefox an unsupported operation is raised.

With matching the capability we would get a clear failure right in New Session for all browsers.

@OrKoN
Copy link
Contributor

OrKoN commented Feb 15, 2024

Chrome does not do matching for this capability and it is a return-only value: https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/chromedriver/session_commands.cc;l=189;drc=21802bc29a3f1f96a333bc52039dc56764a85524 (it will reject the property as invalid argument if it is in the capabilities request)

@whimboo
Copy link
Contributor

whimboo commented Feb 15, 2024

@shs96c @AutomatedTester @titusfortner @jimevans from the Selenium side of view how would you expect a remote end to behave when it doesn't support window manipulation? Should matching the setWindowRect capability fail if it's value in the requested capabilities is set to true? Or would you expect all the tests trying to manipulate the window to fail because the requested operation cannot be achieved?

@gsnedders
Copy link
Member Author

gsnedders commented Feb 15, 2024

It likewise is return-only in safaridriver.

(Despite being the OP, I was just doing what the action item from the meeting said; I don't have any particularly strong opinion on whether we should make it possible to match on it.)

@jimevans
Copy link
Contributor

Generally speaking, Selenium expects that capability to be return-only. I don’t know of anyone attempting to match on that capability in the Selenium world.

@whimboo
Copy link
Contributor

whimboo commented Apr 10, 2024

Given that matching is not expected should we close the issue? Or should we try to match all known and supported capabilities?

@titusfortner
Copy link
Contributor

I've never understood why it was there in the first place. 😂

My philosophy is that your test should already have all the information it needs to execute a deterministic test, and it shouldn't be toggling the action to take based on the returned capabilities, so it shouldn't need to be something that gets returned, let alone set, but that's my opinion.

That said, if the spec says these need to be matching, do we at least need to change the spec language?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants