Starting an app in landscape orientation shows screenshot first in portrait

RESOLVED FIXED in Firefox OS v1.1hd

Status

Firefox OS
Gaia::System
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Harald, Assigned: rexboy)

Tracking

unspecified
1.1 QE3 (26jun)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [mozilla-triage] [LeoVB-])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
When an app launches in landscape,it is is a very noticeable delay between click, startup screenshot and screen rotation. Showing the screenshot in portrait for a longer time looks badly broken.

This affects a high-profile partner which reviews the user experience very critically.

Might be related to bug 840147.
(Reporter)

Comment 1

5 years ago
Reproducible with https://marketplace.firefox.com/app/zombiez

Tested Unagi with 1.1.0.0 20130320070207, but also clearly visible on 1.0.

Need info from mounir as he was looking into bug 840147.
Flags: needinfo?(mounir)
That's a very old build.

Can you reproduce this with the patch included on bug 840147?
Flags: needinfo?(hkirschner)
This has nothing to do with bug 840147. It sounds like Gaia window manager that should do the locking earlier.
Flags: needinfo?(mounir)

Updated

5 years ago
Flags: needinfo?(hkirschner)
(Reporter)

Comment 4

5 years ago
Created attachment 759488 [details]
Test Case - Loading a game in landscape

Tested on Unagi - 1.0.1 - 06-Jun-2013 08:40: http://www.youtube.com/watch?v=JaTIhvtTXq0
1st launch bug: Loading screen has ugly white padding
2nd+ launch bug: Snapshot is shown as filling pattern in portrait
Task switching bug: Games shrinks while showing portrait mode

Tested on Unagi - 1.1 - 06-Jun-2013 08:31: http://www.youtube.com/watch?v=chKgcgBdGMc
1st launch bug: Loading screen has ugly white padding
Task switching bug: Games shrinks/grows while showing portrait mode

This shows how loading and task switching is badly impacted by visual glitches.

As this impacts the approval from a high priority partner I nominate it as blocker.
(Reporter)

Comment 5

5 years ago
As mentioned above this affects user experience for landscape oriented games. This is glitch is very visible to a priority partner that has to approve their app as being preloaded.
blocking-b2g: --- → leo?
Component: Gaia → Gaia::System
We would block on this, but at this point we need partner buy-in to mark as blocking, leaving this for partner triage to evaluate.
Flags: needinfo?(alive)
Whiteboard: [mozilla-triage]

Updated

5 years ago
blocking-b2g: leo? → leo+
Assignee: nobody → alive
Flags: needinfo?(alive)
Exactly bug 840187 solves this problem --- only in master.
But because the work depends on some patches only landed in master, it's not uplift-able.
What we need here is a v1-train only patch.
(In reply to Alive Kuo [:alive] from comment #7)
> Exactly bug 840187 solves this problem --- only in master.
Sorry, it should be bug 840147.

> But because the work depends on some patches only landed in master, it's not
> uplift-able.
> What we need here is a v1-train only patch.
Let's write a v1-train patch based on bug 840147... Stealing.
Assignee: alive → rexboy
Created attachment 762578 [details]
patch (master)

After testing the test case I found we need some small changes on master.
- The top and left position (added for opening animation) should be removed right after opening animation ended.
- We should consider apps with orientation just "landscape" rather than landscape-primary and landscape-secondary. I make landscape open in landscape-primary by default.
Attachment #762578 - Flags: review?(alive)
Created attachment 762581 [details]
Patch (v1-train)

Patch for v1-train. These codes mostly the same as patch for bug 840147, but the css part changes significantly because the transition differ from master. See comments on Github.
Attachment #762581 - Flags: review?(alive)
Attachment #762581 - Flags: review?(alive) → review+
Comment on attachment 762578 [details]
patch (master)

r=me if this works when the orientation of app is 'landscape-secondary'.
If it doesn't, please use mozOrientation as parameter to decide rotation angle instead of amending '-primary' to non suffix orientation.
Attachment #762578 - Flags: review?(alive) → review+
Comment on attachment 762578 [details]
patch (master)

Alive: I've added codes to determine actual device orientation for opening apps. Please take a look, thanks!
Attachment #762578 - Flags: feedback?(alive)
Attachment #762581 - Flags: feedback?(alive)
Comment on attachment 762578 [details]
patch (master)

Some notions I remember:

0) Use screen.mozOrientation and mozorientationchange event if possible.
1) Don't use dataset as cross module communication. Use object attributes.
   app.frame.dataset.orientation -> app.currentOrientation
   Therefore, please move these into appWindow object.(window.js)
   (A) dataset.orientation
   (B) setRotationOrientation
   (C) setSize ----- if possible and if necessary. See https://github.com/alivedise/gaia/commits/testable-window-manager resize in app_window.js for reference. (Note: do not copy and paste here)
2) Use class as css rules instead of dataset.orientation,
   e.g., div.appWindow.orientation-landscape.
3) The flicking of orientation lock after window is opened seems strange. Investigate more and file a gecko bug if it's really a gecko bug....(flicking when screen.mozLockOrientation('portrait-primary') to screen.mozLockOrientation(APP_ORIENTATION)) Try remove/add css earlier/later and/or use one time orientationchange event handler.
4) We don't need to calculate deviceorientation every second. Use it on demand when transitioning is happening(appwillopen---->appopen, appwillclose---->appclose)
5) Write some jsdoc comment when you move something into AppWindow and it's public/private attribute/method. private with _ prefix.

Sorry for not detailed review last time, but let's refine these in a non-urgent bug and get everything move forward. Thanks.
Attachment #762578 - Flags: review+
Attachment #762578 - Flags: feedback?(alive)
Comment on attachment 762581 [details]
Patch (v1-train)

See last comment.
Attachment #762581 - Flags: review+
Attachment #762581 - Flags: feedback?(alive)
(In reply to Alive Kuo [:alive] from comment #14)
> 2) Use class as css rules instead of dataset.orientation,
>    e.g., div.appWindow.orientation-landscape.

Or div.appWindow.rotate-to-landscape-orientation ?
Blocks: 845251
For (3), after some investigating, css for rotating is sometimes removed too early before |orientationchange| raised, so the window looks like flicking.
if we don't want to add one additional mozScreenLock() (to make |orientationchange| raise earlier), we may need to listen to |orientationchange| somewhere and remove css there. (I'm not sure where is appropriate for now, because this event doesn't raise necessarily)

Updated

5 years ago
Target Milestone: --- → 1.1 QE3 (24jun)
Comment on attachment 762578 [details]
patch (master)

There are significant refactor works so let's review on master first.

After that, I will merge changes into v1-train patch.
Attachment #762578 - Flags: review?(alive)
Comment on attachment 762578 [details]
patch (master)

Well done! Window Manager's weight decreases! 
(With some github comments.)
Attachment #762578 - Flags: review?(alive) → review+
master:
https://github.com/mozilla-b2g/gaia/commit/d16372086c5a341e3b308c6d3504909c948fc010
I suggest let's wait some days to see if regression occurs before going to v1-train.
Comment on attachment 762581 [details]
Patch (v1-train)

Updated patch for v1-train.
There were a little bit more works on resolving conflicts.
As last patch, Javascript part are mostly the same with master, CSS part varies significantly but mostly the same as last one.

So I think no bother setting another review, I can set it if someone think it's needed though.
Created attachment 767031 [details]
Patch (unit-test regression)
Created attachment 767034 [details]
Patch (unit-test regression)

Oops... I'm so sorry. A nit error break the unit test after fixing comments on Github.

Alive would you mind reviewing this patch?
Thanks a lot!
Attachment #767031 - Attachment is obsolete: true
Attachment #767034 - Flags: review?(alive)
Comment on attachment 767034 [details]
Patch (unit-test regression)

r+ if travis goes green after processing.
Attachment #767034 - Flags: review?(alive) → review+
Comment on attachment 767034 [details]
Patch (unit-test regression)

master
https://github.com/mozilla-b2g/gaia/commit/44a7f83aff166b897659531f259e3e1dd3ff27d1

I'll merge v1-train patch tomorrow if no more regression is reported.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
Resolution: --- → FIXED
Setting to RESOLVED FIXED since merged in master.

Just a notice that v1-train patch is different with master one so don't uplift master patch to v1-train.

Updated

5 years ago
Keywords: verifyme
QA Contact: jsmith
v1-train
https://github.com/rexboy7/gaia/commit/0dcb200ccaa84fc53fea87009911aa2d682131d6
status-b2g18: affected → fixed
Oops, I was told the patch cause some problem on Keboard.
Backout for now before I fix it. Sorry for this.
status-b2g18: fixed → affected
Seems it's just some mistyping during resolving conflict. Should be good now.
v1-train:
https://github.com/mozilla-b2g/gaia/commit/4a5aa95c17656a84395eb78763f7f1c0f9623827

Sorry for the inconvience.
status-b2g18: affected → fixed
v1.1.0hd: 4a5aa95c17656a84395eb78763f7f1c0f9623827
v1.1.0hd: d55d2d2d2317b09cc61404ec6d23c0346e9e006b
v1.1.0hd: 0dcb200ccaa84fc53fea87009911aa2d682131d6
status-b2g-v1.1hd: affected → fixed
Depends on: 888339
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.
status-b2g18: fixed → affected

Updated

5 years ago
Keywords: verifyme
Can someone also back this out on 1.1 hd? I'd imagine this bug is reproducing there as well.
Depends on: 854759

Updated

5 years ago
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm) → needinfo?(jhford)
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
status-b2g18: affected → fixed
Flags: needinfo?(jhford)

Updated

5 years ago
Keywords: verifyme

Updated

5 years ago
Depends on: 890780
The patch here works on a 7/7 b2g18 build, but I am seeing a problem with delayed app opening transitions now. See bug 890780 for a followup.
status-b2g18: fixed → verified
Keywords: verifyme

Comment 36

5 years ago
We are also seeing unusual behavior with the contacts picker not being sized correctly when the keyboard is up.  See bug 891408.
Depends on: 891408

Updated

5 years ago
Whiteboard: [mozilla-triage] → [mozilla-triage] [LeoVB+]
v1.1.0hd: 5d7a0da528dce626b563abafc135eb828599d3c0

Updated

4 years ago
Whiteboard: [mozilla-triage] [LeoVB+] → [mozilla-triage] [LeoVB-]
You need to log in before you can comment on or make changes to this bug.