PromptDelegate.onFilePrompt is passing an mimeTypes empty list but the input type file has multiple mime types

RESOLVED FIXED in Firefox 66

Status

defect
P1
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: amejiamarmol, Assigned: droeh)

Tracking

unspecified
mozilla66

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 months ago
After implementing [1] onFilePrompt from GeckoSession.PromptDelegate. I'm always getting an empty list for the parameter String[] mimeTypes even though the input type file has multiple mime types. 

To replicate this issue just try to load the Mozilla docs for input type file [2].

<pre>
<label for="avatar">Choose a profile picture:</label>

<input type="file"
       id="avatar" name="avatar"
       accept="image/png, image/jpeg">
</pre>


References:
[1] https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java#3510
[2] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file
Reporter

Comment 1

5 months ago
Tested on nightly version 65.0.20181129095546. and beta version 64.0.20181022150107

Updated

5 months ago
Product: Firefox for Android → GeckoView
Dylan says he can look at this bug because he's working on some other PromptDelegate issues.
Assignee: nobody → droeh
Priority: -- → P1
Assignee

Comment 3

4 months ago

The issue here is that it looks like appendFilter in GeckoViewPrompt.js was written as though it expects the actual filters (MIME types and extension filters of the form ".ext"), when in reality HTMLInputElement::SetFilePickerFiltersFromAccept parses these into lists of extension filters of the form "*.ext".

This patch updates nsIFilePicker to add a function, appendRawFilter, that is intended to be used to pass through the actual values from the accept attribute without processing them as above. Then we pass these directly to onFilePrompt in GV.

Attachment #9038331 - Flags: review?(snorp)
Attachment #9038331 - Flags: review?(bugs)
Comment on attachment 9038331 [details] [diff] [review]
Let nsIFilePicker accept unprocessed filters

Review of attachment 9038331 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLInputElement.cpp
@@ +6880,5 @@
>      // First, check for image/audio/video filters...
>      if (token.EqualsLiteral("image/*")) {
>        filterMask = nsIFilePicker::filterImages;
>        filterBundle->GetStringFromName("imageFilter", extensionListStr);
> +      filePicker->AppendRawFilter(token);

I think it'll be a little more readable if you break out `const bool isExtension = token.First() == '.'` and use that to conditionally call `AppendRawFilter`, then also replace it in the check below.
Attachment #9038331 - Flags: review?(snorp) → review+
Comment on attachment 9038331 [details] [diff] [review]
Let nsIFilePicker accept unprocessed filters

So why do we want to do something completely different with GeckoView than what we do elsewhere? I'm worried about inconsistencies and missing things like
https://searchfox.org/mozilla-central/rev/6c784c93cfbd5119ed07773a170b59fbce1377ea/dom/html/HTMLInputElement.cpp#6920-6931
Assignee

Comment 6

4 months ago

(In reply to Olli Pettay [:smaug] from comment #5)

Comment on attachment 9038331 [details] [diff] [review]
Let nsIFilePicker accept unprocessed filters

So why do we want to do something completely different with GeckoView than
what we do elsewhere? I'm worried about inconsistencies and missing things
like
https://searchfox.org/mozilla-central/rev/
6c784c93cfbd5119ed07773a170b59fbce1377ea/dom/html/HTMLInputElement.cpp#6920-
6931

Because the almost-certain use case here is to fire an ACTION_GET_CONTENT[0] intent to open a file picker, and that filters specifically/only on MIME types, not file extensions[1]. Obviously this still leaves the GV app with a decision to make, since the accept attribute can include file extensions; but it makes it far easier to handle cases (like the example case in this bug) where the accept values are all MIME types to begin with. With current behavior, we'd be leaving the app with the unenviable task of trying to recreate a MIME type from a list of valid extensions.

[0] - https://developer.android.com/reference/android/content/Intent.html#ACTION_GET_CONTENT
[1] - https://developer.android.com/reference/android/content/Intent.html#setType(java.lang.String)

Assignee

Comment 7

4 months ago

Updated to simplify when we call AppendRawFilter, should address snorp's issue.

Attachment #9038331 - Attachment is obsolete: true
Attachment #9038331 - Flags: review?(bugs)
Attachment #9038590 - Flags: review?(bugs)
Comment on attachment 9038590 [details] [diff] [review]
Let nsIFilePicker accept unprocessed filters

I don't see how this compiles on all the platforms.
appendRawFilter is implemented only for nsFilePickerProxy and FilePickerDelegate.
Am I missing something?

InfallibleTArray<nsString> mRawFilters; could probably move to nsBaseFilePicker and also
NS_IMETHOD AppendRawFilter(const nsAString& aFilter) override;
Attachment #9038590 - Flags: review?(bugs) → review-
Assignee

Comment 9

4 months ago

(In reply to Olli Pettay [:smaug] from comment #8)

Comment on attachment 9038590 [details] [diff] [review]
Let nsIFilePicker accept unprocessed filters

I don't see how this compiles on all the platforms.
appendRawFilter is implemented only for nsFilePickerProxy and
FilePickerDelegate.
Am I missing something?

InfallibleTArray<nsString> mRawFilters; could probably move to
nsBaseFilePicker and also
NS_IMETHOD AppendRawFilter(const nsAString& aFilter) override;

Sorry about that, I got a bit myopic and put it up for review after only running GV tests. I'll fix and flag you again after a successful try run on all platforms.

Assignee

Comment 10

4 months ago

Alright, I moved AppendRawFilter out of nsFilePickerProxy and into nsBaseFilePicker and remembered to do a try run on all platforms this time.

Attachment #9038590 - Attachment is obsolete: true
Attachment #9038855 - Flags: review?(bugs)

Updated

4 months ago
Attachment #9038855 - Flags: review?(bugs) → review+

Comment 11

4 months ago
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab592ce5d5ae
Add appendRawFilter to nsIFilePicker to expose actual accept filters to GV for onFilePrompt. r=snorp, smaug

Comment 12

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.