-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add camera intent with file input capture #1609
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit hard to review this due to whitespace and reformatting changes to other parts of the file. Is it possible to include only the changes related to file picking without the other reformatting?
Codecov Report
@@ Coverage Diff @@
## master #1609 +/- ##
=======================================
Coverage 72.61% 72.61%
=======================================
Files 23 23
Lines 1800 1800
=======================================
Hits 1307 1307
Misses 493 493 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Sure! I went ahead and removed those from this pull request. |
I didn't think it would make a difference, but just to be sure I went ahead and re-ran all my tests and the file input works beautifully. |
framework/src/org/apache/cordova/engine/SystemWebChromeClient.java
Outdated
Show resolved
Hide resolved
framework/src/org/apache/cordova/engine/SystemWebChromeClient.java
Outdated
Show resolved
Hide resolved
Co-authored-by: エリス <erisu@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks good to me now. I'll see about trying to test it on a device sometime this week :)
What additional testing needs to be done to get this merged in? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
<!-- // Display: File browser
// restrictions: no file type
// file limit: 1 -->
<h2>1. File</h2>
<input type="file" />
<!-- // Display: File browser
// restrictions: must be an image file type
// file limit: 1 -->
<h2>2. File - accept image</h2>
<input type="file" accept="image/*" />
<!-- // Display: Chooser (File or Camera)
// restrictions: no file type
// file limit: 1 -->
<h2>3. File - cature</h2>
<input type="file" capture />
<!-- // Display: Chooser (File or Camera)
// restrictions: must be an image file type
// file limit: 1 -->
<h2>4. File - accept image and capture</h2>
<input type="file" accept="image/*" capture />
<!-- // Display: File browser
// restrictions: no file type
// file limit: many -->
<h2>5. File - multiple</h2>
<input type="file" multiple />
<!-- // Display: File browser
// restrictions: must be an image file type
// file limit: many -->
<h2>6. File - accept image and multiple</h2>
<input type="file" accept="image/*" multiple />
<!-- // Display: Chooser (File or Camera)
// restrictions: no file type
// file limit: many -->
<h2>7. File - capture & multiple</h2>
<input type="file" capture multiple />
<!-- // Display: Chooser (File or Camera)
// restrictions: must be an image file type
// file limit: many -->
<h2>8. File - accept image, capture & multiple</h2>
<input type="file" accept="image/*" capture multiple />
<!-- // BAD CASES
// If accept type is restrictive to a non-image format and capture flag was provided.
// Display: Chooser (File or Camera)
// restrictions: must be a PDF
// file limit: 1 -->
<h2>9. File - accept pdf and capture</h2>
<input type="file" accept=".pdf" capture />
The list above is in the order of the test sampe. End results:
This PR will be pushed back for a later release, until the above issues are resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve issues discovered from test. See last comment with test examples.
Hi. We've had a related issue in our backlog for quite some time and last time when the topic came up again in our team, I was quite happy to find this PR which was made a few days earlier. Now it's been without an update for quite some time. Are you @KenCorbettJr still motivated and planning to finish it? Any updates? |
@ArturKp I would still love to get this done. I was busy for a bit, but I have some more time now and I think we are close to getting this one over the line. I created a test app that pointed at my fork of cordova android to test all the permutations (git repo: https://github.com/KenCorbettJr/cordova-android-file-tester) and at least on my Samsung S22 everything seems to work great. Every time there was a @erisu is there any way you can take another look with my test app? I am pretty sure I set it up correctly. |
I will try and re-test this PR tomorrow. |
@erisu Did you try long-pressing the files to select multiple? Because as soon as I clicked one it selected it, but long-pressing a file allowed me to select multiple. I've tested https://github.com/KenCorbettJr/cordova-android-file-tester on Android 13 (Samsung Galaxy A22), 10 (Moto g8+) and 8 (Samsung Galaxy A3 2017) devices and was not able to reproduce any of the issues. |
Hi all, is there any timeline this will be released? Thanks! |
When will this be merged? |
What about requesting for camera permission? |
If you mean android.Manifest.permission.CAMERA this permission is only required for accessing the camera APIs, not for using the intents. |
Using the onShowFileChooser implementation in this pull request, and choosing Camera, the Camera screen does not appear. But if I request CAMERA permission first and user grants it, then the Camera will show. |
It seems that while it works on a Samsung Galaxy A50 running Android 11, it neither works on any Android simulator SDK version (30-34), nor does it work on Pixel 8 running Android 14. "Doesn't work" means that after taking a photo and ok'ing it, the input change event does not fire. |
The intent passed into the |
The conditions need to be revised a little. See review below. |
@jacobg how do they need to be revised? |
Hi @KenCorbettJr Thanks for your pull request. Do you see a "review" that was opened and displayed above your last message. I'll paste it again here:The intent == null clause should be removed, and getData() condition moved to the top with non-null intent clause. The camera result may have an intent but without data. I observed this behavior when testing across real multiple devices and simulators. Sometimes the camera photo comes in with an intent but null data, and sometimes without any intent at all. I guess the implementations work differently. So like this: if (intent != null && intent.getData() != null) { // single file
LOG.v(LOG_TAG, "Adding file (single): " + intent.getData());
uris.add(intent.getData());
} else if (captureUri != null) { // camera
LOG.v(LOG_TAG, "Adding camera capture: " + captureUri);
uris.add(captureUri);
} else if (intent != null && intent.getClipData() != null) { // multiple files
ClipData clipData = intent.getClipData();
int count = clipData.getItemCount();
for (int i = 0; i < count; i++) {
Uri uri = clipData.getItemAt(i).getUri();
LOG.v(LOG_TAG, "Adding file (multiple): " + uri);
if (uri != null) {
uris.add(uri);
}
}
} And camera would not open for me without first requesting CAMERA permission. |
I applied your code review feedback. Should I also add a request for camera permission? Or somehow check if the app already has that permission? |
Thanks! I was able to get it working in my app by requesting permission in javascript code first before opening chooser: getFileEl () {
return document.getElementById(`file-${this.$.uid}`)
},
async requestCameraPermission () {
const diagnostic = window.cordova?.plugins?.diagnostic
if (!diagnostic) return true
const releasePermissionLock = await this.rootStore.acquireLockToRequestPermissions()
return new Promise((resolve, reject) => {
diagnostic.requestCameraAuthorization(
status => resolve(status === diagnostic.permissionStatus.GRANTED),
error => reject(error),
false
)
})
.finally(async () => {
releasePermissionLock()
})
},
async addDocument () {
if (this.saving) return
// The permissions and file picker will pause the webview, so we need to prevent that.
this.idleStore.setPreventPause(true)
// First we need to ask for camera permission, even though we don't know
// whether they'll choose camera or not.
// TODO: It seems Cordova needs to do this for us:
// https://github.com/apache/cordova-android/pull/1609#issuecomment-1905156891
const granted = await this.requestCameraPermission()
if (!granted) {
console.warn('User did not grant camera permission before adding document')
}
const fileEl = this.getFileEl()
if (!fileEl) {
console.warn('File input element not mounted after requesting camera permission')
return
}
// Open file picker.
fileEl.click()
}, It would be nice if the native code could intercept the user choosing Camera, and only then ask for permissions, rather than ask for permissions before showing the chooser even though user may choose from gallery. But I'm not sure why @breautek thinks intent doesn't need to grant permission? Maybe there's a different approach? |
I know there was behaviour which was handled in the camera plugin where if the Google has announced that starting in August 2024, that they are going to start cracking down on apps declaring or requesting permissions that the app doesn't actually use or need, so we need to pay close attention when it comes to adding new permission requirements. @jacobg If you could, open up the app that you're testing with inside Android Studio and view your |
Ah, so that explains it. My app does declare CAMERA in the manifest because it has a separate use-case where it needs the camera. If the user has not granted CAMERA permission yet for that other use-case, then the file input won't open until granting CAMERA permission there. So I guess there's two options here:
|
If it can be worked around, then I'd probably prefer that rather than to scope creep this PR. I think perhaps cordova-android can provide a public utility class that this feature can eventually tap into to assist in determining if it should request the camera permission or not. This way other plugins can also tap into the utility class (like the camera plugin), so that are not replicating this logic everywheres. I foresee the in-app-browser potentially using this as well, perhaps media-capture, etc... |
Can this be included in next minor releases? Thank you! |
Am I missing something or do we still not have parity with the web here because we cannot get video from an input element? |
@HazzMan2409 Yes, you are right. This pull request only added camera not video. |
Platforms affected
Android
Motivation and Context
Allowing the user to use the camera or file browser with an HTML file input tag. This is possible on the mobile web and in Cordova apps on other platforms, but on android, the only option is to choose from images in the gallery.
Closes #816
Description
Reworked the action tray to use a chooser intent in cordova\engine\SystemWebChromeClient.java to add an option for the camera.
Testing
I developed the changes in a plugin (https://www.npmjs.com/package/cordova-plugin-android-image-file-input) that copies in the updated file. The plugin works beautifully but is so hacky. I shouldn't need to patch CordovaLib. But the issues has been open since 2019, and another pull request (#1385) has been open since 2021.
Checklist
(platform)
if this change only applies to one platform (e.g.(android)
)