840147 - Closing an app that specifies an orientation lock to landscape closes the app in portrait

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Jason Smith [:jsmith]]

Build: B2G 18 2/11/2013
Device: Unagi

Steps:

1. Install a web app that has orientation lock to landscape
2. Launch it while in portrait-primary
3. Close it while in portrait-primary

Expected:

The app should close in the orientation lock specified.

Actual:

The app closes in portrait mode.

Comment 1

[Jason Smith [:jsmith]]

Talked this over with TCL and tchung - given that we're blocking on bug 861846 since many gaming apps preinstalled on device lock a landscape orientation, we probably also need to block on this as well for the same reasons.

Comment 2

[Alex Keybl [:akeybl]]

Not a blocker for v1.0.1, but Leo would like to block on this fix.

Comment 3

[Alex Keybl [:akeybl]]

To be clear, this is not about closing the app - but by hitting the home button.

Comment 4

[KM Lee [:rexboy] (inactivate after Sep. 22)]

After some investigating, I found it's not so easy to solve it because mozLockOrientation changes orientation globally, which means it's impossible to keep homescreen in portrait while let the app in landscape during close animation. Several solutions proposed here but I need some suggestions.

1. Attach an app screenshot on app iframe before animation starts to shield wrong-oriented app.
2. Rotate app iframe by 90 degrees before animation starts, adjusting its size correspondingly to fit orientation.

I'm preferring 1 because app repaint may cause some displaying issue on 2. But let's ask if Gecko members agree on this or have better idea..

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I've only touched the DOM binding parts of lockOrientation; you want mounir.

Comment 6

[Mounir Lamouri (:mounir)]

What Android is doing isn't that bad: they switch back to the target orientation and do the animation. It is barely visible to the user. Working on the iframe level instead of orientation might work too. We could imitate iOS behaviour like that.

You should definitely get feedback from someone who has UI/UX experience, not me ;)

Comment 7

[Etienne Segonzac (:etienne)]

FWIW animating out the frame as it was (in landscape) sounds good to me.

Comment 8

[Francis Djabri [:djabber]]

(In reply to Etienne Segonzac (:etienne) from comment #7)
> FWIW animating out the frame as it was (in landscape) sounds good to me.

I agree. For the type of animation that we have, I think the best approach would be to do zoom out animation keeping the frame in landscape and then bring the home screen in in portrait.

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

For this case I propose to
1) Paste a screenshot of homescreen of portrait-primary mode until app window is closed.
2) Delay the orientation lock of homescreen into windowClosed function of the closed app, instead of locking once the animation begins.

Because the orientation lock API is for the whole screen, not app-specific.
If we don't do 1) you would see homescreen is locked in landscape mode before locking to portrait primary after zooming out animation you guys proposed in comment 7, comment 8.

Comment 10

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Status update:
After experiment and discussing with Tim and Alive, iframe rotation is used here to solve the problem. It's somewhat dirty work on css because we need to do rotate on the closing / opening app and write css for each direction.
I'm now done with app close and app open animation but open/close from card view may also need to be fixed under this design. I prefer to fix the card view in another bug though.

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

If you think your work could be easily split into 2 bugs then I am fine to have a followup.

Comment 12

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Patch

First try.
This patch uses css rotation to "pretend" that apps can orientate different from homescreen during transition.
I decided to write the required animation inside css so each open/close app related rule turns into 4 of them, corresponding to 4 directions.

Alive, may you take a look and give some suggestions? Thanks a lot!

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 750386 [details]
Patch

Rex, great work though there're still some things to be done.
1) Keyboard resize handle is too dirty
2) Cardview problem.

It's not your fault that this fix doesn't match cardview fix, cause it's strange that 2 modules are having their-self orientation management.

Let's have a one-on-one discussion about how to improve this next week after you finish your blocking bugs, if you're still willing to work on this non-trivial change.

Comment 14

[Alex Keybl [:akeybl]]

rexboy - what are the next steps here?

Comment 15

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Sorry for the late update.
I'll discuss Alive soon and complete the patch.

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

Quick summary about what should be done:
1. (Try)Remove the card view change about orientation(don't rotate the screenshot because you had rotated it in WM)
2. Remove the keyboard sendMessage hack.

Comment 17

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 750386 [details]
Patch

I've updated the patch mainly following these 2 points Alive mentioned.

For card view, I'd like to propose a similar way I did on windowManager (CSS rotation) to achieve orientated screenshots.

Comment 18

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Screenshot

So it looks like this for apps switched into cardview from other orientations.

Comment 19

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 750386 [details]
Patch

r+ with github comment addressed. Please fix them and test again because this is easy to cause regression.

Comment 20

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Thank you, Alive!
All changes commented are updated.

Master:
https://github.com/mozilla-b2g/gaia/commit/92c49c0ef697b6ac795db94eea9d8c31f103460c

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 92c49c0ef697b6ac795db94eea9d8c31f103460c
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 22

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Oops, so many conflicts here..
I'm sure that we need Bug 804270 because this patch is just based on this new transition. Other conflicts (like bug 834709, for app-to-app transition) may not necessary that I can just solve the conflict. We need to take care of regressions though.

I'll update if I found any other required bugs here.

Comment 23

[KM Lee [:rexboy] (inactivate after Sep. 22)]

By the comment of :crdle in bug 804270, bug 804270 depends on bug 866174 which is not going to be uplifted. Thus this bug is not uplift-able on myself.

I've tested the latest v1-train, the app doesn't close in landscape now (it closes without animation, though). Not sure if someone have already made some workaround on it. But landscape apps still open in portrait. If we still care about it, I think we can do one of the following: (1)take the risk and re-nominate Bug 804270 and Bug 866174 above to v1-train again. (2)I can try to write a workaround for app opening and this is just for v1-train. Perhaps no transition either.

Comment 24

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Sorry for the mistyping. I mean, the app can close in landscape (as expected) now, even without uplifting this patch; but there's no animation during closing(acceptable but somewhat unexpected).

On the other hands, in current v1-train, app can't open in landscape (bug 877903). Although this patch solves it but something blocked us from uplifting it as my comment above.

Comment 25

[Jason Smith [:jsmith]]

Per your above comments, do we still need to create a custom patch for this on v1-train? Or did something else land that got this working on v1-train?

Comment 26

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Hi Jason:
Sorry, I forgot to update here.
Please ignore comment #24. I'm now working on a v1-train patch that both fixes bug 877903 and bug 840147. It will be reviewed in bug 877903. After it's fixed I'll update the status here.
Thanks!

Comment 27

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Fixed by v1-train patch in bug 877903.
https://github.com/rexboy7/gaia/commit/0dcb200ccaa84fc53fea87009911aa2d682131d6

Comment 28

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Oops, I was told the v1-train patch cause some problem on Keyboard.
Backout for now before I fix it. Sorry for this.

Comment 29

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Should be good now.

see bug 877903 comment 30 for the commit link.

Comment 30

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 4a5aa95c17656a84395eb78763f7f1c0f9623827
v1.1.0hd: d55d2d2d2317b09cc61404ec6d23c0346e9e006b
v1.1.0hd: 0dcb200ccaa84fc53fea87009911aa2d682131d6

Comment 31

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I just backed out this change in Gaia v1-train,
be24875471b2a23e61c9deda8e75b13b56e1a116, because it broke the smoke test, Bug 888339.

I did not back out the changes in Gaia master, because I could not reproduce Bug 888339 on Gaia master.

Comment 32

[(inactive after 6/18) Alive Kuo [:alive]]

Can we uplift keyboard manager refine patch in https://bugzilla.mozilla.org/show_bug.cgi?id=854759?
I don't know if this is the root cause though, but I think uplift it is risk-less and would help developer on working keyboard resize stuff.

Comment 33

[Jason Smith [:jsmith]]

Can someone also back this out on 1.1 hd? I'd imagine this bug is reproducing there as well.

Comment 34

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Based on Comment #32, let's nominate bug 854759. That way we won't need to write additional code for adjusting keyboard height on v1-train.

Comment 35

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Bug 854759 uplifted so let's do it again (the older patch has been backed out.):

v1-train
https://github.com/mozilla-b2g/gaia/commit/5d7a0da528dce626b563abafc135eb828599d3c0

v1.1.0hd
https://github.com/mozilla-b2g/gaia/commit/ad4d7406bf616c468b0472e9ff99e74e08f7b6a2

( also noted in https://bugzilla.mozilla.org/show_bug.cgi?id=877903#c34 )

Comment 36

[Jason Smith [:jsmith]]

The patch here works as expected in a 7/7 b2g18 build, but I am seeing the closing app transition not showing up occasionally. See bug 890779 for a followup.

Comment 37

[hanj.kim25]

We have been informed that ever since this patch was applied, the usage of sensord has been increased, as well as the voltage consumption.. What's wrong with this patch?

Thanks
Hanjoon

Comment 38

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Is there any detail?

All I can come up is that this patch listens to ondeviceorientation in background, this event raises about 1~2 time per second. The listener is attached only when screen is on.

May you provide if there are some guidance about sensor and voltage consumption?

Comment 39

[(inactive after 6/18) Alive Kuo [:alive]]

See comment 38.
If you couldn't accept this kind of sensor usage, I could only suggest that don't block on this kind of UI polish bug. The app orientation works well "after" its transition finishes. This orientation error during transition shouldn't be a blocker.

Comment 40

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 5d7a0da528dce626b563abafc135eb828599d3c0

Comment 41

[(inactive after 6/18) Alive Kuo [:alive]]

Tim and I has an offline discussion about this, we have some new ideas to re-do this patch.
Rex let's discuss this tomorrow.

The rough idea is:
* Orientate any non-specific landscape orientation to landscape-primary. (Right hand rule)
* After app is opened, lock the screen orientation to what they address in manifest or unlock or lock to default orientation(if we have that as discussed in webAPI)
* If the app doesn't specify, just do nothing(keep the orientation of homescreen) until the app is opened and unlock the orientation.

So we don't need to check device orientation anytime anymore. Because we give a default orientation just before we really lock the orientation.

I think the original issue is caring about that a landscape app shouldn't be opened via portrait.