Closed Bug 1030954 Opened 10 years ago Closed 10 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S5 (4july)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: tif, Assigned: squib)

Details

(Whiteboard: interaction-design)

Attachments

(3 files)

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.
I think this should block 2.0 - the user should not have to kill apps to get functionality back.
Flags: needinfo?(hkoka)
Whiteboard: interaction-design
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.
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?
Flags: needinfo?(alive)
Tif: Lets discuss this in the quick meeting that Rob has setup on Monday
Flags: needinfo?(hkoka)
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.
Assignee: nobody → alive
Component: Gaia::System → Gaia::System::Window Mgmt
Flags: needinfo?(alive)
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.
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.
Attachment #8448139 - Flags: ui-review?(tshakespeare)
Attachment #8448139 - Flags: ui-review?(rmacdonald)
Attachment #8448139 - Flags: review?(dflanagan)
Attachment #8448139 - Flags: feedback?(alive)
For reference, Etienne has suggested that the proper fix for this bug (see comment 6) is bug 967405.
blocking-b2g: --- → 2.0+
(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.
(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.
(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?
Flags: needinfo?(etienne)
Assignee: alive → nobody
Component: Gaia::System::Window Mgmt → Gaia::Ringtones
Assignee: nobody → squibblyflabbetydoo
Attachment #8448139 - Flags: feedback?(alive) → feedback+
(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.
Flags: needinfo?(etienne)
(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.
Target Milestone: --- → 2.0 S5 (4july)
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 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().
Attachment #8448139 - Flags: review?(dflanagan) → review+
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 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.
Attachment #8448139 - Flags: ui-review?(tshakespeare) → ui-review+
Attachment #8448139 - Flags: ui-review?(rmacdonald) → ui-review+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Attachment #8448139 - Flags: approval-gaia-v2.0?
Attachment #8448139 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
Attached video 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
Flags: needinfo?(hlu)
Attached file logcat_flame_0506.txt
Hi Mike, Per talk, please help handle v2.1 bug verified by Marigold. Thank you.
Flags: needinfo?(hlu) → needinfo?(mlien)
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?
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(mlien)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: