1027401 - Orientation lock is not restored after popup is closed

Status: RESOLVED FIXED

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

Description

[Montoya Clemmons (MClemmons)]

Attachment: Screenshot of bunched together text

User taps Contacts App, selects the gear icon and import Contacts button. After switching to landscape and selecting Gmail and logging in with valid credentials, the Found x friends to import screen displays the A-# list in the right margin in a manner that is difficult to read as the characters are bunched together. This is not the case in portrait mode. 

Prerequisites:
1. Have Wi-Fi and/or Data Connection enabled
2. Have active Gmail account 

Repro Steps:
1) Update a Flame to 20140618000202
2) Tap Contacts App and select gear icon in upper right
3) Select Import contacts and change the orientation of the device to landscape
4) Select Gmail and log in with valid credentials
5) Observe device behavior

Actual:
Found x friends to import screen displays the A-# list in the right margin in a manner that is difficult to read as the characters are bunched together. This is not the case in portrait mode. 

Expected:
A-# list of characters are spaced enough to be readable, similar to portrait mode.

Repro frequency: (10/10, 100%, etc.)
Link to failed test case: None. This was found during exploratory testing around an existing test case. 
See attached: 
a) screenshot 
b) Flame logcat unavailable due to https://bugzilla.mozilla.org/show_bug.cgi?id=1010993
c) Open C logcat provided instead of Flame logcat described in b) above

Environmental Variables:
Device: Flame 2.0
Build ID: 20140618000202
Gaia: 83844c7679b3b9f6e7f1116c1eeec2d1e7a64eec
Gecko: 55679dc2e72b
Version: 32.0a2 (2.0) 
Firmware Version: v121-2
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 1

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

This issue DOES reproduce on Flame 2.1, Buri2.1, Open C 2.1, Flame 2.0, Buri 2.0, and Open C 2.0

Flame 2.1

Environmental Variables:
Device: Flame Master
Build ID: 20140618040513
Gaia: 431aed0a7c7560c6eacd35ea69aa0a7a4ebe72c7
Gecko: 37f08ddaea48
Version: 33.0a1 (Master) 
Firmware Version: v121-2
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0


Open_C 2.1

Environmental Variables:
Device: Open_C Master
Build ID: 20140618040513
Gaia: 431aed0a7c7560c6eacd35ea69aa0a7a4ebe72c7
Gecko: 37f08ddaea48
Version: 33.0a1 (Master) 
Firmware Version: P821A10V1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0


Buri 2.1

Environmental Variables:
Device: Buri Master
Build ID: 20140618073003
Gaia: 336c30b6147cdd9122ad0b2bbffb81eb869a9ec2
Gecko: 1cea544c74c5
Version: 33.0a1 (Master) MOZ
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0


Buri 2.0

Environmental Variables:
Device: Buri 2.0
Build ID: 20140618063014
Gaia: 83844c7679b3b9f6e7f1116c1eeec2d1e7a64eec
Gecko: 883d156210cf
Version: 32.0a2 (2.0) MOZ
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0


Open_C 2.0

Environmental Variables:
Device: Open_C 2.0
Build ID: 20140618000202
Gaia: 83844c7679b3b9f6e7f1116c1eeec2d1e7a64eec
Gecko: 55679dc2e72b
Version: 32.0a2 (2.0) 
Firmware Version: P821A10V1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

The Alphabet Picker on the right side is squished together and hard to read when in landscape orientation
_________________________________________________________________________________________

This issue does NOT reproduce on Flame 1.4, Buri 1.4 or Open C 1.4

Buri 1.4

Environmental Variables:
Device: Buri 1.4
Build ID: 20140618063004
Gaia: fc74015d26bcbc3e31a45d34cb65777112a35982
Gecko: fab72d8aa2e0
Version: 30.0 (1.4) MOZ
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Flame 1.4

Environmental Variables:
Device: Flame 1.4
Build ID: 20140618000203
Gaia: 3bdd037ec1a11abebe16a5d7f6ff0d863e80bc07
Gecko: 523491fa3339
Version: 30.0 (1.4) 
Firmware Version: v121-2
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0


Open_C 1.4

Environmental Variables:
Device: Open_C 1.4
Build ID: 20140618000203
Gaia: 3bdd037ec1a11abebe16a5d7f6ff0d863e80bc07
Gecko: 523491fa3339
Version: 30.0 (1.4) 
Firmware Version: P821A10V1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

1.4 Devices do not change to landscape orientation when turning the device horizontally and accessing gmail

Comment 2

[Montoya Clemmons (MClemmons)]

Attachment: Landscape view issue with Imports

Comment 3

[KTucker [:KTucker][Inactive 3/4/2016]]

The actual issue here is that the user can go into landscape mode. This should be locked to portrait mode. Please change the title of the issue to reflect this.

Also, the regression and regressionwindow-wanted tags should be added here since this is a regression.

Comment 4

[Jason Smith [:jsmith]]

Kevin - This is missing a blocking triage analysis on the bug.

Comment 5

[KTucker [:KTucker][Inactive 3/4/2016]]

Nominating 2.0? since this is a regression from 1.4. This issue could lead to a more severe bug since the contacts app orientation is no longer locked in portrait mode.

Comment 6

[Pi Wei Cheng [:piwei] (inactive)]

b2g-inbound Regression Window:

Last Working Environmental Variables:
Device: Flame
Build ID: 20140424023002
Gaia: 4a4a078768423426c7c0fc580c8642bc735033a5
Gecko: 91946180b1e7
Version: 31.0a1 (Master)
Firmware Version: v122

First Broken Environmental Variables:
Device: Flame
Build ID: 20140424053001
Gaia: 4b6c9d4929c57005ab142e8eb3dc6783d55c427c
Gecko: c30df002e626
Version: 31.0a1 (Master)
Firmware Version: v122

Last Working Gaia / First Broken Gecko: Issue Does NOT reproduce
Gaia: 4a4a078768423426c7c0fc580c8642bc735033a5
Gecko: c30df002e626

Last Working Gecko / First Broken Gaia: Issue DOES reproduce
Gaia: 4b6c9d4929c57005ab142e8eb3dc6783d55c427c
Gecko: 91946180b1e7

Gaia Pushlog:
https://github.com/mozilla-b2g/gaia/compare/4a4a078768423426c7c0fc580c8642bc735033a5...4b6c9d4929c57005ab142e8eb3dc6783d55c427c

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

Hi,

I've been checking this and code in contacts didn't change at all. Looking at comment 6 for the regression window, there was a change in the window manager affecting the popupmanager.

That contacts list is being opened in a window.open, so I guess the popupmanager within the window manager is not honoring the orientation in the manifest for the app.

Alive should this be a system app bug? Or we should modify the orientation in the manifest.

Thanks!

Comment 8

[Jason Smith [:jsmith]]

bug 916709 broke this.

Comment 9

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

Interesting.

I and Francisco had some discussion on IRC and AFAIK we don't have perfect solution for this kind of issues. Sometimes the opened page don't want to be orientation-free and sometimes it would like to.

Yes, we could lock the orientation according to the app opens the page,
but I'd like not to block on this issue because I am afraid one day there would be another bug talking about: "why the page opened by window.open cannot be rotated?"

Comment 10

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

More clearly, I don't think this is a blocker - unless
1. User cannot import contact when stay landscape mode
2. When user finish/cancels the import and back to contact app, the contact app becomes orientation-free.

Comment 11

[Francisco Jordano [:arcturus] [:francisco]]

I do agree with :alive here, not a real blocker unless we cannot import contacts.

Is not a trivial solution, since we could break other stuff, it will require several investigation and sharing with people to be aware of possible new behavior.

Can we unblock?

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9)
> I and Francisco had some discussion on IRC and AFAIK we don't have perfect
> solution for this kind of issues. Sometimes the opened page don't want to be
> orientation-free and sometimes it would like to.
> 
> Yes, we could lock the orientation according to the app opens the page,
> but I'd like not to block on this issue because I am afraid one day there
> would be another bug talking about: "why the page opened by window.open
> cannot be rotated?"

Let's WONTFIX this bug. Sounds to me that having the popup window rotatable is a new feature and this bug is literally describing that as a regression.

Comment 13

[Jason Smith [:jsmith]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #12)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9)
> > I and Francisco had some discussion on IRC and AFAIK we don't have perfect
> > solution for this kind of issues. Sometimes the opened page don't want to be
> > orientation-free and sometimes it would like to.
> > 
> > Yes, we could lock the orientation according to the app opens the page,
> > but I'd like not to block on this issue because I am afraid one day there
> > would be another bug talking about: "why the page opened by window.open
> > cannot be rotated?"
> 
> Let's WONTFIX this bug. Sounds to me that having the popup window rotatable
> is a new feature and this bug is literally describing that as a regression.

No. Visual refresh is a critical requirement for 2.0. Anything that involves bunching up a bunch of letters to the point they are not readable is not something we can ship with. Reopening.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Let's confirm with UX on whether or not comment 12 is a desired behavior change or not.

(In reply to Jason Smith [:jsmith] from comment #13)
> No. Visual refresh is a critical requirement for 2.0. Anything that involves
> bunching up a bunch of letters to the point they are not readable is not
> something we can ship with. Reopening.

I don't understand what is not readable in this comment...

Comment 15

[Francisco Jordano [:arcturus] [:francisco]]

Hi folks, 

I think that visually the problem is with the shortcut letters on the right, used to jump from letter to letter.

I was about to suggest a solution in contacts, based on media queries we just hide this control if we are in landscape and don't have space to show this.

But testing it, I realise about a bigger problem, once you one a window a go to landscape mode, if you close the window in landscape mode, the opener app, goes to landscape mode, which to me is a blocker.

Just recorded a video:

https://drive.google.com/file/d/0B3RfQh7fuZjAaWpoYVlXc2hnSkU/edit?usp=sharing

As per conversation with Alive, I'm unassigning myself, since I guess he would like to take over this.

Thanks!
F.

Comment 16

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

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #15)
> Hi folks, 
> 
> I think that visually the problem is with the shortcut letters on the right,
> used to jump from letter to letter.
> 
> I was about to suggest a solution in contacts, based on media queries we
> just hide this control if we are in landscape and don't have space to show
> this.
> 
> But testing it, I realise about a bigger problem, once you one a window a go
> to landscape mode, if you close the window in landscape mode, the opener
> app, goes to landscape mode, which to me is a blocker.
> 
> Just recorded a video:
> 
> https://drive.google.com/file/d/0B3RfQh7fuZjAaWpoYVlXc2hnSkU/edit?usp=sharing
> 
> As per conversation with Alive, I'm unassigning myself, since I guess he
> would like to take over this.
> 
> Thanks!
> F.

This should be fixed but I am still questioning the opened window should be orientation free.
Maybe we need to open another bug to fix the regression you found. Waiting UX decision.

Comment 17

[Francisco Jordano [:arcturus] [:francisco]]

Pinging Carrie, UX owner for contacts.

Carrie, do you think that an acceptable solution could be hide the shortcuts on the right if we detect that we don't have space to display them?

So we could fix this bug, and as Alive comments, open another one to solve the problem in comment 15

Comment 18

[Stephany Wilkes]

Carrie is already flagged so removing general UX flag.

Comment 19

[Carrie Wang [New account: [e-mail]]]

My suggestion would be, if we are going to provide the landscape in this case, we should have a proper alphabet picker for users since the space to show the list is really really small, users may rely on this indicator very much. 
However, I'd go with comment 16 that we shouldn't provide the orientation-free opened window in Contacts. Because the Contacts APP doesn't have landscape mode, I think we shall keep it consistent. Can we apply this solution? If not, then we'll need VD to provide visual spec for this landscape alphabet picker. Thanks!

Comment 20

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Carrie Wang [:carrie] from comment #19)
> My suggestion would be, if we are going to provide the landscape in this
> case, we should have a proper alphabet picker for users since the space to
> show the list is really really small, users may rely on this indicator very
> much. 
> However, I'd go with comment 16 that we shouldn't provide the
> orientation-free opened window in Contacts. Because the Contacts APP doesn't
> have landscape mode, I think we shall keep it consistent. Can we apply this
> solution? If not, then we'll need VD to provide visual spec for this
> landscape alphabet picker. Thanks!

Carrie, Alive is looking at a solution for not displaying the landscape. Don't know if we'll manage to have it or not, but in case we are in landscape finally for that screen, IMHO, the alphabet will be unusable even if we properly display it, since it will be super small, I'm sure we'll end up touching several letters cause of the space.

Comment 21

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

Hmmm...wait, do I misunderstand? Isn't the page is under google? What's different if we are using browser app to import google contact? Should we block orientation in this case? How do we know we are opening a web page which is not orientation-free-friendly?

Comment 22

[Francisco Jordano [:arcturus] [:francisco]]

Right the page is free orientation friendly, the problem is that we are keeping the orientation to the app, but that's a different thing, right?

Should we close this, and keep just the one (don't have the bug number), for the orientation changing after a opened window is changing orientation?

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

After inspecting System app with App Manager, Alive found that the screenshot depicted in comment 0 is NOT the popup, but the import page in Contact iframe after the pop-up is closed. This is indeed a valid regression, not a special use case.

Sorry for the confusion everyone.

Comment 24

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

Root cause
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L1819 doesn't work.

The popupclosing is dispatched in the popup element due to bug 1024168,
but in AppWindow.prototype.handleEvent, it's stopping the propagation of all events of the front window.
This should be corrected.

Comment 25

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

Attachment: patch for master, v1

Change event handling mechanism again:
* Do not propagate any mozbrowser events unless we have rearWindow
* Do removing rear/front/next/previous only on destroyed
* Only care about child's event

Comment 26

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #25)
> Created attachment 8451546 [details] [review]
> patch for master, v1
> 
> Change event handling mechanism again:
> * Do not propagate any mozbrowser events unless we have rearWindow
> * Do removing rear/front/next/previous only on destroyed
> * Only care about child's event

The way injecting popupclosing/activityclosing to each window seems stupid,
maybe we should do it in the child_window_factory.

Comment 27

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #26)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #25)
> > Created attachment 8451546 [details] [review]
> > patch for master, v1
> > 
> > Change event handling mechanism again:
> > * Do not propagate any mozbrowser events unless we have rearWindow
> > * Do removing rear/front/next/previous only on destroyed
> > * Only care about child's event
> 
> The way injecting popupclosing/activityclosing to each window seems stupid,
> maybe we should do it in the child_window_factory.

But a little out of scope so I plan to do it in another bug.

Comment 28

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

Comment on attachment 8451546 [details] [review]
patch for master, v1

v2: I end up moving closing handler into child window to reduce duplicate work.
Also it prevents popup is listening to its own popupclosing event.

Comment 29

[Etienne Segonzac (:etienne)]

Comment on attachment 8451546 [details] [review]
patch for master, v1

Makes sense!
(tiny nit on github)

Comment 30

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

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/4e3f949226e82938aa8ce0fa9c7d29f36243afde

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Reverted for Gaia unit test failures.
v2.0: https://github.com/mozilla-b2g/gaia/commit/d1e3336ddd4510a67e070766a27a6c868270f1b0

https://tbpl.mozilla.org/php/getParsedLog.php?id=43451804&tree=Mozilla-Aurora

Comment 33

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

Looking..

Comment 34

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

Strange, the same patch works for me locally.

Comment 35

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

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=63afa4cb5e23

Wait and see.

Comment 36

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #35)
> https://tbpl.mozilla.org/?tree=Gaia-Try&rev=63afa4cb5e23
> 
> Wait and see.

Gu passes
https://tbpl.mozilla.org/php/getParsedLog.php?id=43490957&tree=Gaia-Try

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Very odd, it was perma-failing on Aurora yesterday until the revert landed.

Comment 38

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #37)
> Very odd, it was perma-failing on Aurora yesterday until the revert landed.

I am going to merge again, please backout if still happens due to the merge.
https://github.com/mozilla-b2g/gaia/commit/18c44a1bc31b374ba00a069904465a8d07971a60