search

apache / cordova-android

Apache Cordova Android
https://cordova.apache.org/
Apache License 2.0
3.59k stars 1.52k forks source link

feat: add camera intent with file input capture #1609

Closed KenCorbettJr closed 2 days ago

KenCorbettJr commented 1 year ago

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 (https://github.com/apache/cordova-android/pull/1385) has been open since 2021.

Checklist

codecov-commenter commented 1 year ago

Codecov Report

Merging #1609 (cf3be88) into master (cb48147) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1609   +/-   ##
=======================================
  Coverage   72.61%   72.61%           
=======================================
  Files          23       23           
  Lines        1800     1800           
=======================================
  Hits         1307     1307           
  Misses        493      493

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

KenCorbettJr commented 1 year ago

Sure! I went ahead and removed those from this pull request.

KenCorbettJr commented 1 year ago

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.

KenCorbettJr commented 1 year ago

What additional testing needs to be done to get this merged in?

erisu commented 12 months ago 
<!-- // 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. These were run on Android 13 device. I created a blank Cordova default app with no additional plugins.

End results:

This PR will be pushed back for a later release, until the above issues are resolved.

ArturKp commented 9 months ago

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?

KenCorbettJr commented 9 months ago

@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 capture attribute on the file input I was shown the option to take a picture using my camera. The multiple and accept attributes seemed to work great too.

@erisu is there any way you can take another look with my test app? I am pretty sure I set it up correctly.

erisu commented 8 months ago

I will try and re-test this PR tomorrow.

christiaan commented 8 months ago

End results:

  • The chooser intent displays as expected.
  • The camera option does not seem to return the image. The file selector continues to say "No file chosen"
  • The multiple file selector flag does not seem to work. Only can select one file.

This PR will be pushed back for a later release, until the above issues are resolved.

@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.

erisu commented 8 months ago

@christiaan I do not recall if I long-pressed when testing.

But, as of right now, I am stuck on how to proceed with this PR.

I stumbled across this StackOverflow, which seems similar in code, almost word for word. There are slight differences, probally due to previous feedback/suggestions... But even the newly added LOG outputs and comment lines are identical.

https://stackoverflow.com/questions/57586259/cordova-android-clicking-input-type-file-brings-up-system-file-browser-not-came

What I am stuck on is this Apache rule: https://www.apache.org/legal/resolved.html#stackoverflow

Can I Copy Code From Stack Overflow And Contribute It To An ASF Project?

No, not without contacting the original author and getting permission from them to use the code in an Apache project under the Apache License 2.0.

Even though on StackOverflow's Terms of Services, under "Content Permissions, Restrictions, and Creative Commons Licensing" for "Subscriber Content", it says:

You should be aware that all Public Content you contribute is available for public copy and redistribution, and all such Public Content must have appropriate attribution.

It still goes against Apache's statement.

I might have to reach out to Apache Legal and confirm if their statement still stands, knowing what StackOverflow's Terms of Services says..

I just noticed the SO post and started dwelling on this...

christiaan commented 8 months ago

@NoodleOfDeath you are the author of the Stack Overflow answer which appears to be one on one copied to this PR. Would you mind if it is incorporated in cordova android?

NoodleOfDeath commented 8 months ago

By all means!

christiaan commented 8 months ago

@erisu Is this enough permission? :smile:

christiaan commented 7 months ago

@breautek @erisu not to sound impatient but, what is holding this back from being merged?

breautek commented 7 months ago

@breautek @erisu not to sound impatient but, what is holding this back from being merged?

It's in the 13.x milestone, I'm not really sure why it was placed there. I don't think there is anything actually breaking, @erisu do you remember why this was placed in in the 13.x milestone?

erisu commented 7 months ago

There was no specific reason; it simply didn't have time to be included in the 12.x milestone, which was designated for the major release. Therefore, I chose another milestone randomly.

As it's a new feature, it can be included into the next minor release, 12.1.x.

Scooby27 commented 5 months ago

Hi all, is there any timeline this will be released? Thanks!

lovelyelfpop commented 3 months ago

When will this be merged?

jacobg commented 3 months ago

What about requesting for camera permission?

breautek commented 3 months ago

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.

jacobg commented 3 months ago

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.

jacobg commented 3 months ago

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.

jacobg commented 3 months ago

The intent passed into the startActivityForResult callback has null data, i.e., intent.getData() == null. Can't figure out what would cause that.

jacobg commented 3 months ago

The conditions need to be revised a little. See review below.

KenCorbettJr commented 3 months ago

@jacobg how do they need to be revised?

jacobg commented 3 months ago

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.

KenCorbettJr commented 3 months ago

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?

jacobg commented 3 months ago

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?

breautek commented 3 months ago

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 CAMERA permission is declared in the manifest, then the CAMERA permission needs to be granted at the app-level. (Which somewhat defeats the purpose of using Intents...) Perhaps you have the CAMERA permission declared in your android manifest somehow, via another plugin or something. The camera plugin handles this by checking if the camera permission is present in the manifest and only request the camera permission if it is.

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 AndroidManifest.xml file. The Android Studio editor for the manifest file will have a tab for Merged Manifest view, which should represent your manifest as natively compiled. If you can see that the CAMERA permission is declared, it should tell what library/module it is coming from. Likely it will be the "app" module since a plugin that you have installed is probably adding it. If you could temporarily remove the CAMERA declaration in the manifest and re-test, I think you should find that it will work as expected without the camera permission. You would also of course have to temporarily disable any code that attempts to request the camera permission as well.

jacobg commented 3 months ago

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: 1) App request permission in javascript if it needs it. 2) Cordova can check if CAMERA is in the manifest, and request permission.

breautek commented 3 months ago

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:

  1. App request permission in javascript if it needs it.
  2. Cordova can check if CAMERA is in the manifest, and request permission.

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...

martjag12 commented 2 months ago

Can this be included in next minor releases? Thank you!