1030954 - [ringtones][window management] create ringtone screen doesn't appear on subsequent tries

Status: VERIFIED FIXED

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

Description

[Tiffanie Shakespeare [:tif]]

STR: 
pre-condition: have music on device

1. Go to Settings -> Sound -> Manage Ringtones
2. Tap plus icon to start the create flow
3. Select any song from music picker
4. See create ringtone screen.
5. Press home button or otherwise navigate away from app
6. Go back to Settings app - observe the Ringtone Manager is displayed
7. Tap plus icon again
8. Pick song from music picker

Expected
Advance to Create Ringtone screen to finish creating a ringtone

Actual
Return to Ringtone Manager without seeing the create screen. It's not possible to return to this screen without killing the Settings app.

Comment 1

[Tiffanie Shakespeare [:tif]]

I think this should block 2.0 - the user should not have to kill apps to get functionality back.

Comment 2

[Tiffanie Shakespeare [:tif]]

Updating my Expected statement:

The ideal is that when the user leaves the create screen and returns the create screen is still visible instead of disappearing and showing the Ringtone Manager screen.

Comment 3

[Jim Porter (:squib)]

I think the issue is that when we call window.open to open the create screen, we're reusing the already-opened window from the first time. Ideally, the system would handle this for us by putting the create screen on top when the ringtones app regains visibility. However, we can just open a new window every time and work around the problem. That's a one-line fix.

Alive: Do you have any better ideas for what we should do here?

Comment 4

[Hema Koka [:hema]]

Tif: Lets discuss this in the quick meeting that Rob has setup on Monday

Comment 5

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

The rational is
1) Settings app uses inline activity to launch ringtone
2) Ringtone is using inner sheet to display the "create screen" page via window.open

I think we could try to fix this issue from system app side. No need to workaround in ringtone I guess.

Comment 6

[Jim Porter (:squib)]

Another wrinkle is that the ringtones app has a role of "system", so Etienne tells me that this means the ringtones app isn't in the window stack, which makes it harder to fix this case. We were talking about adding a new setting in the manifest that says "we want to be shown in the window stack, but we don't want an icon on the homescreen." That would probably fix a bunch of other issues with the ringtones app.

For 2.0, it might be better if we just focused on a workaround for this so that the system folks can just focus on the correct solution for 2.1. In 2.1, we need the "create" screen to remain on top when we go back to the ringtones manager*, but it's not strictly necessary for 2.0.

* In 2.1, the "create" screen will have a lot more state because we'll be adding the ability to crop songs. We don't want to lose that state if the app loses focus.

Comment 7

[Jim Porter (:squib)]

Attachment: Work around this for 2.0

Ok, here's a workaround for 2.0. It just closes the "create" screen when it loses visibility (but only if it was opened by the manager). I tried writing tests for this, but I can't actually get b2g-desktop to open the homescreen at the right time, making it impossible to reproduce the issue. :(

Alive: If you have a better solution for this that can go in 2.0, feel free to work on it. Otherwise, I think we're ok with waiting for 2.1 for a better fix.

Comment 8

[Jim Porter (:squib)]

For reference, Etienne has suggested that the proper fix for this bug (see comment 6) is bug 967405.

Comment 9

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

(In reply to Jim Porter (:squib) from comment #8)
> For reference, Etienne has suggested that the proper fix for this bug (see
> comment 6) is bug 967405.

Bug 967405 doesn't seem related to this issue.
The bug is talking about role=system should not or should appear in the stack,
but stack manager doesn't actively control which sheet to show in the app.

Comment 10

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9)
> (In reply to Jim Porter (:squib) from comment #8)
> > For reference, Etienne has suggested that the proper fix for this bug (see
> > comment 6) is bug 967405.
> 
> Bug 967405 doesn't seem related to this issue.
> The bug is talking about role=system should not or should appear in the
> stack,
> but stack manager doesn't actively control which sheet to show in the app.

Let me clarify:

StackManager is invented before the new window management architecture,
and it's not tracking all window instances since there are lots of new window introduced recently.

This bug should be fixed in AppWindowManager + StackManager.

Since inner sheet is only opened to ringtone app for now,
I don't really care we are using workaround for 2.0 or not,
but we need to fix it before opening to all apps.

Comment 11

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #10)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9)
> > (In reply to Jim Porter (:squib) from comment #8)
> > > For reference, Etienne has suggested that the proper fix for this bug (see
> > > comment 6) is bug 967405.
> > 
> > Bug 967405 doesn't seem related to this issue.
> > The bug is talking about role=system should not or should appear in the
> > stack,
> > but stack manager doesn't actively control which sheet to show in the app.
> 
> Let me clarify:
> 
> StackManager is invented before the new window management architecture,
> and it's not tracking all window instances since there are lots of new
> window introduced recently.
> 
> This bug should be fixed in AppWindowManager + StackManager.
> 
> Since inner sheet is only opened to ringtone app for now,
> I don't really care we are using workaround for 2.0 or not,
> but we need to fix it before opening to all apps.

I am trying to make a proper fix and I think the best and most simple way is to render window.open's child window in its opener container. But the problem is we will not have swipe navigation between opener and open-ee, unless we want to re-implement swipe navigation inside appWindow.

Based on above I agree we should workaround on v2.0 because it's not clear what to do yet.

Also, even we enable swipe navigation between parent window and child window,
does user really want to swipe between ringtone selector and create ringtone page?

My question is do we really want to use inner sheet in this case or we could just lazy load the create ringtone page in ringtone app instead of mis-use window.open?

Etienne, what do you think?

I feel inner sheet is not proper in this use case though.

Jim, could you confirm if we could avoid using window.open in 2.1?

Comment 12

[Etienne Segonzac (:etienne)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #11)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #10)
> > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9)
> > > (In reply to Jim Porter (:squib) from comment #8)
> > > > For reference, Etienne has suggested that the proper fix for this bug (see
> > > > comment 6) is bug 967405.
> > > 
> > > Bug 967405 doesn't seem related to this issue.
> > > The bug is talking about role=system should not or should appear in the
> > > stack,
> > > but stack manager doesn't actively control which sheet to show in the app.
> > 
> > Let me clarify:
> > 
> > StackManager is invented before the new window management architecture,
> > and it's not tracking all window instances since there are lots of new
> > window introduced recently.
> > 
> > This bug should be fixed in AppWindowManager + StackManager.
> > 
> > Since inner sheet is only opened to ringtone app for now,
> > I don't really care we are using workaround for 2.0 or not,
> > but we need to fix it before opening to all apps.
> 
> I am trying to make a proper fix and I think the best and most simple way is
> to render window.open's child window in its opener container. But the
> problem is we will not have swipe navigation between opener and open-ee,
> unless we want to re-implement swipe navigation inside appWindow.

Yeah I'd like to avoid that.
And I don't think we can ship a version of in-app sheets where the edge gesture to go back doesn't work. 

> 
> Based on above I agree we should workaround on v2.0 because it's not clear
> what to do yet.

Yes. We'll probably want to spec out all the cases before doing any implementation.

> 
> My question is do we really want to use inner sheet in this case or we could
> just lazy load the create ringtone page in ringtone app instead of mis-use
> window.open?

Yes that's a fair point. Outside of the 2.0 scope obviously, but I'm pretty sure this screen wasn't spec'ed as an in-app sheet.

Comment 13

[Jim Porter (:squib)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #11)
> My question is do we really want to use inner sheet in this case or we could
> just lazy load the create ringtone page in ringtone app instead of mis-use
> window.open?

What I want is for the "create ringtone" page to work like an activity when it's opened from the manager (i.e. from within the same app). I couldn't use a real activity, because then other apps could potentially hijack that activity name and cause a lot of confusion. window.open seemed like the closest approximation to what I wanted.  If there's a better way, I'd be happy to hear it.

Comment 14

[David Flanagan [:djf]]

Jim,

I think the underlying problem is that you were trying to use the same "create ringtone" page for both the share activity and for creating a ringtone after a pick activity. If that was a shared JavaScript module that created its UX as needed, then the two separate flows could just use the shared code to create their own create ringtone views.  What you're trying to do is share a single html file between the two cases and that is where it gets hacky.

I've never heard the phrase "in-app sheet" before and am not really following what Alive and Etienne are discussing, but maybe fore 2.1 we just need to refactor so that there is not a shared html file and you don't have to use window.open().

Comment 15

[David Flanagan [:djf]]

Comment on attachment 8448139 [details] [review]
Work around this for 2.0

If this workaround satisfies Tif and Rob, I'm fine with it from a technical perspective. If the user leaves the add ringtone flow before actually clicking the done button, they lose their place and have to go back and pick a song again, right?

For 2.1, with the addition of ringtone cropping, let's design the cropping and verification step as a shared module that can be used by both share and manage flows without actually sharing the html file and without using window.open().

Comment 16

[Jim Porter (:squib)]

Yeah, I thought I was being forward-thinking, since it was my understanding that the "create ringtone" screen from the manager was conceptually an in-app sheet. I guess not!

Comment 17

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8448139 [details] [review]
Work around this for 2.0

The issue seems fixed - I don't have to kill the settings app to get the create screen again. 

To be clear - it doesn't meet the ideal as stated in comment #2, but as discussed in meetings I think this behaviour is fine for 2.0 and we can improve in 2.1/2.2 to save state for when the cropping user story lands.

Comment 18

[Jim Porter (:squib)]

Landed: https://github.com/mozilla-b2g/gaia/commit/2296fed93a3842ffac335bdbc335e562a0967aa6

Comment 19

[Jim Porter (:squib)]

Comment on attachment 8448139 [details] [review]
Work around this for 2.0

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 960329
[User impact] if declined: Sometimes the "create ringtone" screen won't show up!
[Testing completed]: Manually tested (marionette tests weren't working)
[Risk to taking this patch] (and alternatives if risky): Low-risk
[String changes made]: None

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Paladin]

Attachment: Verify_video.3gp

This bug has been verified to fail on Flame 2.1
Steps:
1. Go to Settings -> Sound -> Manage Ringtones
2. Tap plus icon to start the create flow
3. Select any song from music picker
4. See create ringtone screen.
5. Press home button or otherwise navigate away from app
6. Go back to Settings app - observe the Ringtone Manager is displayed
7. Tap plus icon again
8. Pick song from music picker
** step 6: Return to Ringtone Manager without seeing the create screen.
 
See attachment: Verify_video.3gp and logcat_flame_0506.txt
Reproducing rate: 5/5
Flame 2.1 build:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0

Comment 22

[Paladin]

Attachment: logcat_flame_0506.txt

Comment 23

[Hubert Lu[:hlu] <[e-mail]>]

Hi Mike,
    Per talk, please help handle v2.1 bug verified by Marigold. Thank you.

Comment 24

[Jim Porter (:squib)]

I've watched the video in attachment 8529501 [details] and I don't see where the problem is. Everything appears to be behaving correctly. Am I missing something?

Comment 25

[Mike Lien[:mlien]]

Verified again with v2.1
Refer to comment 15 and comment 17, what the v2.1 result I saw is the same with workaround on v2.0
Do we have any plan to improve it on v2.1 or v2.2 to the ideal state like comment 2 mentioned?

[Build Information]
v2.0
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141126160203
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.192814
FW-Date         Wed Nov 26 19:28:25 EST 2014
Bootloader      L1TC10011880


v2.1
Gaia-Rev        5372b675e018b6aac97d95ff5db8d4bd16addb9b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b
Build-ID        20141126161202
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.193631
FW-Date         Wed Nov 26 19:36:42 EST 2014
Bootloader      L1TC10011880