817753 - [Home screen][camera] Not able to cancel camera option when setting a new wallpaper

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect, P3)

Description

[Natalya Kot [:nkot]]

1. Open Home screen
2. Press and hold background
3. Select Camera option
4. Press Home button to exit Camera w/o taking a picture

Expected result:
User is able to exit Camera

Actual result:
Not able to exit camera w/o taking picture

Comment 1

[Jason Smith [:jsmith]]

Smells like a blocker to me - we can't force the user to be stuck having to take an action - they should always be able to cancel it.

Comment 3

[bhsubram]

Difference between invoking the Gallery/Camera app directly by clicking on their icon and invoking the Gallery/Camera via the Pick menu that appears on the homescreen when you long-touch the homescreen:

First Workflow:
---------------

1. Go to homescreen, 
2. Open Gallery app directly
3. Exit to home screen by clicking the home button. 

The variable displayedApp at the end of step 2 is Gallery. The call trace here includes the following:

createFrame()
setDisplayedApp()
OpenWindow() (Here is where the displayedApp variable is set to Gallery)
home event handler (This invokes homescreen app if app is not Gallery)

Content JS LOG at app://system.gaiamobile.org/js/window_manager.js:1516 in anonymous: home event: displayedApp app://gallery.gaiamobile.org
Content JS LOG at app://system.gaiamobile.org/js/window_manager.js:1027 in appendFrame: In appendFrame calling createFrame

Second Workflow: (Bug)
----------------

1. Go to homescreen
2. hold the screen area to get the WallPaper Pick menu and click on gallery app
3. Exit to home screen by clicking the home button. Nothing happens.

The variable displayedApp is not set to Gallery at the end of step 2. This is the cause of the bug. The call trace here stops with createFrame(). Also another difference here is startInlineActivity calls createFrame.

Content JS LOG at app://system.gaiamobile.org/js/window_manager.js:1516 in anonymous: home event: displayedApp app://homescreen.gaiamobile.org
Content JS LOG at app://system.gaiamobile.org/js/window_manager.js:1065 in startInlineActivity: In startInlineActivity calling createFrame

Waiting to here from David Flanagan on how displayedApp variable should be set in the second workflow.

Comment 4

[David Flanagan [:djf]]

Bhuvana,

The two cases are quite different. In the second, the gallery is running as an inline activity, which is handled completely differently than a regular app. So I'm not surprised that displayedApp would be different. Interestingly, this select wallpaper from homescreen activity is different because it is launched from the homescreen app which is special.  So this is a really interesting analysis you did.  I think it is okay for displayedApp to be unset, but the bug is that we don't handle the home key correctly in the case where there is no displayedApp but there is an active inline activity.

This bug needs to be fixed, but it is a system app bug so (no offense intended Bhuvana) I'd suggest that someone with more Gaia experience would be better to work on it.  The system app is complicated and prone to regressions!

#817934 was the same bug as this one, but got closed. Alive was assigned to that bug, and he is probably the perfect person to do it because he has worked on something related very recently.

Alive: do you have time to take this bug?  If not, I can take it, though I haven't worked on the system app for awhile. Bhuvana: is that okay with you? Your analysis on this one is *very* helpful. Do you have other bugs you can work on if we take this one from you?

Also: the camera app probably should have a cancel button when invoked as an activity.  Dale: there is empty space where the gallery button would be. Can we put a cancel button there?  (I think I saw that discussed... Here's a blocking+ bug that presents the opportunity to make it happen if you want :-)

Comment 5

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

Sorry, I should take this bug instead of bug 817934 :)

Bhuvana: Your analysis is interesting, but we cannot set displayedApp at the wrong way. It's a public attribute in window manager for other modules in system app to query. The displayedApp is always set to an app, not the inline activity callee.
The same mechanism applies to attention screen, which is a special version of window.open, but could be invoked by a non-foreground, and even, not-running app. When attention screen or inline activity is up, the displayedApp is still the original foreground app. But it then may be hidden by setVisible(false) by window manager so itself knows it is not visible to the user now.

David Flanagan: Thanks for flagging me. This bug is due to I change the home button behavior in bug 812252, and the home button applies to homescreen goes through another code path which is not covered by bug 812252's patch. So this is my bug, exactly :) The way to resolve this bug is introduce stopInlineActivity() in the path of ensureHomescreen(). Surely this needs test I could not tell this method 100% works, but not hard.

Dale: Welcome to add a cancel button in camera app if you'd like to in this bug.

(In reply to David Flanagan [:djf] from comment #4)
> Bhuvana,
> 
> The two cases are quite different. In the second, the gallery is running as
> an inline activity, which is handled completely differently than a regular
> app. So I'm not surprised that displayedApp would be different.
> Interestingly, this select wallpaper from homescreen activity is different
> because it is launched from the homescreen app which is special.  So this is
> a really interesting analysis you did.  I think it is okay for displayedApp
> to be unset, but the bug is that we don't handle the home key correctly in
> the case where there is no displayedApp but there is an active inline
> activity.
> 
> This bug needs to be fixed, but it is a system app bug so (no offense
> intended Bhuvana) I'd suggest that someone with more Gaia experience would
> be better to work on it.  The system app is complicated and prone to
> regressions!
> 
> #817934 was the same bug as this one, but got closed. Alive was assigned to
> that bug, and he is probably the perfect person to do it because he has
> worked on something related very recently.
> 
> Alive: do you have time to take this bug?  If not, I can take it, though I
> haven't worked on the system app for awhile. Bhuvana: is that okay with you?
> Your analysis on this one is *very* helpful. Do you have other bugs you can
> work on if we take this one from you?
> 
> Also: the camera app probably should have a cancel button when invoked as an
> activity.  Dale: there is empty space where the gallery button would be. Can
> we put a cancel button there?  (I think I saw that discussed... Here's a
> blocking+ bug that presents the opportunity to make it happen if you want :-)

Comment 6

[Dale Harvey (:daleharvey)]

Cool, I will add the cancel button, if I havent done it by the time your patch is in can you assign it to me once yours is in? Cheers

Comment 7

[bhsubram]

Dale, Alive, just want to mention that this problem happens for all three of the homescreen wallpaper pick apps: Camera, Gallery and Wallpaper.

Comment 8

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 9

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

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

https://github.com/mozilla-b2g/gaia/pull/6846
Patch proposal:
Stop inline activity when pressing home key

Comment 10

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

Comment on attachment 689064 [details]
https://github.com/mozilla-b2g/gaia/pull/6846

r=me, thanks.

Comment 11

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

https://github.com/alivedise/gaia/commit/8b2bbf8e87e7ef5d2d998625dc135537905758d7
Switch the assignee to Dale.

Comment 12

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

(In reply to Alive Kuo [:alive] from comment #11)
> https://github.com/alivedise/gaia/commit/
> 8b2bbf8e87e7ef5d2d998625dc135537905758d7
> Switch the assignee to Dale.

Can you close this bug and clone another one for Dale? We don't land two commits with same bug #.