Closed Bug 1337692 Opened 7 years ago Closed 7 years ago

Firefox doesn't ask for camera or audio permission with input type="file", accept=

Categories

(Firefox for Android Graveyard :: Settings and Preferences, defect, P1)

50 Branch
defect

Tracking

(fennec+, firefox51 wontfix, firefox52+ wontfix, firefox53+ verified, firefox54+ verified)

VERIFIED FIXED
Firefox 54
Tracking Status
fennec + ---
firefox51 --- wontfix
firefox52 + wontfix
firefox53 + verified
firefox54 + verified

People

(Reporter: lgbrowser5, Assigned: cnevinchen)

References

()

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; Trident/7.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E; InfoPath.3; NetHelper70; CNS_UA; AD_LOGON=4C47452E4E4554; rv:11.0) like Gecko

Steps to reproduce:

               1. Open Firefox app 
               2. Go to www.facebook.com and Log in with credentials 
               3. Select 'Add Photos/Video' 
               4. Select 'Add Photo' 
               5. Select Camera (observe issue) 
               6. Select 'Add Video' > select Camera icon > select Camcorder (observe issue) 


Actual results:

Camera app fails to open when selected in Firefox. Issue is seen on both facebook.com and twitter.com **If same steps are followed on Chrome app, camera app opens properly**


Expected results:

Camera app should open when selected.
UPDATE: When permission was given manually (Settings > Apps > Firefox > Permissions > Camera ON) Camera app opened properly when following tester's action. 
Firefox never provides pop-up that requests user permissions for camera like other apps. 
Example: When using Chrome, if I open facebook.com and select to add photo from camera for the first time, there will be a pop-up requesting permission for Chrome to use the camera app.  

But I am not seeing those first time pop-ups in Firefox. When camera is selected, nothing happens.
This might make some users assume Firefox is incompatible with the camera app. 
Is that also normal operation?
https://support.mozilla.org/t5/Firefox-for-Android/How-does-Firefox-for-Android-use-the-permissions-it-requests/td-p/24296
Firefox will always display a message asking for permission each time a site asks to add a photo or record audio.
[Tracking Requested - why for this release]: we should fix this behavior

That support.mozilla.org document is referring to webrtc. File upload which Facebook uses is a different source. Though I can confirm that the behavior reported in comment 0 and 1 is broken for me as well.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Tracking 52/53/54+. I don't think we need to track this for 51, but I will let Gerry decide. I agree with Comment 3 that we should fix this so it works as expected.
This is an important issue for a partner. We should at least see if there is an easy fix.
More detail. See example URL.

On Chrome, when you click "Choose File" you are immediately asked to give permission to the camera. If you choose deny, it takes you to a recent file chooser. If you click allow, it gives you the popup and you can choose the camera.

On Firefox, when you click "Choose File", you are immediately presented with a chooser that has the camera  (and other options) in it, but if you select Camera, it simply doesn't work.

We need to ask for camera permission when choose file is selected with image/* type.
Summary: Camera fails to open in facebook → Firefox doesn't ask for camera permission with input type=image
tracking-fennec: ? → +
Priority: -- → P1
I've verified the problem.
Currently, Fennec can support camera request via JS
    navigator.getUserMedia({video: true}, handleVideo, videoError);
(see sample here https://www.kirupa.com/html5/accessing_your_webcam_in_html5.htm)
but not via html5 (https://mobilehtml5.org/ts/?id=23)


Hi Sebastian
Do you think we should let Gecko to handle this RuntimePermission request (which I think is not implemented yet) and use existing mechanism here : http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#762

Or I can just check for permission before handling the chooser intent ?
https://hg.mozilla.org/mozilla-central/annotate/698de2db1b16a5ef3c6a39f0f72885e69aee4022/mobile/android/base/java/org/mozilla/gecko/FilePicker.java#l118
Flags: needinfo?(s.kaspari)
Assignee: nobody → cnevinchen
Comment on attachment 8839863 [details]
Bug 1337692 - Check runtime permission in FilePicker.

https://reviewboard.mozilla.org/r/114470/#review116086

::: mobile/android/base/java/org/mozilla/gecko/FilePicker.java:13
(Diff revision 1)
> +import org.mozilla.gecko.permissions.Permissions;
>  import org.mozilla.gecko.util.BundleEventListener;
>  import org.mozilla.gecko.util.EventCallback;
>  import org.mozilla.gecko.util.GeckoBundle;
>  
> +import android.*;

nit: This doesn't seem to be needed? :)

::: mobile/android/base/java/org/mozilla/gecko/FilePicker.java:66
(Diff revision 1)
>              if ("mimeType".equals(mode)) {
>                  mimeType = message.getString("mimeType");
>              } else if ("extension".equals(mode)) {
>                  mimeType = GeckoAppShell.getMimeTypeFromExtensions(message.getString("extensions"));
>              }
> +            String[] requiredPermission = checkIfPermissionNeeded(mimeType);

nit: final?

::: mobile/android/base/java/org/mozilla/gecko/FilePicker.java:99
(Diff revision 1)
> +            case "audio/*":
> +                return new String[]{Manifest.permission.RECORD_AUDIO, Manifest.permission.READ_EXTERNAL_STORAGE};
> +            case "image/*":
> +                return new String[]{Manifest.permission.CAMERA, Manifest.permission.READ_EXTERNAL_STORAGE};
> +            case "video/*":
> +                return new String[]{Manifest.permission.RECORD_AUDIO, Manifest.permission.READ_EXTERNAL_STORAGE};

What if I specify a specific MIME type like image/jpeg? Will this not trigger the camera?

::: mobile/android/base/java/org/mozilla/gecko/FilePicker.java:107
(Diff revision 1)
> +                return new String[]{Manifest.permission.CAMERA, Manifest.permission.READ_EXTERNAL_STORAGE};
> +            case "video/*":
> +                return new String[]{Manifest.permission.RECORD_AUDIO, Manifest.permission.READ_EXTERNAL_STORAGE};
> +
> +        }
> +        return new String[]{Manifest.permission.READ_EXTERNAL_STORAGE};

nit: style [spaces] (here and above):
> new String[] { Foo, Bar }

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1148
(Diff revision 1)
>       * and other one-shot constructions.
>       **/
>      @Override
>      public void onCreate(Bundle savedInstanceState) {
>          GeckoAppShell.ensureCrashHandling();
> +        FilePicker.init(this);

This is already called from GeckoApplication.onCreate(). Why do we need to call it again here?
Attachment #8839863 - Flags: review?(s.kaspari)
Comment on attachment 8839863 [details]
Bug 1337692 - Check runtime permission in FilePicker.

https://reviewboard.mozilla.org/r/114470/#review116214

::: mobile/android/base/java/org/mozilla/gecko/FilePicker.java:39
(Diff revision 2)
>  
> +    // FilePicker must use activity context if it want to prompt for runtime permission. (bug 1337692)
>      public static void init(Context context) {
>          if (sFilePicker == null) {
> -            sFilePicker = new FilePicker(context.getApplicationContext());
> +            sFilePicker = new FilePicker(context);
>          }
>      }

Ah, okay, this change was for requesting the runtime permission. However now we have a static reference to the activity here and therefore it will never get garbage collected. That's a problem.

You could either try to get the current activity from GeckoAppShell (not sure about the side effects here):
GeckoAppShell.getGeckoInterface().getActivity()

Or you need to make sure that the reference is cleared once the activity is killed (There should be some other examples in BrowserApp or GeckoApp).

::: mobile/android/base/java/org/mozilla/gecko/FilePicker.java:66
(Diff revision 2)
> +            final String[] requiredPermission = checkIfPermissionNeeded(mimeType);
>  
> -            showFilePickerAsync(title, mimeType, new ResultHandler() {
> +            final String finalMimeType = mimeType;
> +            Permissions.from(context)
> +                    .withPermissions(requiredPermission)

checkIfPermissionNeeded() can return null now. It looks like the permissions code expects a non-null array.

::: mobile/android/base/java/org/mozilla/gecko/FilePicker.java:94
(Diff revision 2)
> +        if (mimeType == null) {
> +            return null;
> +        }

see comment above.
Attachment #8839863 - Flags: review?(s.kaspari)
Flags: needinfo?(s.kaspari)
Comment on attachment 8839863 [details]
Bug 1337692 - Check runtime permission in FilePicker.

https://reviewboard.mozilla.org/r/114470/#review116214

> Ah, okay, this change was for requesting the runtime permission. However now we have a static reference to the activity here and therefore it will never get garbage collected. That's a problem.
> 
> You could either try to get the current activity from GeckoAppShell (not sure about the side effects here):
> GeckoAppShell.getGeckoInterface().getActivity()
> 
> Or you need to make sure that the reference is cleared once the activity is killed (There should be some other examples in BrowserApp or GeckoApp).

yah...sFilePicker is static so it'll kept a reference to the context/activity. Keeping activity context as static member is a bad idea ...

I saw a lot of clases using GeckoAppShell.getGeckoInterface().getActivity() (e.g. IntentChooserPrompt) without worried about side effects. And I saw some delegates in BrowserApp do their onDestroy() when BrowserApp's onDestroy() is called. 

I'll go for GeckoAppShell.getGeckoInterface().getActivity() since it's cleaner.
Comment on attachment 8839863 [details]
Bug 1337692 - Check runtime permission in FilePicker.

https://reviewboard.mozilla.org/r/114470/#review116368
Attachment #8839863 - Flags: review?(s.kaspari) → review+
I'm not seeing the ask for permissions using this testcase:

http://html.com/input-type-image/

I don't see anything on the console.

The File Upload comes up immediately and Camera doesn't work.
So it appears that Chrome asks for the pictures/video permission any time the file upload widget is used. Otherwise you end up with camera items that do not work on the file picker.

I think we should do the same thing.

Notice on Chrome, they add the permission based on what the device supports, not based on the mime types.

The mime types are only used to determine the type of chooser.
I put together a new testcase for every type. 

https://people-mozilla.org/~mkaply2/inputs.html

You'll need to install an audio recorder to get all of the popups.

Video and photo permission is shared. Audio is a separate permission, so for many of these you'll get two popups.

For the "Everything" permission on Chrome, you get both audio and video permission prompts.
Summary: Firefox doesn't ask for camera permission with input type=image → Firefox doesn't ask for camera or audio permission with input type="file", accept=
One last thing. If you deny permission, when you click the button again, you don't get asked for permission again. You should. The only case where you should not get asked again is if you checked the box that said "don't ask again"
I've update the patch with two small changes.

1. In the "deny" case, we still need to callback otherwise we can never open the dialog again.
2. I've simplified the "checkIfPermissionNeeded". External storage is never needed by this (and Chrome doesn't do it).
Comment on attachment 8840658 [details]
Bug 1337692 - Check runtime permission in FilePicker.

Switching review to snorp. No idea if mozreview magic will happening.

Sebastian reviewed the bulk of the patch. This is only for my additional change.
Attachment #8840658 - Flags: review?(s.kaspari) → review?(snorp)
Attachment #8840658 - Flags: review?(snorp) → review?(s.kaspari)
Comment on attachment 8840658 [details]
Bug 1337692 - Check runtime permission in FilePicker.

https://reviewboard.mozilla.org/r/115106/#review117202

drive-by r+; seeing as this change only added an error callback, which looks fine, feel free to treat the rest as nits. But I'd really rename that method ;-)

::: mobile/android/base/java/org/mozilla/gecko/FilePicker.java:69
(Diff revision 1)
>              }
>  
> -            showFilePickerAsync(title, mimeType, new ResultHandler() {
> +            final String[] requiredPermission = checkIfPermissionNeeded(mimeType);
> +            final String finalMimeType = mimeType;
> +            // Use activity context cause we want to prompt for runtime permission. (bug 1337692)
> +            Permissions.from(GeckoAppShell.getGeckoInterface().getActivity())

Can't you use `this.context`?

::: mobile/android/base/java/org/mozilla/gecko/FilePicker.java:74
(Diff revision 1)
> +            Permissions.from(GeckoAppShell.getGeckoInterface().getActivity())
> +                    .withPermissions(requiredPermission)
> +                    .andFallback(new Runnable() {
> +                        @Override
> +                        public void run() {
> +                            callback.sendError(false);

Looks OK. JS code seems to do `file = file || null; if !file return` kind of a thing, so that should work.

::: mobile/android/base/java/org/mozilla/gecko/FilePicker.java:91
(Diff revision 1)
> -        }
> +                        }
> +                    });
> +        }
> +    }
> +
> +    private String[] checkIfPermissionNeeded(final String mimeType) {

Perhaps annotate mimeType as `@NoneNull`?

Could also similarly annotate the method itself if you care enough.

::: mobile/android/base/java/org/mozilla/gecko/FilePicker.java:91
(Diff revision 1)
> -        }
> +                        }
> +                    });
> +        }
> +    }
> +
> +    private String[] checkIfPermissionNeeded(final String mimeType) {

This doesn't seem like a good name for this method. Perhaps something like `getPermissionsForMimeType`?

Ideally this should have unit tests as well to describe & validate the behaviour (marking method as static will make that easier).
Attachment #8840658 - Flags: review+
> Can't you use `this.context`?

Crashes. See https://bugzilla.mozilla.org/show_bug.cgi?id=1337692#c13

I've made the other requested changes. And I'll work on the testcases.

I didn't do the @noneNull yet because I want to understand that better.
> Can't you use `this.context`?
We can't use `this.context` cause if we want to use Permissions.from(...) to prompt for run time permission , we need an activity context[1].

[1] http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/mobile/android/geckoview/src/main/java/org/mozilla/gecko/permissions/PermissionBlock.java#80
Flags: needinfo?(mozilla)
Flags: needinfo?(gkruglov)
Thanks for that answer Nevin. I have checked this in.
Flags: needinfo?(mozilla)
https://hg.mozilla.org/mozilla-central/rev/da494e30eb87
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment on attachment 8840658 [details]
Bug 1337692 - Check runtime permission in FilePicker.

Approval Request Comment
[Feature/Bug causing the regression]: Using camera from websites
[User impact if declined]: Unable to use camera for file inputs on Android N unless explicitly enabled in the settings for the app.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: Go to https://people-mozilla.org/~mkaply2/inputs.html with an Androind N device. Verify that you get permission prompts clicking on the various buttons.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low
[Why is the change risky/not risky?]: Very isolated. Adds permission where there was none using existing permissions code.
[String changes made/needed]:

This is requested by a partner who is putting Firefox on their device. This should hopefully be the last bug for 52. Without this, using the camera to upload things to Twitter/Facebook on Android N is broke.
Attachment #8840658 - Flags: review?(s.kaspari)
Attachment #8840658 - Flags: approval-mozilla-beta?
Attachment #8840658 - Flags: approval-mozilla-aurora?
Comment on attachment 8840658 [details]
Bug 1337692 - Check runtime permission in FilePicker.

Let's uplift this to aurora now; if it needs to be in 52 mobile release then we will need another build so I'll leave it for the 52 owners.
Attachment #8840658 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8840658 [details]
Bug 1337692 - Check runtime permission in FilePicker.

Too late for 52, sorry.
Attachment #8840658 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Tested with Nexus 9 (Android 7.1.1) on latest Aurora build 54.0a2 (2017-03-08) and 53.ob1 following the instructions from comment #30.

I've got the permissions prompts only for the first time I tapped on browse button. But manually change the permission in Android settings and checked for every button. The prompt was displayed for all of them if the permission was unchecked.
Verified as fixed on Beta build 53.0b7.
Device: Nexus 9 (Android 7.1.1).
Status: RESOLVED → VERIFIED
Flags: needinfo?(gkruglov)
Verified as fixed on Beta build 54.0b2.
Device: Nexus 6 (Android 7.0).
Comment on attachment 8840658 [details]
Bug 1337692 - Check runtime permission in FilePicker.

https://reviewboard.mozilla.org/r/115106/#review152490

Seems ok to me modulo the one issue, but we should get someone in Taipei to look as well (Max?).

::: mobile/android/base/java/org/mozilla/gecko/FilePicker.java:69
(Diff revision 1)
>              }
>  
> -            showFilePickerAsync(title, mimeType, new ResultHandler() {
> +            final String[] requiredPermission = checkIfPermissionNeeded(mimeType);
> +            final String finalMimeType = mimeType;
> +            // Use activity context cause we want to prompt for runtime permission. (bug 1337692)
> +            Permissions.from(GeckoAppShell.getGeckoInterface().getActivity())

Presumably we need an Activity context here, and the FilePicker only has an app context.

Regardless, this call is gone from GeckoInterface. Use GeckoActivityMonitor.getInstance().getCurrentActivity() until we wire this up some better way.
Attachment #8840658 - Flags: review+
Product: Firefox for Android → Firefox for Android Graveyard
No longer depends on: 1362919
Regressions: 1362919
You need to log in before you can comment on or make changes to this bug.