1135789 - [Camera][Flame] Volume overlay should not be displayed when pressing volume button

Status: VERIFIED FIXED

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

Description

[No-Jun Park [:njpark]]

STR:
Start the phone, and on homescreen, use the volume key to set it to vibrate mode
Open camera app, use volume key to turn volume on, take picture

Expected:
shutter sound heard
Actual:
shutter sound is not heard

-Go back to homescreen, use the volume key to turn up the call volume
-Go back to camera app, use the volume key to mute the app sound, take picture

Expected:
shutter sound not heard
Actual:
shutter sound heard

In 2.2, this is not reproducible.  But in 2.2, it was noted that when volume keys are pressed, the icon showed that it is controlling the call volume (phone icon), not the app volume (speaker ico)

Version Info:
Gaia-Rev        a6881205deae450757a8d1e1ed65e5e5be0ec633
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/86d2bb8bb1c9
Build-ID        20150223010224
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150223.042702
FW-Date         Mon Feb 23 04:27:13 EST 2015
Bootloader      L1TC000118D0

Comment 1

[No-Jun Park [:njpark]]

[Blocking Requested - why for this release]:
Marked as regression, since the feature itself was working in 2.2, although the icon shown was incorrect.

Impact: can't control sound in camera app.

Comment 2

[Mike Habicher [:mikeh] (high bugzilla latency)]

The Camera app uses the 'notification' channel:

https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/lib/sounds.js#L158

From the description in comment 0, the observed behaviour sounds correct.

Comment 3

[No-Jun Park [:njpark]]

mistakenly changed its status. changing back.

Comment 4

[No-Jun Park [:njpark]]

ni?ing tif and amylee to get opinions on this UX behavior.

Comment 5

[Tiffanie Shakespeare [:tif]]

Omega is the one who has done all the sound channel work. Here's his spec from a different bug:
https://bug991026.bugzilla.mozilla.org/attachment.cgi?id=8420872

Looking at this, the notification sound and ringer are tied to the same volume setting. So increasing the volume in the camera app should change the notification volume - unless the volume key is dedicated to another function (e.g. iphone does continuous photo)

Based on the actual results, it seems like the volume control isn't changing volume but it isn't doing anything else either...?

Comment 6

[No-Jun Park [:njpark]]

> Looking at this, the notification sound and ringer are tied to the same
> volume setting. So increasing the volume in the camera app should change the
> notification volume - unless the volume key is dedicated to another function
> (e.g. iphone does continuous photo)

This was the behavior in 2.2, but in master, instead of changing the notification volume, it changes 'app volume' which I don't think the camera app actually uses at all.

Comment 7

[Tiffanie Shakespeare [:tif]]

In light of the current information I have, we should revert to the previous behaviour so that things work again. 

However, I'm ni'ing Omega who is the UX designer working on sound channels/settings. He's currently out for Chinese New Year so give him a minute to reply.

Omega - two things, firstly did anything change in regards to the sound setting spec I referenced in comment 5? If not, we are going to revert the behaviour so that the camera volume is tied to the notification volume again.

Additionally, I want to double check that the notification setting should still be tied to the ringer and they are not separate settings. Thanks!

Comment 8

[Omega Feng [:Omega] [:馮於懋]]

ni? audio UX owner Jenny. Please help on this, thanks!

Comment 9

[Jenny Lee]

Hi Tiffanie,

See Bug 1068219 attachment 8541546 [details] for the latest spec (v1.5). On p.8 adjusting volume with volume key - it is stated that using volume key = take photo in Camera, volume shouldn't be adjustable in Camera at all. 

And yes notification setting is tied to the ringer. Thanks!

Comment 10

[No-Jun Park [:njpark]]

Sorry, just checked that it does take photos when the volume up/down switch is pressed, didn't notice this before.

I guess the according to the spec, then perhaps we should *not* display the volume control overlay when the volume button is pressed, since it might be confusing to the user?

Comment 11

[Tiffanie Shakespeare [:tif]]

No, volume should not be displayed if the volume button is taking photos. 

Thanks Jenny for the updated spec!

Comment 12

[No-Jun Park [:njpark]]

Updated the bug title- ni?ing mikeh FYI.

Comment 13

[Tiffanie Shakespeare [:tif]]

Let me also be clear that the volume also should not be changing. The volume key should not have any impact on volume. It should only be taking photos.

Comment 14

[Stephany Wilkes]

Hi y'all. I'm the Product Manager for Media and PM for the UX team, who knew nothing about this. Tif is UX for Media, including the Camera app, and she knew nothing about this. :-) Just saying. 

This is not primarily audio issue (which I see Jenny was flagged for), but a camera functionality change, which means Tif is the UX lead here. 

One of these two behaviors at a time is fine: the button can either control volume OR take a photo. It seems like Wilson and Jenny would like the button to take a photo, which is fine, but then the button cannot ALSO be the volume control at the same time. That's a bug.

Expected behavior:
1. Launch camera app on device.
2. Press volume rocker.
Expected: Photo taken OR video recording started, as per Wilson, AND the manufacturer must be able to disable this.
Actual: Photo taken AND volume notification sound changes. 
 
I'm taking the liberty of blocking on this because this cannot ship as it is today, both because of the UX but also because the manufacturer needs to be able disable this. Also, it sounds like the icons are wrong in 2.2, so there are three distinct issues going on here. I don't know exactly where everything has landed, so I'm using the 2.2+ flag until someone tells me otherwise.

I apologize to everyone for the UX process fail here. If there had been a ui-review? set for Tif or myself we would have set it to ui-review-. We'll look for that next time around.

Comment 15

[Stephany Wilkes]

Wilson, please add a link to the patch in the attachments so we can find it and test on device. Thanks!

Comment 16

[Wilson Page [:wilsonpage]]

Fixed by bug 1137013

Comment 17

[Wilson Page [:wilsonpage]]

(In reply to Stephany Wilkes from comment #15)
> Wilson, please add a link to the patch in the attachments so we can find it
> and test on device. Thanks!

This feature is already on master.

Comment 18

[Wilson Page [:wilsonpage]]

I should also mention that this isn't in v2.2 branch. Although it is a pretty excellent feature, so maybe it should be uplifted?

Comment 19

[Tiffanie Shakespeare [:tif]]

Created follow up bugs - these need to be fixed for the feature to land in 2.2.

Bug 1137917 - [camera] volume setting UI is shown and volume is changed when pressing the volume rocker
Bug 1137918 - [camera] capture interaction via volume key needs to be optional for manufacturers
Bug 1137924 - [camera] volume key in preview should return to viewfinder

Ni'ing Wilson just in case I forgot him on one of the bugs. Also, here is a mini-spec.
https://github.com/mozilla-b2g/gaia-specs/blob/master/%5B2.2%5D%20Camera%20Shutter.pdf

Comment 20

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue is verified fixed on the latest Nightly Flame 3.0 build.  Setting verifyme to check this issue if/when this gets to 2.2.

Actual Results: The volume buttons only take pictures now.

Environmental Variables:
Device: Flame 3.0 KK (Full Flash) (319 MB)
BuildID: 20150312010235
Gaia: 0c4e8b0b330757e261b031b7e7f326ef419c9808
Gecko: 5334d2bead3e
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Comment 21

[Wilson Page [:wilsonpage]]

I'm super confused. What is the desired behaviour?

Current volume button behaviour:

- 'master': Shows no volume overlay now as volume buttons now capture picture/video.
- 'v2.2': Shows a volume overlay and doesn't capture picture.

Comment 22

[Tiffanie Shakespeare [:tif]]

Please see spec in comment 19...

In viewfinder, volume keys should capture. In preview, volume keys should change media volume (which affects video playback).

Hope that helps!

Comment 23

[Wilson Page [:wilsonpage]]

(In reply to Tiffanie Shakespeare from comment #22)
> Please see spec in comment 19...
> 
> In viewfinder, volume keys should capture. In preview, volume keys should
> change media volume (which affects video playback).
> 
> Hope that helps!

comment 19 doesn't include the patch (bug 1133965) which actually implemented the volume button capture. I'm assuming we'll want to uplift that to v2.2 too?

Comment 24

[Wilson Page [:wilsonpage]]

For volume key capture feature to be included in v2.2 the following patches must be uplifted:

- bug 1133965
- bug 1137918
- bug 1137013

Comment 25

[Tiffanie Shakespeare [:tif]]

Oops! Yep, we definitely want to include the actual patch too along with the updates. Thanks Wilson :)

Comment 26

[Wilson Page [:wilsonpage]]

I've created 4 v2.2 patches layered in the following order:

1. bug 1133965
2. bug 1137013
3. bug 1137918
4. bug 1144830

They must land in this order and each be rebased against mozilla/v2.2 after its parent patch lands.

Comment 27

[Hema Koka [:hema]]

Stephany/Tif,

Why do we need this in 2.2, I see that this is moved from 3.0 to 2.2 blocker. I don't know of any asks on this in 2.2 release and moreover this late in the cycle, we should not be adding new features. Seems like there is some confusion or mis-understanding in the trail of comments on this bug.

Sticking it back into the 2.2 nom queue until we confirm the need for this to be blocking 2.2 release. Stephany, if this is not a blocker please remove the nom for 2.2

Wilson,

If any related patches have landed, can we please back it out. It is too late to land new features in 2.2 branch. We are way past that phase. 

https://bugzilla.mozilla.org/show_bug.cgi?id=1135789#c26

Thanks
Hema

Comment 28

[Stephany Wilkes]

Hema, see my comments here. This did not come from UX.
https://bugzilla.mozilla.org/show_bug.cgi?id=1137918

Comment 29

[Hema Koka [:hema]]

(In reply to Stephany Wilkes from comment #28)
> Hema, see my comments here. This did not come from UX.
> https://bugzilla.mozilla.org/show_bug.cgi?id=1137918

Thanks Stephany, it looks like the previous comments in the bug and moving bug to blocker caused some confusion. I am removing this from 2.2 nom since it is not a blocker for this release.

Comment 30

[Wilson Page [:wilsonpage]]

(In reply to Hema Koka [:hema] from comment #27)
> Wilson,
> 
> If any related patches have landed, can we please back it out. It is too
> late to land new features in 2.2 branch. We are way past that phase. 
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1135789#c26
> 
> Thanks
> Hema

Only one related patch got landed on v2.2 and was backed out yesterday.