Bugzilla

Assignee

Comment 5

•

10 years ago

(In reply to Gregor Wagner [:gwagner] from comment #3)
> Can lead to undefined behavior.
> Sam, can you take a look?

Yes, I'm looking at this today. There's a lot that is new to me in there, but its all stuff I need to figure out anyhow.

Flags: needinfo?(sfoster)

Assignee

Comment 6

•

10 years ago

Here's what I've got so far. When you click on the Import Facebook Contacts link in the FTU, a PopupWindow is created so you can login to Facebook. It is correctly linked to the FTU AppWindow by passing a rearWindow reference to it. It gets rendered inside the FTU's window. 
If you click on the 'Firefox OS' link in the facebook page, it creates a new AppWindow instance to load up oauth.gaiamobile.org, which errors. The 'try again' just reloads the error page. 'close' closes the window, which dumps you back to the homescreen. The FTU app is still running, but because you cant see it in the task manager, there's no way to get back to it. 

There's a couple of problems here: 
1) The 'Server not found' error in the oauth window. I'm not sure what exactly is supposed to happen here, but its obviously wrong. Maybe if it was working right, we wouldn't see the original problem in the first place.

2) The new AppWindow isn't linked in anyway to the FTU. When it closes we have no context and there's no check to prevent the default behavior of opening the homescreen. Maybe this window should be another PopupWindow with the rearWindow ref pointing back to FTU window? I'm not sure how to make that happen as I cant see at present how this window is being created.

Flags: needinfo?(alive)

Comment 7

•

10 years ago

(In reply to Sam Foster [:sfoster] from comment #6)
> Here's what I've got so far. When you click on the Import Facebook Contacts
> link in the FTU, a PopupWindow is created so you can login to Facebook. It
> is correctly linked to the FTU AppWindow by passing a rearWindow reference
> to it. It gets rendered inside the FTU's window. 
> If you click on the 'Firefox OS' link in the facebook page,

I don't see this link, where is it? Any screenshot?

 it creates a new
> AppWindow instance to load up oauth.gaiamobile.org, which errors. The 'try
> again' just reloads the error page. 'close' closes the window, which dumps
> you back to the homescreen. The FTU app is still running, but because you
> cant see it in the task manager, there's no way to get back to it. 
> 
> There's a couple of problems here: 
> 1) The 'Server not found' error in the oauth window. I'm not sure what
> exactly is supposed to happen here, but its obviously wrong. Maybe if it was
> working right, we wouldn't see the original problem in the first place.

This should be a question to the FTU owner or facebook.

> 
> 2) The new AppWindow isn't linked in anyway to the FTU. When it closes we
> have no context and there's no check to prevent the default behavior of
> opening the homescreen. Maybe this window should be another PopupWindow with
> the rearWindow ref pointing back to FTU window? I'm not sure how to make
> that happen as I cant see at present how this window is being created.

I have some idea, (check ftu is running in AppWindowFactory/ChildWindowFactory to do something).
But the 'oauth.gaiamobile.org' sounds to me an invalid url anyway. Could you tell me what is expected to be done here?

Flags: needinfo?(alive)

Assignee

Updated

•

10 years ago

Blocks: 1072043

Assignee

Updated

•

10 years ago

No longer blocks: 1072043

Assignee

Comment 8

•

10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #7)
> (In reply to Sam Foster [:sfoster] from comment #6)
> > Here's what I've got so far. When you click on the Import Facebook Contacts
> > link in the FTU, a PopupWindow is created so you can login to Facebook. It
> > is correctly linked to the FTU AppWindow by passing a rearWindow reference
> > to it. It gets rendered inside the FTU's window. 
> > If you click on the 'Firefox OS' link in the facebook page,
> 
> I don't see this link, where is it? Any screenshot?

There's a youtube video of the problem at http://youtu.be/XKOo9j611Kg


> > 2) The new AppWindow isn't linked in anyway to the FTU. When it closes we
> > have no context and there's no check to prevent the default behavior of
> > opening the homescreen. Maybe this window should be another PopupWindow with
> > the rearWindow ref pointing back to FTU window? I'm not sure how to make
> > that happen as I cant see at present how this window is being created.
> 
> I have some idea, (check ftu is running in
> AppWindowFactory/ChildWindowFactory to do something).
> But the 'oauth.gaiamobile.org' sounds to me an invalid url anyway. Could you
> tell me what is expected to be done here?

Yes, lets ignore the specifics for now. You're right, we shouldn't be seeing this error, and it may be an artifact of the build and the facebook app id being used. The point is that its possible to get into a state where the current window is not linked to the FTU and closing it dumps you into the homescreen. I'm not having any luck catching where the window is created in the debugger.

Assignee

Comment 9

•

10 years ago

Franciso, can you confirm if this oauth.gaiamoible.org server not found is a legitimate error, or an artifact of how we are testing (see the youtube link). What /should/ be happening here? 
Are you aware of other ways to get into the same situation in the FTU app? I.e. do we need a generalized solution in the windows management code to these unattached windows in FTU flow? 

Also, if/when I find a solution for this, I'm not sure how to test it. Do I just need the MOZILLA_OFFICIAL build flag? Testing with my local builds I get an app id error when I click the Import Facebook Contacts link.

Flags: needinfo?(francisco)

Assignee

Comment 10

•

10 years ago

Attached file Create child/dialog windows while FTU is running (obsolete) — Details

I'm tackling this issue by asserting that whatever weird thing happens to open an new non-child window from the FTU, when it closes we should go back to the FTU. To accomplish this I've augmented the logic in AppWindowManager.display to default to the FTU app window when FtuLauncher.isFtuRunning() is true - rather than defaulting to the homescreen. 

The FtuLauncher does not keep or provide any way to retrieve the FTU's AppWindow - unlike HomescreenLauncher. And I didn't want to introduce a dependency on AppWindowManager in FtuLauncher. So I'm left with this slightly messy loop over this._apps in the AWM display method. I'm open to suggestions for how to refactor. 

Note that I'm not addressing the issue that reveals this bug - the oauth.gaiamobile.org server not found dialog. I'm leaving the ni? for francisco for clarification there. I think however we arrive in this kind of state, this patch lets us recover without leaving the user in the lurch. We can open a follow-up non-blocking FTU or Contacts bug to track that down if necessary. 

This is a 2.1 blocker, it looks like it will merge cleanly.

Attachment #8496276 - Flags: review?(alive)

Assignee

Comment 11

•

10 years ago

Comment on attachment 8496276 [details] [review]
Create child/dialog windows while FTU is running

Cancelling review while I work out a head-slapping issue with completing the FTU.

Attachment #8496276 - Flags: review?(alive)

Assignee

Comment 12

•

10 years ago

Comment on attachment 8496276 [details] [review]
Create child/dialog windows while FTU is running

Ok we're back on. Also, I did my homework and found AWM getApp() so this is a somewhat cleaner patch. Still open to refactoring suggestions -  bearing in mind this is a 2.1 blocker and needs to not mushroom too much in scope. We can always follow-up.

Attachment #8496276 - Flags: review?(alive)

Comment 13

•

10 years ago

Comment on attachment 8496276 [details] [review]
Create child/dialog windows while FTU is running

It depends on what we really want.
IMO we should just not let the user moving from FTU to any other app. This is an UX change, I hope UX could confirm before we go.

An option is checking System.isRunningFTU inside childWindowFactory, then override the browser src. What the user will see is the popup will change to the fxos website and could easily closed by close button.

Attachment #8496276 - Flags: ui-review?(firefoxos-ux-bugzilla)

Comment 14

•

10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #13)
> Comment on attachment 8496276 [details] [review]
> Default to FTU in AWM.display when FTU is running
> 
> It depends on what we really want.
> IMO we should just not let the user moving from FTU to any other app. This
> is an UX change, I hope UX could confirm before we go.
> 
> An option is checking System.isRunningFTU inside childWindowFactory, then
> override the browser src. What the user will see is the popup will change to
> the fxos website and could easily closed by close button.

I think we are confusing the user by the home button.

After v2.1, pressing home button has two kind of results:
* Return to homescreen app while there is no task manager opened.
* Return to the last opened app while there is task manager opened.

Now we are adding more rules to home button.
* Return to ftu is the user accidently switch to another app during FTU.

Why not call it 'magic' button ? :)

Flags: needinfo?(firefoxos-ux-bugzilla)

Assignee

Comment 15

•

10 years ago

> I think we are confusing the user by the home button.
> 
> After v2.1, pressing home button has two kind of results:
> * Return to homescreen app while there is no task manager opened.
> * Return to the last opened app while there is task manager opened.

This has been reverted for 2.1 in bug 1071852

> 
> Now we are adding more rules to home button.
> * Return to ftu is the user accidently switch to another app during FTU.

TBH, it was not my intention to do this, but I see how that will work - home button will call AWM.display(null). I can try to fix that, but I think this is actually what we want, right? Unless we try and prevent those windows from opening - which is hard to know which the user should and shouldn't see (we had this same discussion about the dialer app interupting FTU) the only thing we can do is ensure the user can close those windows and get back to the FTU. 

> 
> Why not call it 'magic' button ? :)

I think of it as an escape hatch. Like a root window or base camp. 'Home' is a concept. Homescreen is the default 'home' once FTU is complete. Anyhow, I'm happy to talk about fixing our metaphors for 2.2 but for 2.1 we are just plugging up these gaps. I get that you are uncomfortable with this patch, what would you prefer to see?

Assignee

Comment 16

•

10 years ago

Flagging for ni? on comment #15

Flags: needinfo?(alive)

Stephany Wilkes

Comment 17

•

10 years ago

Flagging Jacqueline on FTU.

Flags: needinfo?(firefoxos-ux-bugzilla)

Stephany Wilkes

Updated

•

10 years ago

Attachment #8496276 - Flags: ui-review?(firefoxos-ux-bugzilla) → ui-review?(jsavory)

Stephany Wilkes

Updated

•

10 years ago

Attachment #8496276 - Flags: ui-review?(jsavory) → ui-review?(msandberg)

Comment 18

•

10 years ago

Hi there - filling in for Jacqueline this week while she is on PTO. 

I'm not exactly sure how Sam's solution is tied to the home button technically. Is it the case that setting the default in AMW.display to FTU while the FTU is running affects the home button behavior? 

Currently tapping the home button during FTU has no effect (apart from vibration feedback) and I'm guessing that is by design so that the user can not skip out of the FTU. Can we keep it this way but still incorporate Sam's change to the AMW.display default?

Assignee

Comment 19

•

10 years ago

> > Now we are adding more rules to home button.
> > * Return to ftu is the user accidently switch to another app during FTU.
> 
> TBH, it was not my intention to do this, but I see how that will work - home
> button will call AWM.display(null). 

Actually, I don't see this. The 'home' event listener in FTULauncher intercepts the event and calls stopImmediatePropagation(). My patch doesnt currently change this behavior, So I don't understand this comment. 

That said, I think the home button is a vital escape hatch for this and lots of other scenarios in the FTU where the user can inadvertently end up in a new browser window with no way to get back. To be clear, as far as I can tell we don't need to change home button behavior to fix this FTU/import facebook contacts bug. But there are others (e.g. the 'privacy' link from the login page you get from 'Import Gmail' contacts button) where there is no close button, no task-manager yet and no way out apart from restart.

I can open a new ticket for this - its a separate issue really. But its the same argument - should FTU actively block app/browser windows somehow (how?), allow them but ensure they are close-able popup windows that return to FTU not homescreen when closed, or allow them but hook up the home button to take you back to the front FTU window while the FTU is running. We need to answer this question for 2.1 and 2.2/master.

Comment 20

•

10 years ago

(In reply to Sam Foster [:sfoster] from comment #19)
> I can open a new ticket for this - its a separate issue really. But its the
> same argument - should FTU actively block app/browser windows somehow
> (how?), allow them but ensure they are close-able popup windows that return
> to FTU not homescreen when closed, or allow them but hook up the home button
> to take you back to the front FTU window while the FTU is running. We need
> to answer this question for 2.1 and 2.2/master.

Ideally I think we should go with the middle option here. Any window leading away from FTU should have an on-screen escape of some sort, following our UX patterns (likely a close/x button). The home button should have no effect during the FTU (pressing it does nothing). That way we aren't overloading the home button, and we ensure users can't be put in a situation without a clear way back. 

Does that make sense?

Comment 21

•

10 years ago

Sam,

I don't want to frustrate you on a blocker, but what I want to see is we have a cooperation with UX and make sure they are aware of this case before we rush merging any blocker to avoid more bugs in the future.

It works for me we come up with a temporary solution that both UX and developer understand at this point,
but we should not introduce a hidden feature like this without letting UX be aware of that. Even they may not figure out a proper solution now.

After thinking again, I tend to think this is NOT a regression.

Could you cooperate with UX here?

That is to say, if we don't come up with a solution with UX, I will r+ your original proposal but hope there's a bug opened for master as long term solution investigation.

Does this work for you?

Flags: needinfo?(alive)

Comment 22

•

10 years ago

Comment on attachment 8496276 [details] [review]
Create child/dialog windows while FTU is running

Canceling before final decision.

Attachment #8496276 - Flags: review?(alive)

Assignee

Comment 23

•

10 years ago

Maria and I talked (she's standing in for Jacqueline who normally owns the FTU UX). I think the home button discussion is a distraction here, I'm willing to drop it. From UX' point of view, the home button should basically do nothing for the duration of the FTU. 

To fix this bug, we want to ensure that any windows opened by the content in these import/login screens:

1) can be closed (we have no task manager at this point to close windows from so we need popup/dialog windows with their own close button), 

2) when closed drop you back into the FTU. I'm still studying the window management and creation code, and at this point I'm not yet clear how to accomplish this. If I've not figured it out by later today I'll ping Alive for help.

Assignee

Comment 24

•

10 years ago

Comment on attachment 8496276 [details] [review]
Create child/dialog windows while FTU is running

So it looks like to ensure any windows created while in the FTU are closable, we'll need to hijack ChildWindowFactory's handleEvent and call createPopupWindow rather than createNewWindow. Something like: https://github.com/sfoster/gaia/commit/27f182cf53d362dae62ad92a85d6962e14f990b4 (not tested)

Obviously some windows - like the incoming call - should be left as-is. I still need to make sure FtuLauncher.isFtuRunning() will be accurate and not race. But mostly what worries me here is that I don't know what I don't know. 

I'm clearing the ui-review flag, see comment #23

Attachment #8496276 - Attachment description: Default to FTU in AWM.display when FTU is running → Create child/dialog windows while FTU is running

Attachment #8496276 - Flags: ui-review?(msandberg) → feedback?(alive)

Comment 25

•

10 years ago

Comment on attachment 8496276 [details] [review]
Create child/dialog windows while FTU is running

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/system.js#L75
It's better than access the global Object FtuLauncher

I'd recommend to comment something in the code to describe why we need to check Ftu isRunning

Attachment #8496276 - Flags: feedback?(alive) → feedback+

Assignee

Comment 26

•

10 years ago

Attached file Open _blank windows as popups when FTU is running (obsolete) — Details

I've tested this with the STR on this bug, also similar cases in the gmail import contacts content. I also tested an incoming call while in the FTU. Are there other test cases and potential regressions I can cover before requesting 2.1 approval?

Attachment #8496276 - Attachment is obsolete: true

Attachment #8498353 - Flags: review?(alive)

Assignee

Comment 27

•

10 years ago

Marca, any early input you have on testing and verification would be appreciated. With the limited time left for 2.1 approvals, I'll take any help I can get in identifying regression risk early and nailing this down properly before uplift.

Flags: needinfo?(mozillamarcia.knous)

Comment 28

•

10 years ago

Comment on attachment 8498353 [details] [review]
Open _blank windows as popups when FTU is running

I don't think incoming call is related to this issue? It is an attention window.

Attachment #8498353 - Flags: review?(alive) → review+

Candice Serran (:cserran)

Comment 29

•

10 years ago

Sam,

I've looked at the patch and haven't been able to break it from a UX point of view. There are situations where I've been able to spawn a new window from the browser window (when going through the gmail flow) but even then the close button works and there is no path out but to return to FTU. 

I did notice an inconsistency with the windows in add contacts and the Fx Accounts sign up. If I tap the terms of service link for example from the Fx Accounts sign up page, a new window is spawned but comes in from the top and bottom at the same time. (Header part from top, rest from bottom). This looks like a different type of window to me. Not only is the transition in different, so is the transition out of it (fades out on close instead of animating down). It doesn't have a loading indicator and the header in these windows lacks padding and ends up overlaying the status bar. Ideally we'd want the same behavior from these windows as the ones used for the contacts import scenarios. 

This may belong in a different bug or have other complications I'm unaware of (just filling in for Jacqueline) but I wanted to bring it up in case it's something we haven't noticed before. Let me know how you'd like me to deal with it :)

Updated

•

10 years ago

Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)

Assignee

Comment 30

•

10 years ago

Hi Maria, thanks for trying this out. I see the issue you describe. The FxA external links are using an EntrySheet vs. the PopupWindow used elsewhere. I'm trying to find out more about this. But I think this is a good candidate for a follow-up. This bug blocks 2.1, I think we should take a separate decision over whether the different animation and behavior of these windows is a blocker?

Flags: needinfo?(msandberg)

Comment 31

•

10 years ago

Yep, that makes sense to me. We can break the FxA issue into a new bug if that's better :)

Flags: needinfo?(msandberg)

Assignee

Comment 32

•

10 years ago

Attached file Re-opened PR — Details

New PR as old one was closed in the purge. Carrying alive's r+.

Attachment #8498353 - Attachment is obsolete: true

Attachment #8499820 - Flags: review+

Assignee

Comment 33

•

10 years ago

Merged into master: https://github.com/mozilla-b2g/gaia/commit/d418d3d86a3e6471c902d39966df02d04d77c158
Commit: https://github.com/mozilla-b2g/gaia/commit/69130dc59981aa7930774692dbc137baa289c382

We'll verify on master before requesting approval for 2.1

Status: REOPENED → RESOLVED

Closed: 10 years ago → 10 years ago

Resolution: --- → FIXED

Assignee

Updated

•

10 years ago

Blocks: 1077688

Assignee

Comment 34

•

10 years ago

Filed follow up bug 1077688 to address comment #29
Clearing ni flags for Marcia. Franciso, I could still use your input on that comment #9

Francisco Jordano [:arcturus] [:francisco]

Assignee

Updated

•

10 years ago

Flags: needinfo?(mozillamarcia.knous)

Updated

•

10 years ago

Flags: needinfo?(francisco)

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

Reporter

Comment 37

•

10 years ago

This issue DOES reproduce on todays Flame 2.2, but only if the FTU is launched from the developer options.

If the user taps on "cancel" they will be brought to the homescreen. This behaviour is different than the behavior mentioned in the original post. Because the FTU was launched from the developer options, it behaves as an app and will be closed, rather than skipped. This means that the home button will still be functional as expected.

Flame 2.2 KitKat Base (319mb)(Full Flash)

Environmental Variables:
Device: Flame 2.2 Master
BuildID: 20141007040205
Gaia: 83de447d9ae9a59459d7a445f9348a254c661850
Gecko: 9ee9e193fc48
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

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

Assignee

Comment 38

•

10 years ago

(In reply to Derek Harris [:DerekH] from comment #37)
> This issue DOES reproduce on todays Flame 2.2, but only if the FTU is
> launched from the developer options.

This is difficult, we could arrange for window.System.runningFTU to be true when you start the FTU from the developer menu which would fix this issue. But running the FTU from the developer menu will never truly be the same as when it runs from a reset/first-run. Would you mind opening a new bug for this? It is an edge case which we should get to, but almost certainly not a 2.1 blocker.

Flags: needinfo?(dharris)

Reporter

Comment 39

•

10 years ago

Yes I will open a separate bug to track this issue

Flags: needinfo?(dharris)

Marcia Knous [:marcia]

Comment 40

•

10 years ago

I tested this as well and it looks good on the trunk. I see the same results as Derek does in Comment 37. I think one can be uplifted based on our testing results.

Verified fixed on Flame using

Gaia   83de447d9ae9a59459d7a445f9348a254c661850
SourceStamp 9ee9e193fc48
BuildID 20141007040205
Version 35.0a1
base: v180

Status: RESOLVED → VERIFIED