861846 - Landscape orientation locked apps render portrait in the task manager, which breaks the view of the app

Status: VERIFIED FIXED

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

Description

[Jason Smith [:jsmith]]

STR:

1. Install an app that locks landscape orientation
2. Launch it
3. Open it in the task manager

Expected

The app should be rendered with it's landscape orientation.

Actual

The app is rendered in a portrait orientation. This ends up making the task manager card for the app looking broken, as the app isn't designed to viewed in portrait.

In triage, the motivating bug this derives from is bug 850102. Daniel indicated to nom this as this will break gaming apps, which are common apps that take advantage of locking landscape orientation during app usage.

Comment 1

[Alex Keybl [:akeybl]]

Jaime/Josh - can you help with a UX proposal here? We'll decide whether or not to block on the risk of implementing the new spec.

Comment 2

[Josh Carpenter [:jcarpenter]]

Hi guys, I just discussed with Jason and we're in agreement: we should capture and display screenshots for locked-landscape apps in landscape. The example from #850102 is pretty bad.

Comment 3

[Jose Manuel Cantera]

(In reply to Josh Carpenter [:jcarpenter] from comment #2)
> Hi guys, I just discussed with Jason and we're in agreement: we should
> capture and display screenshots for locked-landscape apps in landscape. The
> example from #850102 is pretty bad.

Hi Josh,

Cristian will look into this, you can talk during the WW about the best way to deal with it.

Comment 4

[Josh Carpenter [:jcarpenter]]

Attachment: Screenshot of correct implementation

Attached is a screenshot of the ideal implementation for locked-landscape apps.

Comment 5

[Cristian Rodriguez (:crdlc)]

It is perfect, thanks for the feedback

Comment 6

[Alex Keybl [:akeybl]]

Given the simplicity of this spec, we think this will be OK for v1.0.1. This was a request Daniel heard from many of our partners, and he wanted to take this if simple.

Comment 7

[Cristian Rodriguez (:crdlc)]

Attachment: New approach, screenshot 1

Comment 8

[Cristian Rodriguez (:crdlc)]

Attachment: New approach, screenshot 2

Comment 9

[Cristian Rodriguez (:crdlc)]

Hi Josh, could we follow this new approach? Basically if we have to implement your screenshot, we have problems with the different aspect ratios between landscape and portrait and we should add more code and complexity.

Thanks

https://bug861846.bugzilla.mozilla.org/attachment.cgi?id=738398
https://bug861846.bugzilla.mozilla.org/attachment.cgi?id=738399

Comment 10

[Cristian Rodriguez (:crdlc)]

Here [1] is the patch for this solution. Pretty easy without risk

[1] https://github.com/crdlc/gaia/commit/d05f1efcc90787c98475185545c3675fb8ff6fbb

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Cristian Rodriguez (a tope!) from comment #10)
> Here [1] is the patch for this solution. Pretty easy without risk
> 
> [1]
> https://github.com/crdlc/gaia/commit/d05f1efcc90787c98475185545c3675fb8ff6fbb

I would say this is good enough to make the patch not a blocker anymore. I would be fine to land this and top open a new bug to implement the right spec.

Comment 12

[Cristian Rodriguez (:crdlc)]

I like to listen that from you :) Here is the patch for master 

https://github.com/crdlc/gaia/commit/84d2d5f36e44262dfe8f4b89c58311975fa4f0a9

Comment 13

[David Scravaglieri [:scravag]]

(In reply to Cristian Rodriguez (a tope!) from comment #12)
> I like to listen that from you :) Here is the patch for master 
> 
> https://github.com/crdlc/gaia/commit/84d2d5f36e44262dfe8f4b89c58311975fa4f0a9

Cristian could you create a new bug for the remaining UI work and mark this one as Resolved Fixed after review please ?

Comment 14

[Cristian Rodriguez (:crdlc)]

Follow the new implementation from bug 863249

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Cristian Rodriguez (a tope!) from comment #12)
> I like to listen that from you :) Here is the patch for master 
> 
> https://github.com/crdlc/gaia/commit/84d2d5f36e44262dfe8f4b89c58311975fa4f0a9

The patch sounds good to me. r+.

Comment 16

[Cristian Rodriguez (:crdlc)]

Josh we are waiting for your feedback, are you comfortable with this approach? thanks a lot

Comment 17

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v1 for v1.0.1

Comment 18

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v1 for master

Comment 19

[Josh Carpenter [:jcarpenter]]

(In reply to Cristian Rodriguez (a tope!) from comment #16)
> Josh we are waiting for your feedback, are you comfortable with this
> approach? thanks a lot

I can live with this for 1.0.1 and 1.1, but we'd strongly prefer to force all Task Manager screenshots into portrait. I'll file a separate 1.x bug for that.

Comment 20

[Josh Carpenter [:jcarpenter]]

Filed bug #863699 to correct this in next version.

Comment 21

[Andrew Overholt [:overholt]]

Are the fixes here ready to land?

Comment 22

[Cristian Rodriguez (:crdlc)]

v1.0.1

https://github.com/mozilla-b2g/gaia/commit/417e6826f9089d110d90ec8b19933f0f58b921e3

master

https://github.com/mozilla-b2g/gaia/commit/d487a2c66c70cfee5ccb122a4fe788a9429bcfc4

Comment 23

[Julien Wajsberg [:julienw]]

I think this broke an unit test, see https://travis-ci.org/mozilla-b2g/gaia/builds/6591554

Should hopefully be easy to fix, just add the necessary function to the mock, but it could be useful to also add a test case for your fix here.

Comment 24

[Cristian Rodriguez (:crdlc)]

I gonna fix it

Comment 25

[Cristian Rodriguez (:crdlc)]

follow up bug 865197

Comment 26

[Jason Smith [:jsmith]]

FYI - you still need to land this on b2g 18.

Comment 27

[Cristian Rodriguez (:crdlc)]

Should I do the uplift? Thanks

Comment 28

[Julien Wajsberg [:julienw]]

James Lal does the uplifts this week (jhford is in holiday).

Comment 29

[James Lal [:lightsofapollo]]

I see there is a followup here? Should I wait for that to land prior to uplifting ?

Comment 30

[Cristian Rodriguez (:crdlc)]

That bug only fixes an broken unit test. We could uplift both at the same time, better?

Comment 31

[James Lal [:lightsofapollo]]

Would rather wait since the tef part has already landed ( no time urgency to break the tests on v1.1 further ).

Comment 32

[James Lal [:lightsofapollo]]

OK tests landed I will do the uplift but I can see there is now a css conflict on v1-train...

Comment 33

[Julien Wajsberg [:julienw]]

bug 859259 is generating this conflict.

Comment 34

[Cristian Rodriguez (:crdlc)]

I understand that it should be on v1-train but the new transition is not already landed on v1-train. I think that we are going to try to nominate this bug 859259 to v1-train. Maybe we could wait if finally this one would be landed on v1-train. Are you comfortable with it? If finally it wouldn't landed I will have to implement the patch for v1-train.

Thanks

Comment 35

[Jason Smith [:jsmith]]

So this patch definitely isn't working for me on 1.01 on a 4/26 build. I tested this using Poppit and I'm still getting exactly the same behavior I had before the patch existed.

Any ideas why?

Comment 36

[Cristian Rodriguez (:crdlc)]

Is Poppit defining landscape-primary as orientation in the manifest?

Comment 37

[Jason Smith [:jsmith]]

(In reply to Cristian Rodriguez (a remontar!) from comment #36)
> Is Poppit defining landscape-primary as orientation in the manifest?

Nope. They are defining landscape generally:

    "orientation": ["landscape"]

See https://github.com/telefonicaid/firefoxos-gaia-latam/blob/master/external-apps/poppit/application.zip for the actual app.

Does this patch only work with landscape-primary? How does this patch work with landscape-secondary and landscape then?

Comment 38

[Cristian Rodriguez (:crdlc)]

Yes, my patch only works with landscape-primary. AFAIK I don't know how to get the current orientation of the apps before closing. Maybe, it is due to my ignorance in the system app. 

I know how to read the app's manifest and if the orientation is landscape-primary I know that the screen orientation is locked to this. 

Well, If you say me that if the orientation is defined as landscape implies that the app is always displayed with this orientation as well and it is locked; I could re-open the bug and I gonna implement both cases: landscape and landscape-primary.

I mean, I am sure that if the app is landscape-primary, the orientation is locked to landscape. I don't know if the app is landscape, the orientation is locked or not. Or if somebody knows that there is an API to know the current screen orientation it will be very useful.

Thanks

Comment 39

[Jason Smith [:jsmith]]

Okay. I'll file a followup bug for the landscape-secondary and landscape use cases.

FWIW, I think you can rely on using the app manifest property here to figure this out. You could do something like:

- landscape-primary - do what you are already doing
- landscape-secondary - same idea of what you are doing with landscape-primary but with different orientation
- landscape - since both orientations are valid, if it makes things easier, you could just render this as landscape-primary in the task manager

Comment 40

[Jason Smith [:jsmith]]

Filed bug 867149 as a followup. Marking verified to indicate to testing was done here on 1.01 and mozilla-central.

Comment 41

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

I'm not sure what needs to be done here to be able to set status-b2g18 to fixed.  Is this fix on v1-train?  Does it need to be on v1-train?

Comment 42

[Julien Wajsberg [:julienw]]

yes john, please

with Bug 865197

Comment 43

[Maria Angeles Oteo (:oteo)]

It's pending the uplift of this patch to V1-train
thanks!!

Comment 44

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

I missed that this was manually landed to v1.0.1 but not v1-train in the middle of the comments.

v1-train: b635c47de803bb95d9ddd8eecf70eb0ace050a77

Comment 45

[Timur Kristóf]

I can still see this bug in latest v1.1