Closed Bug 840147 Opened 9 years ago Closed 9 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:leo+, b2g18+ verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 + verified
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: jsmith, Assigned: rexboy)

References

()

Details

(Whiteboard: [LeoVB-])

Attachments

(2 files)

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.
Component: General → Gaia::System
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.
blocking-b2g: --- → tef?
Not a blocker for v1.0.1, but Leo would like to block on this fix.
blocking-b2g: tef? → leo+
tracking-b2g18: --- → +
To be clear, this is not about closing the app - but by hitting the home button.
Assignee: nobody → rexboy
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..
Flags: needinfo?(mounir)
Flags: needinfo?(Ms2ger)
I've only touched the DOM binding parts of lockOrientation; you want mounir.
Flags: needinfo?(Ms2ger)
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 ;)
Flags: needinfo?(mounir) → needinfo?(etienne)
FWIW animating out the frame as it was (in landscape) sounds good to me.
Flags: needinfo?(etienne) → needinfo?(firefoxos-ux-bugzilla)
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(fdjabri)
(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.
Flags: needinfo?(fdjabri)
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.
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.
If you think your work could be easily split into 2 bugs then I am fine to have a followup.
Attached file 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!
Attachment #750386 - Flags: review?(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.
Attachment #750386 - Flags: review?(alive)
rexboy - what are the next steps here?
Flags: needinfo?(rexboy)
Sorry for the late update.
I'll discuss Alive soon and complete the patch.
Flags: needinfo?(rexboy)
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 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.
Attachment #750386 - Flags: review?(alive)
Attached image Screenshot
So it looks like this for apps switched into cardview from other orientations.
Comment on attachment 750386 [details]
Patch

r+ with github comment addressed. Please fix them and test again because this is easy to cause regression.
Attachment #750386 - Flags: review?(alive) → review+
Thank you, Alive!
All changes commented are updated.

Master:
https://github.com/mozilla-b2g/gaia/commit/92c49c0ef697b6ac795db94eea9d8c31f103460c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
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
Flags: needinfo?(rexboy)
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.
Depends on: 804270
Flags: needinfo?(rexboy)
Flags: in-moztrap?
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.
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.
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?
Flags: needinfo?(rexboy)
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!
Flags: needinfo?(rexboy)
Keywords: verifyme
Flags: in-moztrap? → in-moztrap+
Oops, I was told the v1-train patch cause some problem on Keyboard.
Backout for now before I fix it. Sorry for this.
Should be good now.

see bug 877903 comment 30 for the commit link.
Keywords: verifyme
v1.1.0hd: 4a5aa95c17656a84395eb78763f7f1c0f9623827
v1.1.0hd: d55d2d2d2317b09cc61404ec6d23c0346e9e006b
v1.1.0hd: 0dcb200ccaa84fc53fea87009911aa2d682131d6
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.
Keywords: verifyme
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.
Can someone also back this out on 1.1 hd? I'd imagine this bug is reproducing there as well.
Depends on: 854759
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.
Keywords: verifyme
Depends on: 890779
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.
No longer depends on: 890779
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
Flags: needinfo?(alive)
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?
Flags: needinfo?(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.
Whiteboard: [LeoVB+]
v1.1.0hd: 5d7a0da528dce626b563abafc135eb828599d3c0
Whiteboard: [LeoVB+] → [LeoVB-]
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.
You need to log in before you can comment on or make changes to this bug.