915039 - [Video] There is no easy way to navigate back to the activity caller if the library is empty

Status: RESOLVED FIXED

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

Description

[xiupinglong]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.22 (KHTML, like Gecko) Ubuntu Chromium/25.0.1364.160 Chrome/25.0.1364.160 Safari/537.22

Steps to reproduce:

 1.Open the message composer
 2.Ensure that there are no video on the device
 3.Try to attach a video file to the message
 4.A message is shown basically stating that no videos are available and this message overlays the back button at the top of the screen which can therefore not be selected
 5.Home button needs to be used which will move the MMS composer into the background


Actual results:

There is no easy way to navigate back to the MMS composer if an empty category has been selected to attach an item.


Expected results:

 Need add a back key

Comment 1

[Aras Balali Moghaddam]

Attachment: bug915039-back-button-for-open-video-activity-when-n.patch

Asking John Hu to review this patch since he has done simillar fixes. Here is the link to PR on github: https://github.com/mozilla-b2g/gaia/pull/12258

I usually attach html with autoredirect to PR; but someone mentioned to me that it is ok to attach the patch here instead. Let me know if I can improve my patch submission process in any way. Thanks!

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

In gaia, we always use the PR which is auto redirected by html. Thanks for providing both. To have a patch is helpful to apply the patch in my local branch.

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 805762 [details] [diff] [review]
bug915039-back-button-for-open-video-activity-when-n.patch

Aras,

Thanks for taking this bug. I suggest we view this bug more broadly.

When dealing with pick activity with blocking overlay, we got some situations that user have no way to go back to previous state or to do something to hide the overlay, like no sdcard, no video, mounted, etc.

We have the following cases:

 error case       pick          buttons
--------------------------------------------
 no sdcard         no          media storage settings 
 no sdcard         yes         media storage settings [close]
 usb plugged in    no          none
 usb plugged in    yes         none [close]
 empty list        no          none
 empty list        yes         none [close]
 upgrading         no          none (waiting is fine)
 upgrading         yes         none (waiting is fine)
 scanning          no          none (waiting is fine)
 scanning          yes         none (waiting is fine)

I had tested the top 6 cases. Your patch handles both usb plugged in and empty list with one shot. I suggest to have the no sdcard case included.

So, I reset the review flag. Once you add it, please reset it again.

I will open a bug for requesting a "go to camera" button like you did before in the case of empty list in Gallery app.

Comment 4

[Aras Balali Moghaddam]

John Hu, thanks for the quick feedback! I will try to test the no sdcard case and cover it as well, then will get back to you.

Comment 5

[Aras Balali Moghaddam]

Attachment: bug915039-close-button-for-open-video-activity.patch

I have updated the patch based on PR feedback and added the no sdcard case

Comment 6

[Aras Balali Moghaddam]

Attachment: Screenshot of different overlay states for video app after applying the above patch

Without the close button, user experience when trying to access video using a video pick activity is brocken. The above patch address the problem by adding the close button only to those cases, so user can discard the overlay and go back to the app that triggered the pick avtivity.

I would like to get feedback from UX about this, and make sure it is an acceptable fix. These are not very commong cases, but when user get there, they should not get stuck. I think we can improve the visual representation, and maybe come up with a better layout for the case when both setting button and close button are presented.

Comment 7

[Rob MacDonald [:robmac]]

Thanks for drawing our attention to this one. Jacqueline will be putting together some wires for this in the next few days and I've flagged her on the bug. 

Also adding David Flanagan as a heads-up. David, Jacqueline may be in touch with you in the next couple of days with questions. David, feel free to clear the flag if it's not relevant for you.

Comment 8

[David Flanagan [:djf]]

Aras,

Thank you for working on this bug!  I don't know why everyone (including me) has been dragging their feet so much on it.  It is pretty disheartening to see Hema's nominations of this bug for 1.2, 1.3 and now 1.4 all go ignored.

Jacqueline: I don't actually think we need your input on this. We've fixed the equivalent bug in gallery already, and it is clear what the right thing to do here is.

Aras: are you still interested in working on this?  If not, please reassign the bug to John.

If you are, I'd like to see some changes to what John proposed in comment 3:

- The button should read "Cancel" instead of "Close" because the user is choosing to cancel the pick, not just close an app.

- In any pick activity we should have a "Cancel" option. Even for the scanning overlay.  (If cancelling an upgrade could cause data loss, then we shouldn't have a cancel button on that screen.  Needinfo John to ask about that case).

- During a pick activity, Cancel should be the only button shown. An inline activity like this needs to be able to go back to its caller, but it should not give the user any way to go to some other app. So please do not display the Storage Settings button in the pick activity case.

- Similarly, please don't add a Camera button to the "no videos" screen in the pick activity case. But you can add it in the non-pick case.  And you could update the message so that in addition to saying "load videos" it says "download or record videos" or something like that. That should probably just be a different bug, however, and we will want Jacqueline or Chris's help to decide on the wording.

Comment 9

[David Flanagan [:djf]]

Aras,

Please see also bug 919834. In that bug, I claim that on devices that have and sdcard and internal memory, you will never get into the "nocard" state and will never see the "Go to Media Storage Settings" button.  And on devices that have only a sdcard slot, there won't be any interesting settings there.  If there is no card, you're just stuck and going to settings does not help.  So I think there should never settings button on the nocard overlay.  Just a cancel button in the pick activity case.

Let me know if that does not make sense.

Comment 10

[Aras Balali Moghaddam]

Hi David,
Thanks for looking into this. I think it would be best if John or someone else would wrap this up. The earliest I can work on it would be next weekend. I will assign this bug to him.

I would love to get back into gaia and contribute more. If you would be kind enough to assign a couple of slightly more challenging bugs to me to work on over the holidays, I would really appreciate it. I need a safe haven to scape to after dealing with so much horrible code at my day job.

Comment 11

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to David Flanagan [:djf] from comment #8)
> - In any pick activity we should have a "Cancel" option. Even for the
> scanning overlay.  (If cancelling an upgrade could cause data loss, then we
> shouldn't have a cancel button on that screen.  Needinfo John to ask about
> that case).

We can cancel the upgrade procedure with throwing an error and the database will rollback to its original state. So, when we close the activity during upgrading, the whole updated records will be rolled back.

Comment 12

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: add cancel button

This patch does the following change:
1. remove storage setting button, sync with bug 919834, we don't need this button anymore.
2. add cancel/camera button
3. change the showOverlay to show cancel button when pick mode and show camera only when non-pick mode and id is empty
4. update text

Comment 13

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: patched screenshots.

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8348610 [details] [review]
add cancel button

r+ if you remove the unused code, remove the string change (or change the id) and at least consider an alternative to the CSS overrides.  See github for details.

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

merged to master:
https://github.com/mozilla-b2g/gaia/commit/a523c1ee4130a7883981924f8e68fb94be02428a

Comment 16

[Axel Hecht [:Pike]]

Can we back this out or revert the string change, like David requested?

Comment 17

[John Hu [:johnhu][:johu][:醬糊小弟]]

Axel,

Sorry about that. I thought I was removed the change of string before. I will revert the string change within this bug.

Comment 18

[John Hu [:johnhu][:johu][:醬糊小弟]]

Reopen this bug to revert the patch and remove the string change.

Comment 19

[John Hu [:johnhu][:johu][:醬糊小弟]]

reverting this patch:
https://github.com/mozilla-b2g/gaia/commit/0fe11036d1d7128a7288e8efa3b73dab7497b8e9

Comment 20

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: new pr without string change.

move the r+ to this patch and remove the string change.

Comment 21

[John Hu [:johnhu][:johu][:醬糊小弟]]

re-land the code to master without string change:
https://github.com/mozilla-b2g/gaia/commit/28a3012b8dc4a7794b25798ac151fc25a9d42e36