Closed
Bug 877903
Opened 11 years ago
Closed 11 years ago
Starting an app in landscape orientation shows screenshot first in portrait
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(blocking-b2g:leo+, b2g18 verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)
People
(Reporter: Harald, Assigned: rexboy)
References
Details
(Whiteboard: [mozilla-triage] [LeoVB-])
Attachments
(4 files, 1 obsolete file)
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•11 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)
Comment 2•11 years ago
|
||
That's a very old build.
Can you reproduce this with the patch included on bug 840147?
Flags: needinfo?(hkirschner)
Comment 3•11 years ago
|
||
This has nothing to do with bug 840147. It sounds like Gaia window manager that should do the locking earlier.
Flags: needinfo?(mounir)
Updated•11 years ago
|
Flags: needinfo?(hkirschner)
Reporter | ||
Comment 4•11 years ago
|
||
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•11 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
Comment 6•11 years ago
|
||
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•11 years ago
|
Assignee: nobody → alive
Flags: needinfo?(alive)
Comment 7•11 years ago
|
||
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.
Comment 8•11 years ago
|
||
(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.
Assignee | ||
Comment 9•11 years ago
|
||
Let's write a v1-train patch based on bug 840147... Stealing.
Assignee: alive → rexboy
Assignee | ||
Comment 10•11 years ago
|
||
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)
Assignee | ||
Comment 11•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #762581 -
Flags: review?(alive) → review+
Comment 12•11 years ago
|
||
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+
Assignee | ||
Comment 13•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #762581 -
Flags: feedback?(alive)
Comment 14•11 years ago
|
||
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 15•11 years ago
|
||
Comment on attachment 762581 [details]
Patch (v1-train)
See last comment.
Attachment #762581 -
Flags: review+
Attachment #762581 -
Flags: feedback?(alive)
Comment 16•11 years ago
|
||
(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 ?
Assignee | ||
Comment 17•11 years ago
|
||
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)
Assignee | ||
Comment 18•11 years ago
|
||
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 19•11 years ago
|
||
Comment on attachment 762578 [details]
patch (master)
Well done! Window Manager's weight decreases!
(With some github comments.)
Attachment #762578 -
Flags: review?(alive) → review+
Assignee | ||
Comment 20•11 years ago
|
||
Comment 21•11 years ago
|
||
I suggest let's wait some days to see if regression occurs before going to v1-train.
Assignee | ||
Comment 22•11 years ago
|
||
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.
Assignee | ||
Comment 23•11 years ago
|
||
Assignee | ||
Comment 24•11 years ago
|
||
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 25•11 years ago
|
||
Comment on attachment 767034 [details]
Patch (unit-test regression)
r+ if travis goes green after processing.
Attachment #767034 -
Flags: review?(alive) → review+
Assignee | ||
Comment 26•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
status-b2g18:
--- → affected
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-b2g-v1.1hd:
--- → affected
Resolution: --- → FIXED
Assignee | ||
Comment 27•11 years ago
|
||
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.
Assignee | ||
Comment 28•11 years ago
|
||
Assignee | ||
Comment 29•11 years ago
|
||
Oops, I was told the patch cause some problem on Keboard.
Backout for now before I fix it. Sorry for this.
Assignee | ||
Comment 30•11 years ago
|
||
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.
Comment 31•11 years ago
|
||
v1.1.0hd: 4a5aa95c17656a84395eb78763f7f1c0f9623827
v1.1.0hd: d55d2d2d2317b09cc61404ec6d23c0346e9e006b
v1.1.0hd: 0dcb200ccaa84fc53fea87009911aa2d682131d6
Comment 32•11 years ago
|
||
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 33•11 years ago
|
||
Can someone also back this out on 1.1 hd? I'd imagine this bug is reproducing there as well.
Updated•11 years ago
|
Flags: needinfo?(ryanvm)
Updated•11 years ago
|
Flags: needinfo?(ryanvm) → needinfo?(jhford)
Assignee | ||
Comment 34•11 years ago
|
||
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
Flags: needinfo?(jhford)
Comment 35•11 years ago
|
||
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.
Keywords: verifyme
Comment 36•11 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
Comment 37•11 years ago
|
||
v1.1.0hd: 5d7a0da528dce626b563abafc135eb828599d3c0
You need to log in
before you can comment on or make changes to this bug.
Description
•