Bug 214169 - MobileSafari rotates its scene to portrait upside down if it has a PiP on screen
Summary: MobileSafari rotates its scene to portrait upside down if it has a PiP on screen
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-09 17:45 PDT by Peng Liu
Modified: 2020-07-10 13:13 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.72 KB, patch)
2020-07-09 17:50 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (2.17 KB, patch)
2020-07-10 10:37 PDT, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2020-07-09 17:45:42 PDT
MobileSafari rotates its scene to portrait upside down if it has a PiP on screen
Comment 1 Peng Liu 2020-07-09 17:50:10 PDT
Created attachment 403937 [details]
Patch
Comment 2 Peng Liu 2020-07-09 17:50:44 PDT
<rdar://problem/64820800>
Comment 3 Sam Weinig 2020-07-09 17:51:39 PDT
Comment on attachment 403937 [details]
Patch

What will it take to make a test for this?
Comment 4 Jer Noble 2020-07-09 17:56:02 PDT
Comment on attachment 403937 [details]
Patch

I'm concerned that this line was in here for a good reason, like allowing a portrait-locked app to open video in landscape fullscreen.

Maybe a better idea is to set `m_viewContrtoller.get()._ignoreAppSupportedOrientations = NO` when we hide the window?
Comment 5 Peng Liu 2020-07-10 10:32:09 PDT
(In reply to Sam Weinig from comment #3)
> Comment on attachment 403937 [details]
> Patch
> 
> What will it take to make a test for this?

We need a full UI test, where we can open a WK2 app, rotate the device into upside-down-mode, and verify that the UI doesn’t flip that way. That cannot be done with a layout test. We might be able to do that with an API test, but we need to fix a bug regarding video fullscreen and picture-in-picture test first (bug 212654).
Comment 6 Jer Noble 2020-07-10 10:35:39 PDT
(In reply to Peng Liu from comment #5)
> (In reply to Sam Weinig from comment #3)
> > Comment on attachment 403937 [details]
> > Patch
> > 
> > What will it take to make a test for this?
> 
> We need a full UI test, where we can open a WK2 app, rotate the device into
> upside-down-mode, and verify that the UI doesn’t flip that way. That cannot
> be done with a layout test. We might be able to do that with an API test,
> but we need to fix a bug regarding video fullscreen and picture-in-picture
> test first (bug 212654).

We need more than API tests, because we can't drive simulated hardware rotations from API tests. XCUITest would be a reasonable framework for doing this, I think, but we don't have any XCUITests set up at the moment. 

XCUITests would let us do a bunch more tests of things like "is the fullscreen airplay button "active" during an airplay session", and the like.
Comment 7 Peng Liu 2020-07-10 10:37:55 PDT
Created attachment 403981 [details]
Patch
Comment 8 Peng Liu 2020-07-10 10:41:12 PDT
(In reply to Jer Noble from comment #4)
> Comment on attachment 403937 [details]
> Patch
> 
> I'm concerned that this line was in here for a good reason, like allowing a
> portrait-locked app to open video in landscape fullscreen.
> 
> Maybe a better idea is to set
> `m_viewContrtoller.get()._ignoreAppSupportedOrientations = NO` when we hide
> the window?

Good point! I think we need to ignore App's supported orientations when a video is in fullscreen. But, when the video is in picture-in-picture, we should NOT ignore that. I have uploaded a new patch.
Comment 9 Peng Liu 2020-07-10 10:43:03 PDT
(In reply to Jer Noble from comment #6)
> (In reply to Peng Liu from comment #5)
> 
> We need more than API tests, because we can't drive simulated hardware
> rotations from API tests. XCUITest would be a reasonable framework for doing
> this, I think, but we don't have any XCUITests set up at the moment. 
> 
> XCUITests would let us do a bunch more tests of things like "is the
> fullscreen airplay button "active" during an airplay session", and the like.

Right!
Comment 10 EWS 2020-07-10 13:13:38 PDT
Committed r264237: <https://trac.webkit.org/changeset/264237>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403981 [details].