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

Add support for "isLandscape" argument for "browsingContext.setViewport" command #771

Open
whimboo opened this issue Sep 13, 2024 · 7 comments
Labels
enhancement New feature or request module-browsingContext Browsing Context module needs-discussion Issues to be discussed by the working group

Comments

@whimboo
Copy link
Contributor

whimboo commented Sep 13, 2024

Both Puppeteer and Playwright support an isLandscape boolean parameter for the setViewport command, which is currently missing in WebDriver BiDi. We should add support for this parameter with the following behavior:

  • Default Behavior: By default, isLandscape is set to false, meaning the orientation will not change, even if the width is smaller than the height of the requested viewport.

  • Landscape Mode: If isLandscape is set to true, the provided dimensions will automatically swap when the width is smaller than the height (e.g., a setting of 800x1280 will result in a viewport of 1280x800).

  • Portrait Mode: To force portrait mode, isLandscape should be set to false, and the dimensions must be manually specified (e.g., 800x1280).

@whimboo whimboo added enhancement New feature or request module-browsingContext Browsing Context module needs-discussion Issues to be discussed by the working group labels Sep 13, 2024
@jgraham
Copy link
Member

jgraham commented Sep 13, 2024

I don't really understand the use case here. If you want to set a landscape display, just set a larger width than height. One can also imagine a tablet or a fold-out phone with a square display. Is there anything else that this is controlling?

@whimboo
Copy link
Contributor Author

whimboo commented Sep 13, 2024

We would have to let Puppeteer or Playwright folks speak up here. But I assume that this is helpful for the knownDevices class so that you don't have to define all the viewports twice, but just swap the width and height.

@jgraham
Copy link
Member

jgraham commented Sep 13, 2024

Yes, feedback from client authors would be very helpful. On the face of it I don't see why setting a boolean is much easier than swapping argument values, but maybe I'm missing some extra bit of state that this value controls.

@mxschmitt
Copy link

mxschmitt commented Sep 13, 2024

We use CDP's screenOrientation, like Puppeteer to emulate screen.orientation.angle, screen.orientation.type and window.orientation - see here and here for our tests. I think thats what it is mostly for from our side.

@OrKoN
Copy link
Contributor

OrKoN commented Sep 16, 2024

Puppeteer uses it to emulate https://w3c.github.io/screen-orientation/#dom-screen-orientation (tests) and uses the same method. A workaround for a one-time emulation can be probably done using a preload script but it might be also useful to emulate the screen orientation dynamically to test transitions from one mode to another after the page is loaded.

From the WebDriver BiDi spec perspective, we should probably closer follow the corresponding Web spec and what CDP does instead of only exposing isLandscape flag. Perhaps, we should even define the WebDriver API in https://w3c.github.io/screen-orientation/#dom-screen-orientation (although I am not sure if this feature might span multiple specifications in the future).

@jgraham
Copy link
Member

jgraham commented Sep 16, 2024

At least in gecko, the effective orientation seems to initially be set with just the height >= width logic I suggested: https://searchfox.org/mozilla-central/source/widget/Screen.cpp#19 and a quick test on my phone didn't give a value of angle other than 0 or 90 (i.e. portrait or landscape), although that may depend on phone settings or similar.

So I wouldn't be opposed to adding an angle parameter, but we might need some mechanism to indicate that certain values are unsupported; I'm not sure.

@jgraham
Copy link
Member

jgraham commented Sep 16, 2024

(alternatively I suppose just allowing setting the orientation type to one of the four supported values might be enough, and the angle could be calculated).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request module-browsingContext Browsing Context module needs-discussion Issues to be discussed by the working group
Projects
None yet
Development

No branches or pull requests

4 participants