1456557 - Multiple files won't upload after selecting for HTML input element

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Andrei Lazar]

How to reproduce: 
1. open https://xianqiao.wang/test/firefox-file-upload/ in firefox for android
2. click upload
3. choose multiple file

Comment 1

[Andrei Lazar]

Bogdan, can you please confirm this?

Expected result: output the files size
Actual result: throws an exception

Comment 2

[Bogdan Surd, QA [:bsurd, NI]]

Device:
 - Sony Xperia Z5 (Android 6.0.1)

I can confirm that this is happening when selecting multiple files. The page trows an exception.

Comment 3

[Andrei Lazar]

Attachment: Bug 1456557 - Multiple files won't upload after selecting for HTML input element. (WIP)

Created support for multiple files input on the mobile part and removed redundant logic for uploading files from storage.
After selecting multiple files, a cursor loader is being used for fetching the files from storage. We have two different types of cursors, one for image files and one for videos.
However, the implementation of FilePicker.js is not fully compatible for multiple files input.

Review commit: https://reviewboard.mozilla.org/r/241142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241142/

Comment 4

[Andrei Lazar]

In the documentation https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file it's said that:

A file input's value attribute contains a DOMString that represents the path to the selected file(s). If the user selected multiple files, the value represents the first file in the list of files they selected. The other files can be identified using the input's HTMLInputElement.files property.

I followed this trail but with no success on FilePicker.js, so at the moment, even though multiple files are selected, only one is going to be uploaded.

Comment 5

[:sdaswani]

Mike we need some tech consultation on JavaScript on this - Andrei has tried to get the JS correct but he can’t get multiple files to upload. Can you take a look at his patch and see if you can advise on a fix?

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> In the documentation https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file it's said that

I don't think you need to worry about `<input>`. FilePicker.js appears to pass on the files with `this.file` [1]:


    EventDispatcher.instance.sendRequestForResult(msg).then(file => {
      this._filePath = file || null;
      this._promptActive = false;

      if (!file) {
        return;
      }

      if (this._domWin) {
        return this._domWin.File.createFromNsIFile(this.file, { existenceCheck: false });
      }

      return File.createFromNsIFile(this.file, { existenceCheck: false });

Under the hood, `this.file` comes from `this_filePath` that we set earlier on in this function [2]:

  get file() {
    if (!this._filePath) {
        return null;
    }

    return new FileUtils.File(this._filePath);
  },

To support multiple files, I think parts of FilePicker.js needs to be rewritten to support multiple files. fwiw, there is also a `this.files` (which currently just wraps one `this.file`) but it doesn't seem we use it all the time.

NB: before you do that, I don't know what state FilePicker.js' callers expect it to end up in (e.g. does the function return multiple files? Or does the caller actually access the value via FilePicker.file?) and, if the callers expect just a single file, how hard it will be to change the caller. I'd recommend doing simple tests first (e.g. try to return 2 hard-coded file Uris) to try to get the code working end-to-end to ensure it's reasonable/feasible before you invest in doing more work on this component.

Unfortunately, Gecko code, especially ones shared with desktop, is hard-to-change and, in my experience, hard to follow the code base of. As such, I don't actually know who is calling FilePicker.js to be able to tell you more and figure out how difficult this actually is! x_x

---

fwiw, it's possible to get a debugger working in JS land, if it's helpful (e.g. to verify that the first function is getting all the right files). iirc, you can use remote debugging to start a debugger session on the main process and set breakpoints in the UI or with `debugger;` JS statements. These docs [3] detail setting up remote debugging for a web page but I think the process is the same. tbh, I find Firefox's WebIDE to be unpredictable so I'd actually recommend using Chrome's remote dev tools (they use the same protocol and work interchangeably).

[1]: https://searchfox.org/mozilla-central/rev/53afcfdbabed96883126d0ebbcac499b358e32f2/mobile/android/components/FilePicker.js#230
[2]: https://searchfox.org/mozilla-central/rev/53afcfdbabed96883126d0ebbcac499b358e32f2/mobile/android/components/FilePicker.js#130

[3]: https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging/Debugging_Firefox_for_Android_with_WebIDE

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8972571 [details]
Bug 1456557 - Multiple files won't upload after selecting for HTML input element. (WIP)

https://reviewboard.mozilla.org/r/241142/#review247700

I didn't go through this super thoroughly yet because it's missing the final solution but the general idea lgtm!

fwiw, this could be easier to review if it was broken apart into several commits that each did one thing. That being said, don't waste a lot of time making the commits – only do it if it's actually convenient! For example, a few chunks that I see that can be broken apart are:
- Modify onActivityResult to support multiple files
- Modify `*LoaderCallbacks` to support multiple files
- Modify `FilePicker.js` to support multiple files

If done in order, this shows where the data initially comes from (the Android Intent -> onActivityResult) and evenually ends up (-> LoaderCallbacks -> FilePicker.js), making it easier to understand the content that has been changed.

::: mobile/android/base/java/org/mozilla/gecko/FilePickerResultHandler.java:67
(Diff revision 1)
>      }
>  
>      @Override
>      public void onActivityResult(int resultCode, Intent intent) {
>          if (resultCode != Activity.RESULT_OK) {
> -            sendResult("");
> +            sendResult(new String[]{});

nit: iirc, generally spacing is `new String[] {}` or `new String[] { "some strings" }`. If checkstyle doesn't complain (I sent it to try), I don't think we should waste time changing it in these commits but I just wanted to let you know for future reference.

::: mobile/android/base/java/org/mozilla/gecko/FilePickerResultHandler.java:228
(Diff revision 1)
> -            this.cacheDir = cacheDir;
> -            this.tabId = tabId;
>          }
>  
>          @Override
>          public Loader<Cursor> onCreateLoader(int id, Bundle args) {

This is a much better solution, thanks for cleaning things up! :)

::: mobile/android/base/java/org/mozilla/gecko/FilePickerResultHandler.java:233
(Diff revision 1)
>          public Loader<Cursor> onCreateLoader(int id, Bundle args) {
>              final Context context = GeckoAppShell.getApplicationContext();
> +            Uri[] filesUris;
> +            Parcelable[] parcelables = args != null ? args.getParcelableArray(FILES_PATHS_BUNDLE_KEY) : null;
> +
> +            if (parcelables != null) {

I see a lot of this code is duplicated with the other loader – can we share any code?

::: mobile/android/base/java/org/mozilla/gecko/FilePickerResultHandler.java:263
(Diff revision 1)
>          @Override
>          public void onLoadFinished(Loader<Cursor> loader, Cursor cursor) {
> +            List<String> filesNames = new ArrayList<>();
>              if (cursor.moveToFirst()) {
> -                String fileName = cursor.getString(0);
> -
> +                do {
> +                    filesNames.add(cursor.getString(cursor.getColumnIndex(DATA)));

nit: cache `cursor.getColumnIndex` instead of calling it each time: it's caused perf problems in the past.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8972571 [details]
Bug 1456557 - Multiple files won't upload after selecting for HTML input element. (WIP)

https://reviewboard.mozilla.org/r/241142/#review247710

Please re-add r? when you've finished the commit (or NI if you have more questions)!

Comment 9

[Anton]

There is very similar issue in Chrome. Please check it and see if it's helpful https://bugs.chromium.org/p/chromium/issues/detail?id=854425&can=2&start=0&num=100&q=&colspec=ID%20Pri%20M%20Stars%20ReleaseBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified&groupby=&sort=

Could it be due to a bug in some of the Android apps like Photos?

Comment 10

[p.scheit]

Android 6.0.1 Firefox 61.0

using accept="image/*"

- can't select multiple files (by pressing long) in gallery
- can't select multiple files via google photos (known bug in google photos)


using accept="*.jpg, *.png" or no accept attribute

- can't select multiple files (by pressing long) when using documents => gallery
- can't select multilpe files via documents =>google photos (known bug in google photos)
- after selecting multiple files with  google photos the upload file is unusable

- can select multiple files (by pressing long) when using "documents" => recent files: BUT only one file is selected in input.files-array

Hope this helps? I know this bug for over 2 years now ...

Comment 11

[Jovan Gerodetti]

Andrey are you still working on this? It would be great if this bug could get fixed.

Comment 12

[Andrei Lazar]

Yes, I postponed this just because Oreo migration has priority.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Chris, can you find someone to check if this also affects geckoview. 
It also doesn't sound like it's a regression, it's a new feature. So we might investigate a little further for fennec but not give it a top priority especially if it's fennec-only.

Comment 14

[Andreas Bovens [:abovens]]

I just checked in a GV-powered Fennec Custom Tab, and while I can check multiple files in the Android file picker, the page returns an error.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

I'm not sure this is supported in Firefox or in Chrome, currently. The workaround is to upload one file at a time.

Comment 16

[Håvard Wigtil]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> I'm not sure this is supported in Firefox or in Chrome, currently.

Do you mean that selecting multiple images is not supported in Chrome? My app works in Chrome 70 (and earlier) on Android, and Firefox desktop, but breaks in Firefox mobile.

Comment 17

[Chris Peterson [:cpeterson]]

Sending to GV triage based on Andreas's comment 14.

Comment 18

[u628319]

Is there a chance this gets picked up any time soon?

Comment 19

[nhinkle]

Still having this problem with Firefox 66.0.2 on Android 8.1.0. Works fine with Chrome 73 on Android, and both Chrome and Firefox on desktop.

• If you select multiple files, input.files will only have one file listed
• If you attempt to use a FileReader to read the file contents, you get an exception with NS_ERROR_FILE_TARGET_DOES_NOT_EXIST
• If you only select a single file, it shows up correctly and does not throw an exception when reading the file

Here is a codepen demonstrating the issue: https://codepen.io/anon/pen/jRZxmB

This is the exception on Android, captured using WebIDE with USB debugging:

https://i.imgur.com/mCgZnhx.png

NS_ERROR_FILE_TARGET_DOES_NOT_EXIST: pen.js:55
    <anonymous> Node
    forEach self-hosted:262
    <anonymous> Node
    _sendMessage jar:jar:file:///data/app/org.mozilla.firefox-HFFja-D3J4Q0co4WGNeWGw==/base.apk!/assets/omni.ja!/components/FilePicker.js:22

Comment 20

[Alex]

Still having this problem with Firefox 66.0.2 on Android. Please Fix It. Chrome on Android Works Fine.

Comment 22

[lrdarknesss]

It's also not working for me on Firefox Mobile 67 and Android 9.
This is a big problem, because everytime i make a public folder for pictures etc. on my Nextcloud, i have to tell the people who use it to use Chrome.
I've not found any other solution for uploading multiple files at once.
Uploading every picture one by one is not really a solution.

Is there anything that i can do to help fixing this problem asap?

Comment 23

[Nima]

Still having this problem with Firefox 67.0.3 on Android 8.0.0.

Comment 25

[Florian]

This bug is also in firefox68. I've got the same problem as @lrdarknesss

Comment 26

[[ex-Mozilla] Agi Sferro | :agi]

Looks like this works now.

Comment 27

[Diego Casorran [:diegocr]]

Nice it works now, but can i ask how and when was this fixed?

Some reference bug(s) and/or commit(s) at the time of closing/resolving would have been appreciated.

Comment 28

[[ex-Mozilla] Agi Sferro | :agi]

We completely rewrote the frontend of Firefox for Android, it would be really hard (if not impossible) to pin point what fixed this particular bug.