964927 - [B2G][Contacts] Changing phone orientation while on crop screen when adding a new contact picture distorts image

Status: RESOLVED FIXED

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

Description

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot

Description:
Changing phone orientation from portrait to landscape mode while choosing a new contact picture stretches image, distorting it noticeably. This occurs on the crop screen after choosing an image in the gallery.

Repro Steps:
1) Updated Buri to BuildID: 20140128040224
2) Open Contacts app
3) Tap plus icon to create new contact
4) Choose "Add Picture"
5) Select from Gallery
6) Press any available image
7) Rotate phone from portrait mode to landscape orientation

Actual:
Image distorts on gallery crop screen when choosing a new contact image with phone in landscape mode.

Expected:
Changing orientation of phone does not distort images.

Environmental Variables:
Device: Buri v1.4 Master Mozilla RIL
BuildID: 20140128040224
Gaia: 6586d2b8b43c6997be5cf5895bbdf3dd20490725
Gecko: 4da3e21a0e5f
Version: 29.0a1
Firmware Version: V1.2-device.cfg

Notes:
Repro frequency: 3/3, 100%
See attached: screenshot

Comment 1

[Jason Smith [:jsmith]]

Can we confirm that this does not reproduce on 1.3?

Comment 2

[Peter Bylenga [:PBylenga]]

Issue does not reproduce on today's nightly Buri v1.3.  Attempting to change the orientation has no effect, Crop screen is still in portrait mode.

Environmental Variables:
Device: Buri v1.3 MOZ RIL
BuildID: 20140129004052
Gaia: b0aa74619cffb7a39ee0e7da5e9361a98e69f3f6
Gecko: 7d1173c4b173
Version: 28.0a2
Firmware Version: V1.2-device.cfg

Comment 3

[Peter Bylenga [:PBylenga]]

The regression window is 1-9-2014 to 1-10-2014

First build issue reproduces in:
Master Environmental Variables:
Device: buri Master MOZ
BuildID: 20140110040206
Gaia: f400efc804366c7b7cf5476d1d5d325e6651ee71
Gecko: 37516445a0b5
Version: 29.0a1
Firmware Version: v1.2-device.cfg

Last build issue does not reproduce in:
Master Environmental Variables:
Device: buri Master MOZ
BuildID: 20140109040203
Gaia: 47206ac66b084c6f6c4503a3b10d0e0760df2b6f
Gecko: 9409405e0739
Version: 29.0a1
Firmware Version: v1.2-device.cfg

Comment 4

[David Flanagan [:djf]]

Based on comment #2, this may not be a regression but a change in the window manager.  The contacts app is always in portrait mode.  In 1.3, this meant that when picking an image we were always locked in portrait mode as well.

Is it intentional that in 1.4 we are able to change orientation like this as part of the pick activity?  Needinfo on Alive for the answer to that.

Does the gallery app correctly handle orientation changes while cropping a photo when you edit it?  If so, then this is only broken during a pick activity, not in the general case.

Comment 5

[David Flanagan [:djf]]

Peter,

In the 1/9 build when the bug did not reproduce, was that because, the orientation would not change at all?  Or did the orientation change, but the image was resized correctly?

In the former case, it means that a window manager change (or bug) has exposed a missing feature that we've never needed before.  In the later case this is just a regression.

Comment 6

[Peter Bylenga [:PBylenga]]

In the 1/9 build the bug did not reproduce due to the fact that the orientation would not change.  Sorry about that I tried to call that out in Comment 2.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Peter Bylenga from comment #6)
> In the 1/9 build the bug did not reproduce due to the fact that the
> orientation would not change.  Sorry about that I tried to call that out in
> Comment 2.

That implies this is likely a window manager regression with the fact that were allowing a portrait-locked app to rotate to landscape.

This might be a dupe also - I think we fixed an issue like this recently.

Comment 8

[Russ Nicoletti [:russn]]

Based on comment #7, I will look at the window manager code changes in the regression window to see if I can find the change responsible for the difference in behavior.

Comment 9

[Russ Nicoletti [:russn]]

This is the commit responsible for the regression:

apps/system/js/activity_window.js a98b09c55fb819cc5689b898a9bd92c04844f8d2
Bug #948226

Comment 10

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

Yes, the change is to fix bug 948226. The rationale now is: if activity's manifest has orientation specified, use it. Otherwise use the activity caller's orientation.

I will update the algorithm again to fix this and not cause bug 948226.

Russ are you working on this or just find out the regression range?

Comment 11

[Russ Nicoletti [:russn]]

I was working on it, initially to find the change that caused the regression. But I'd like to understand the changes you made for bug #948226. I'm not seeing that the changes correspond to the logic in your comment #10. They way I see the changes, they change which manifest to examine -- activity's manifest first then activity caller's -- they don't consider whether the activity's manifest has orientation before considering whether the activity caller's manifest has orientation. Because even if this.manifest.orientation is undefined, the code decides to use this.manifest as the manifest only if this.manifest is not null. 

It seems the code should be something like:
var orientation1 = (this.manifest) ? this.manifest.orientation : null;
var orientation2 = (this.config.manifest) ? this.config.manifest.orientation : null;
var orientation3 = (this.activityCaller.manifest) ? this.activityCaller.manifest.orientation : null;

var orientation = orientation1 || orientation2 || orientation3;

With that said, I'm still coming up to speed with ffos dev so I lack context here. I assume the activity caller's manifest is the manifest.webapp of the caller. What is the activity's manifest? Is that what is defined in the MozActivity? What is config.manifest?

Also, can you give me some context on how this code from your commit -- https://github.com/alivedise/gaia/blob/a98b09c55fb819cc5689b898a9bd92c04844f8d2/apps/system/js/activity_window.js#L133-L140 -- helped fix bug #948226?

Thanks.

Comment 12

[Russ Nicoletti [:russn]]

In apps/system/js/activity_window.js, I've replaced:

        var manifest = this.manifest || this.config.manifest ||
                       (this.activityCaller ?
                        this.activityCaller.manifest : null);

        if (!manifest) {
          if ('unlockOrientation' in screen) {
            screen.unlockOrientation();
          } else if ('mozUnlockOrientation' in screen) {
            screen.mozUnlockOrientation();
          }
          return;
        }

        var orientation = manifest ? (manifest.orientation ||
                          OrientationManager.globalOrientation) :
                          OrientationManager.globalOrientation;

with:

        // TODO name these variables properly
        var orientation1 = (this.manifest) ? this.manifest.orientation : null;
        var orientation2 = (this.config.manifest) ? this.config.manifest.orientation : null;
        var orientation3 = (this.activityCaller.manifest) ? this.activityCaller.manifest.orientation : null;

        var orientation = orientation1 || orientation2 || orientation3 || OrientationManager.globalOrientation;
        
I've tested the Twitter-->Tweet-->Camera scenario from bug 948226 and I've tested the Contacts-->New Contact-->Add Photo-->Gallery scenario from this bug. Both work properly.

Alive, can you let me know the purpose of the 'unlockOrientation' code you added? I will incorporate those changes as necessary.

Comment 13

[David Flanagan [:djf]]

Russ: unlockOrientation() is the call that allows the screen to rotate when the user rotates the phone. If an app specifies portrait or landscape then it is locked in that orientation.  Otherwise it is "unlocked".

Alive: if you'd prefer to fix this yourself, feel free to take it from Russ.  If you're busy with the rocketbar workweek, and think that Russ is on the right track here, your guidance is appreciated on this bug.

Comment 14

[Russ Nicoletti [:russn]]

Attachment: Suggested change

Hi Alive, I've attached a diff with my suggested change. I believe it's functionally equivalent to the existing code except for the difference in logic for getting the orientation.

Comment 15

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

Comment on attachment 8370534 [details]
Suggested change

Thanks for your patch.
I'd appreciate if you have a unit test for this change (see activity_window_test.js)
and file a github pull request then ask review.

Thanks again!

Comment 16

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

My 2cent:
If an activity page needs a specific orientation lock than the one in its manifest,
I wonder we could:
(1) add orientation in the activity object in manifest.
(2) let the activity to call the orientation when it's loaded.

But for now the patch is fine to go IMO.

Comment 17

[Russ Nicoletti [:russn]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/16062

Comment 18

[Russ Nicoletti [:russn]]

Alive, I added unit tests for the new logic. Although there are now only three test cases for the setOrientation function even though there are four decisions in the code to determine the orientation. This is because, from my reading of the code, it's not possible for this.manifest to be null while this.config.manifest is not null (and vice-versa) -- because the ActivityWindow constructor copies all the config attributes to the ActivityWindow object. Am I missing something? Is it necessary or useful to test this.manifest and this.config.manifest?

Comment 19

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

(In reply to Russ Nicoletti [:russn] from comment #18)
> Alive, I added unit tests for the new logic. Although there are now only
> three test cases for the setOrientation function even though there are four
> decisions in the code to determine the orientation. This is because, from my
> reading of the code, it's not possible for this.manifest to be null while
> this.config.manifest is not null (and vice-versa) -- because the
> ActivityWindow constructor copies all the config attributes to the
> ActivityWindow object. Am I missing something? Is it necessary or useful to
> test this.manifest and this.config.manifest?

Nice!
You are right that currently only app with manifest could have be an activity.
It's up to you to remove this.manifest checkage or not.

Comment 20

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

Comment on attachment 8371918 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16062

Awesome work and really quick catch up! Thanks for patching window management!

Comment 21

[Kevin Grandon :kgrandon]

Hey Russ - Just letting you know that all merges needs to have their commits squashed before merging. I noticed that ~4 patches landed as part of this bug, which will make any reverting or cherry-picking more difficult. You can do this with git rebase against master. Just letting you know for the future. NI? on you so you are aware. Thanks!

Comment 22

[Russ Nicoletti [:russn]]

Got it. Thanks, Kevin.