Closed
Bug 1337692
Opened 8 years ago
Closed 8 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)
Tracking
(fennec+, firefox51 wontfix, firefox52+ wontfix, firefox53+ verified, firefox54+ verified)
People
(Reporter: lgbrowser5, Assigned: cnevinchen)
References
()
Details
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
sebastian
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Grisha
:
review+
snorp
:
review+
lizzard
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta-
|
Details |
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.
Reporter | ||
Comment 1•8 years ago
|
||
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?
Reporter | ||
Comment 2•8 years ago
|
||
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.
Comment 3•8 years ago
|
||
[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: --- → ?
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
tracking-firefox51:
--- → ?
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
tracking-firefox54:
--- → ?
Ever confirmed: true
Comment 4•8 years ago
|
||
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.
Comment 5•8 years ago
|
||
This is an important issue for a partner. We should at least see if there is an easy fix.
Comment 6•8 years ago
|
||
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
Comment 7•8 years ago
|
||
I think it should be as simple as asking permission here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/FilePicker.java#109
Comment 8•8 years ago
|
||
Chrome code is here:
https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java?type=cs&q=ACTION_IMAGE_CAPTURE&l=124
We're probably broken for audio and video as well.
Updated•8 years ago
|
tracking-fennec: ? → +
Priority: -- → P1
Assignee | ||
Comment 9•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → cnevinchen
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 13•8 years ago
|
||
mozreview-review |
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)
Updated•8 years ago
|
Flags: needinfo?(s.kaspari)
Assignee | ||
Comment 14•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 16•8 years ago
|
||
mozreview-review |
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+
Comment 17•8 years ago
|
||
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.
Comment 18•8 years ago
|
||
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.
Comment 19•8 years ago
|
||
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.
Updated•8 years ago
|
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=
Comment 20•8 years ago
|
||
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"
Comment hidden (mozreview-request) |
Comment 22•8 years ago
|
||
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 23•8 years ago
|
||
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)
Updated•8 years ago
|
tracking-firefox51:
? → ---
Attachment #8840658 -
Flags: review?(snorp) → review?(s.kaspari)
Comment 24•8 years ago
|
||
mozreview-review |
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+
Comment 25•8 years ago
|
||
> 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.
Comment 26•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/da494e30eb87f4ff827cbb3ffe7923694bc49c8c
Bug 1337692 - Ask for permission on input=file/accept. r=sebastian,grisha
Assignee | ||
Comment 27•8 years ago
|
||
> 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)
Comment 28•8 years ago
|
||
Thanks for that answer Nevin. I have checked this in.
Flags: needinfo?(mozilla)
Comment 29•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 30•8 years ago
|
||
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 31•8 years ago
|
||
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+
Updated•8 years ago
|
Comment 32•8 years ago
|
||
bugherder uplift |
Comment 33•8 years ago
|
||
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-
Updated•8 years ago
|
Comment 34•8 years ago
|
||
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.
Comment 36•8 years ago
|
||
Verified as fixed on Beta build 53.0b7.
Device: Nexus 9 (Android 7.1.1).
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
Flags: needinfo?(gkruglov)
Comment 37•8 years ago
|
||
Verified as fixed on Beta build 54.0b2.
Device: Nexus 6 (Android 7.0).
Comment 38•7 years ago
|
||
mozreview-review |
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+
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
Updated•3 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•