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

Prevent hidden documents from locking orientation #232

Merged
merged 4 commits into from
Feb 15, 2023

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Oct 27, 2022

Closes #221

The following tasks have been completed:

Implementation commitment:


Preview | Diff

@annevk
Copy link
Member

annevk commented Oct 27, 2022

I guess this doesn't apply to unlock() because that shouldn't be possible? Maybe assert it? (Although maybe it's possible with manifest apps?)

@marcoscaceres
Copy link
Member Author

I guess this doesn't apply to unlock() because that shouldn't be possible? Maybe assert it? (Although maybe it's possible with manifest apps?)

I was thinking it shouldn't be possible because to hide the page/app it once locked, a user would need to exit fullscreen. But yeah, the assumption doesn't hold when there are multiple virtual desktops.

It may be safer just to prevent it in both cases.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably makes sense to split out the safety checks into a shared algorithm at some point.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Oct 31, 2022

Prepared test web-platform-tests/wpt#36734

Filed WebKit bug.

@saschanaz or @makotokato, @michaelwasserman, could you please file bugs in your respective engine repos if you support the change?

@marcoscaceres
Copy link
Member Author

Sent a patch for WebKit WebKit/WebKit#6283

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Nov 28, 2022

This change has been merged in WebKit.

@makotokato
Copy link

@makotokato
Copy link

makotokato commented Jan 24, 2023

BTW, web-platform-tests/wpt#36734 test (hidden_document.html) is still unclear for me. "hidden documents must not unlock the screen orientation" and "Once maximized, a minimized window can lock or unlock the screen orientation again" don't set pre-lock condition (All browser requires full screen). So I think the latest version of these tests are always failure due to no fullscreen.

Does it forget adding documenttElement.requestFullscreen before orientation.lock call?

@makotokato
Copy link

And, why don't we care hidden state in "8.3 Applying an orientation lock" section like fully active if it is in parallel case?

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 2, 2023
I would like to enable this API in Nightly for testing.

Supported in GeckoView and Windows tablet mode. Other platforms are no
affect by this preference. WPT [*1] runs on non-supported platform, so
a lot of tests are failure. (GeckoView passes a lot of tests [*2].)

Also, there is an issue of hidden document [*3], it isn't fixed by spec
side yet.

*1 https://wpt.fyi/results/screen-orientation
*2 https://searchfox.org/mozilla-central/source/testing/web-platform/meta/screen-orientation
*3 w3c/screen-orientation#232

Differential Revision: https://phabricator.services.mozilla.com/D168349
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Feb 3, 2023
I would like to enable this API in Nightly for testing.

Supported in GeckoView and Windows tablet mode. Other platforms are no
affect by this preference. WPT [*1] runs on non-supported platform, so
a lot of tests are failure. (GeckoView passes a lot of tests [*2].)

Also, there is an issue of hidden document [*3], it isn't fixed by spec
side yet.

*1 https://wpt.fyi/results/screen-orientation
*2 https://searchfox.org/mozilla-central/source/testing/web-platform/meta/screen-orientation
*3 w3c/screen-orientation#232

Differential Revision: https://phabricator.services.mozilla.com/D168349
@marcoscaceres
Copy link
Member Author

@makotokato, apologies, just coming back to this.

Thanks for pointing out the issues with the tests and in section "8.3 Applying an orientation lock". I think that was just an oversight. Having a look now.

@marcoscaceres
Copy link
Member Author

@makotokato, about the tests, I've gone over them again and you are right... they were making incorrect assumptions. For example, on macOs at least, it appears one cannot minimize a window once it's in fullscreen. One needs to first exit fullscreen and then the window can be minimized.

Anyway, I'll delete those two tests.

And, why don't we care hidden state in "8.3 Applying an orientation lock" section like fully active if it is in parallel case?

Yeah, good catch. I think instead what needs to happen there is that if the preconditions are not longer met (which is remaining visible and in fullscreen).

@marcoscaceres
Copy link
Member Author

@makotokato, I've filed #243 to follow up on your comment above. I'll draft up a separate pull request for that.

About the tests, I sent web-platform-tests/wpt#38511

@marcoscaceres marcoscaceres merged commit 0ef45ef into gh-pages Feb 15, 2023
@marcoscaceres marcoscaceres deleted the lock_hidden branch February 15, 2023 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should hidden documents be allowed to change the orientation?
3 participants