Skip to content
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

Merged
merged 12 commits into from
May 9, 2024

Conversation

KenCorbettJr
Copy link
Contributor

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

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

Copy link
Member

@dpogue dpogue left a 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-commenter
Copy link

codecov-commenter commented Apr 26, 2023

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           

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

@KenCorbettJr
Copy link
Contributor Author

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

@KenCorbettJr
Copy link
Contributor Author

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.

templates/project/res/xml/opener_paths.xml Outdated Show resolved Hide resolved
templates/project/res/xml/opener_paths.xml Outdated Show resolved Hide resolved
templates/project/res/xml/opener_paths.xml Outdated Show resolved Hide resolved
templates/project/res/xml/opener_paths.xml Outdated Show resolved Hide resolved
templates/project/AndroidManifest.xml Outdated Show resolved Hide resolved
templates/project/AndroidManifest.xml Outdated Show resolved Hide resolved
@erisu erisu changed the title (android) Allow image file input to be from camera feat: add camera intent with file input capture May 2, 2023
Copy link
Member

@dpogue dpogue left a 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 :)

@breautek breautek self-requested a review May 3, 2023 14:17
@KenCorbettJr
Copy link
Contributor Author

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

Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@erisu
Copy link
Member

erisu commented May 17, 2023

<!-- // 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 />
  • File browser appears
    • I can see all file types
  • File browser appears and I see only images
  • Chooser intent appears
    • I can select Camera or File
    • I see all file type
  • Chooser intent appears
    • I can select Camera or File
    • I see only image files
  • Multiple flag does not work.
    • I see all file type
  • Multiple flag does not work.
    • I see image only type
  • Chooser intent appears.
    • I can select Camera or File
    • I see all file type
    • Multiple flag doesn't seem to work
  • Chooser intent appears.
    • I can select Camera or File
    • I see only image files
    • Multiple flag doesn't seem to work
  • Chooser intent appears
    • I can select Camera or File

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:

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

Copy link
Member

@erisu erisu left a 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.

@erisu erisu added this to the 13.x milestone May 17, 2023
@ArturKp
Copy link

ArturKp commented Jul 25, 2023

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
Copy link
Contributor Author

@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
Copy link
Member

erisu commented Aug 17, 2023

I will try and re-test this PR tomorrow.

@christiaan
Copy link

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.

@christiaan
Copy link

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

@breautek
Copy link
Contributor

@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
Copy link
Member

erisu commented Sep 15, 2023

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.

@erisu erisu modified the milestones: 13.x, 12.1.0 Sep 15, 2023
@Scooby27
Copy link

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

@lovelyelfpop
Copy link

When will this be merged?

@jacobg
Copy link

jacobg commented Jan 22, 2024

What about requesting for camera permission?

@breautek
Copy link
Contributor

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
Copy link

jacobg commented Jan 23, 2024

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
Copy link

jacobg commented Jan 23, 2024

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
Copy link

jacobg commented Jan 23, 2024

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

@jacobg
Copy link

jacobg commented Jan 23, 2024

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

@KenCorbettJr
Copy link
Contributor Author

@jacobg how do they need to be revised?

@jacobg
Copy link

jacobg commented Jan 23, 2024

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
Copy link
Contributor Author

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
Copy link

jacobg commented Jan 23, 2024

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
Copy link
Contributor

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
Copy link

jacobg commented Jan 23, 2024

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
Copy link
Contributor

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
Copy link

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

@erisu erisu modified the milestones: 12.1.0, 13.0.0 May 8, 2024
@erisu erisu merged commit b773ae4 into apache:master May 9, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android clicking input type=file brings up system file browser, not camera options