Closed Bug 826185 Opened 11 years ago Closed 10 years ago

Inconsistent <input accept=...> behavior

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: keremonal, Assigned: arnaud.bienner)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 3 obsolete files)

Draft: more trusted mime type filters
3.69 KB, patch
Details | Diff | Splinter Review
removeGetFilterFromAccept.patch
3.51 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
bug826185-3.patch
7.81 KB, patch
arnaud.bienner
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.45 Safari/537.17

Steps to reproduce:

http://jsfiddle.net/lembas/4JSsJ/1/




Actual results:

Firefox lists images as default when we use <input accept=image/*>. 
But when we use <input accept=application/pdf>, Firefox does not list PDF files as default. 


Expected results:

For all types of mime types, <input accept=...> should filter the file list or not (Preferably the default behavior is to filter results according to the accept value, I think that is what a web developer expects from the accept attribute)
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
CCing Jonas and Arnaud who were part of the discussion about adding mime-type support to <input accept=''>.

Rational about this decision has been made in bug 565274.
Blocks: 565274
What I would like to report is a bit different from bug 565274.

Using <input accept=image/*> filters files but <input accept=image/png> does not. It simply puts an options to the file type combobox. But this is not selected as default. 

What I expect is that either <input accept=image/*> should behave like <input accept=image/png> or otherwise.

http://jsfiddle.net/lembas/4JSsJ/2/
Please refer to the discussion we had in bug 565274.

To summarize, it has been decided to make image/* video/* and audio/* filters as default, because their extension list is hard-coded in Firefox.
For the other (mime type) filters, OS's mime type <-> extensions mapping is involved, and it may changed from a system to another, and that's one of the reason why these filters are not selected by default.

However, Jonas stated in comment 21 that we should make these filters non-default in a first step, waiting for users feedback (like yours ;) to maybe change this behavior.

IMHO also having mimetype filter selected by default is a good idea, but indeed, it was wise to wait for some feedback about this.

If it's finally decided to change the behavior, I'm fine to write the corresponding patch.

About the last point you mentioned in bug 773985, indeed, allowing filters based on extensions list instead of mimetype might be nice, so I think you can open a new bug for this (I think no one has already been created), so we can discuss the opportunity of this, and how it should be implemented.
On all platforms that Firefox ship on, we can only do filtering based on file names. I.e. platforms doesn't track the actual mime types of files, the only thing we have to go on is the extension of the file.

Hence we have to map the mimetype from the accept attribute to a list of extensions that we filter on. We can only do that for mimetypes that we know well, which means that it has to be a hardcoded list of mimetypes that is mapped to a hardcoded list of extensions.

Hence there is no way we can always create a filter.

We can argue about which set of mimetypes to hardcode, and which list of extensions to hardcode them to, but that's not what this bug is filed on.

This means that the only possible solution to this bug would be to completely remove support for the accept attribute. I think that's a worse behavior than what we currently have. I'd rather add additional hardcoded filters, such as application/pdf. If you have good use cases, then hardcoding application/png might make sense too.

However as filed, I think the only two possible solutions to this bug are:
* Remove support for accept attribute. The people that have filed bugs requesting that we add the support that we currently have, clearly think this would be a bad change. And I agree with them.
* Support all mime types that can be put in the accept attribute. This is unfortunately impossible with how OSs work today.

Hence I think this bug is a WONTFIX unfortunately.
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
IMHO, inconsistency is the worst behavior. At least we could have changed the default filter of image/* to non-default so anyone can say that "Firefox is consistent and its interpretation of the standard is this way".

I think implementing "file extension filter" mentioned in http://www.w3.org/html/wg/drafts/html/master/forms.html#attr-input-accept
is the right way to go.

I opened a bug on this:
https://bugzilla.mozilla.org/show_bug.cgi?id=826176
In browser which implement accept with mimetype (Chrome, Opera), filter is always selected by default.
From what I remember when looking at Chrome source code, Chrome resolves mimetypes->file extensions the same way as Firefox does, if I understood the code well, and make this filter the default one anyway.
I don't know how it works internally for Opera.

But I understand Jonas' point of view: so this is just to complete previous comments ;)
If we really want to not make mime-type filters the default one, I  propose to change the mime service's API a bit to get only mime-types from our hard-coded set and to consider them as trusted (or we may continue to have mime type from OS as a second filter anyway if you think it's worth it).

Here is a small patch which does this.
It's just a draft and it deserves to be improved (and tests to be updated etc.), but it's just to demonstrate what we can do.

Basically, I've added another method in |nsExternalHelperAppService| to fill a mimeinfo from the hard-coded set (default entries + extra set) and to override mime info's existing extensions.
There are already similar methods in this class to do similar things but using only the "extra" set. And they're protected.
Maybe it would be better to access this method through the mime service, but I guess we want to keep it simple though.

In |nsHTMLInputElement| we get a mimeinfo as the usual way, but try to override it using the new function and, if we succeed, we mark the corresponding filter as trusted, so it will be displayed by default.

Jonas, Mounir, I would like to have your feedback about this. Let me know if you want something to be clarified.

To summarize remaining questions are:
- How to properly change the API to do this (I'm not sure what I did is the best way, even if it doesn't look that bad).
- Should we still use the OS mapping for mime type, or completely get rid of it? As we now have another reliable source which contains most common mime types, this makes sense IMHO.
Attachment #723682 - Flags: feedback?(mounir)
Attachment #723682 - Flags: feedback?(jonas)
Comment on attachment 723682 [details] [diff] [review]
Draft: more trusted mime type filters

I'll defer to Mounir
Attachment #723682 - Flags: feedback?(jonas)
Re-opening the bug otherwise the feedback request will not show up in my dashboard.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
Comment on attachment 723682 [details] [diff] [review]
Draft: more trusted mime type filters

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

Regarding the changes in Mime Type Service, the best person to ask I think would be bz.

About the HTMLInputElement changes, I am still wondering why are we considering Mime Type from the system as untrusted and how often would a user have messed up mime types. I asked this in another bug but didn't get any answer IIRC.
Attachment #723682 - Flags: feedback?(mounir)
(In reply to Mounir Lamouri (:mounir) from comment #12)
> About the HTMLInputElement changes, I am still wondering why are we
> considering Mime Type from the system as untrusted and how often would a
> user have messed up mime types. I asked this in another bug but didn't get
> any answer IIRC.
Indeed, you asked this in bug 826176, and I just gave a short answer.

Not trusting mime type mapping from OS and make corresponding filters non selected by default has been decided in bug 565274: see comments from 12 to 16.

Particularly, Jonas said: "However that mapping will vary wildly from user to user. So something that works on the developer's machine is very likely to not work on some users machine. [...] I guess I would be ok with defaulting to *.* as a filter, but having a secondary filter which is based on the OS mime service."
So Mounir, what is your opinion about this?

Btw, IMO this bug is blocking bug 826176, because, as you stated in bug 826176: comment 18, people will probably use only file extension filters (if we decide to implement bug 826176) until this bug will be fixed.

Should we:
- just remove the "trusted" logic and keep the current mimetype->file extensions resolver (which used OS mapping)?
- use another resolver based only on our (already existing) hard-coded set of mimetypes, as my previous patch does?
It would be wonderful if MIME types would work with accept.  Currently I know application/pdf doesn't.
(In reply to Matt Langston from comment #15)
> It would be wonderful if MIME types would work with accept.  Currently I
> know application/pdf doesn't.

application/pdf does work, but the filter isn't selected by default.
This bug is here to discuss the opportunity to change the current behavior.
No longer blocks: 565274
Depends on: 565274
Blocks: 826176
(from bug 826176 comment 22, where I first mistakenly posted this comment instead of posting it here)

Thinking again about this bug, I would like to have it fixed now :)
We got some feedback about this, only about the inconsistent behavior, but not (as far as I know) about the feature working poorly.

So I think we should get rid of the "trusted" logic, which makes things unnecessary complex.

After all, IMHO, it's not very important if the mimetype resolution is using OS's data or not (for me it will just be "bonus" for some mime types so it's good to keep it).
But if so, I think I should just improve the patch I did (attachment 723682 [details] [diff] [review]) and always look to the hard-coded list of mime type only.
I don't think it's a big deal to add few other well known mime type mapping to this list if we think some are missing.

The real problem I see is unknown mime type.
Currently we just ignore them.
What if it's just a non famous mime type, but "valid" anyway from a user point of view?
If we have only one mimetype, it's fine because we keep the "All files" filter.
But if we have several mimetypes (e.g. accept="image/*,application/rtg") we will filter out only images for most people :(
In this case, it might be great to create a filter anyway, with extensionList = *.*

To summarize:
1) How do we deal we unknown mime types?
 * Proposal: Don't ignore them but set extensionList to *.*
2) Remove the "trusted" logic and decide if we use only our hard-coded set or also OS mapping.

Jonas, I would like to have your point of view about my proposal.
Flags: needinfo?(jonas)
OS: Windows 7 → All
Hardware: x86_64 → All
I don't quite understand the proposal. In part because your first item in your proposal is a question :) But also because I don't quite remember what the current logic we use is.

Could you describe what you think we should do?
Flags: needinfo?(jonas)
OK, I will try to be clearer :)

* What we should do IMHO:
1) When an accept attribute is specified, always have the corresponding filter selected by default.
2) If a mimetype is unknown (maybe the base we looked in is just incomplete?) don't ignore it but set its extensionList to "*.*".
That way, by default, we will not hide ".mp" files for people who don't have Maya installed, for example.
3) In case we have multiple values specified in accept: keep the "All valid filters" and select it by default (except in case of 2 maybe).

* As a reminder, current situation:
- image/*, video/* and audio/* results in having this filter selected by default, or the "All valid filter" (which is the concatenation of all filters) selected by default. (filterAll (*.*) remains available, but is not the default)
- Having another mimetype (e.g. application/pdf) results in having filterAll selected by default instead.
This was done because you weren't sure how well this will work (partly because "mimetype -> file extensions" lookup uses OS information) IIRC, so we can have a chance to know how well the mapping is working (bug 565274 comment 21).
- "All valid filters" is selected by default only if accept's value contains only image/*, video/*, audio*.

IMHO, this bug and all the duplicate clearly show how people expect the accept attribute to behave: the mimetype associated filter should be selected by default.

It is quite clear to me how to implement this: I can provide a patch of my proposition if you think it can help to make things clearer.
While working on this bug, I realized this function isn't needed anymore.
I remember I wanted to remove it while working on bug 565274, but at that time, it was used somewhere else at that time IIRC.
Anyway, the logic inside isn't the one we decided to have finally in bug 565274: in addition of adding mimetype support, we decided to allow multiple filters (e.g. accept="image/*,video/*") while this function considers multiple filters as invalid.

Asking you, Jonas, for review, but feel free to delegate to someone else.
Attachment #8390483 - Flags: review?(jonas)
A patch to demonstrate what I meant.
Tests updated as well: this might help to understand what is the expected behavior under different circumstances.

- No more "trusted" logic: all filters are considered "trusted", so will be selected by default, no matter if they come from the OS resolver or not.
- Because OS mime type might be broken (probably very unlikely but...) or just miss some mime type (more likely), unknown mime types result in a filter anyway, but which filter all files (so we don't unnecessary restrict user selection).
In this case, because "All supported type" will contain "*", better to make "All filter" the default filter.

Btw, the spec suggests it's fine to user OS mime type resolver to get files extensions: http://www.w3.org/html/wg/drafts/html/master/forms.html#attr-input-accept
"MIME types can be used with the system's type registration table (mapping MIME types to extensions used by the system), if any, to determine any other extensions to allow"
Attachment #8391248 - Flags: review?(jonas)
I forgot to mention to mention that I also refactored a bit the SetFilePickerFiltersFromAccept method, which started to be a bit too huge IMO, and created GetFilterNameFromMimeType and GetFilterExtensionsFromMimeType helper functions. Hopefully, this makes the code clearer.
Comment on attachment 8391248 [details] [diff] [review]
Change accept behavior for input type="file"

Hi Jonathan,
I asked Jonas first for review, as he worked with me on this in bug 565274, but he's busy atm, and suggested you might have a look to this.
Do you have any opinion about changing the current behavior, as this patch does?

Don't hesitate to ask me to summarize the whole discussion about "accept" attribute support: there are lot of comments here and in bug 565274, and I think it might be difficult to get in without any context explanation :)
Attachment #8391248 - Flags: review?(jwatt)
The main concern here is that I don't want to rely too much on data in the OS. The stuff there is highly dependent on what apps the user happens to have installed, and which versions of those apps has been installed at some point.

The other thing that I think we should keep in mind is that users in most cases likely do not know how to change the default filter.

With those things in mind, happy to let jwatt drive this.
Comment on attachment 8391248 [details] [diff] [review]
Change accept behavior for input type="file"

There's pretty much zero chance of me finding time to look at or think about this properly in the next two and a half weeks, then I'm off for a week and a half, and then I expect to be really busy again once I get back. Cancelling review for now and suggesting you find someone else. If that doesn't work out feel free to re-request review and I'll do my best to make some time for this in a month or so.
Attachment #8391248 - Flags: review?(jwatt)
Comment on attachment 8390483 [details] [diff] [review]
removeGetFilterFromAccept.patch

Punting these to Jwatt as I don't have time for forms stuff these days :(
Attachment #8390483 - Flags: review?(jonas) → review?(jwatt)
Attachment #8391248 - Flags: review?(jonas) → review?(jwatt)
Ping? I found this bug after answering something on https://stackoverflow.com/questions/23590301/input-accept-attribute-in-firefox.
Jonathan, could you please at least review the first patch (attachment 8390483 [details] [diff] [review]), which is just a trivial clean up patch? (old function not used anywhere now).

Happy to discuss about the second patch when you will be less busy (here or on IRC).
Flags: needinfo?(jwatt)
Attachment #8390483 - Flags: review?(jwatt) → review+
Flags: needinfo?(jwatt)
Keywords: checkin-needed
Hey Guys,

could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Keywords: checkin-needed
I don't think we need to run anything on try for this patch (attachment 8390483 [details] [diff] [review]): this is just a clean up patch, which removed an now unused function.
I don't expect this to fail anywhere, and, from what I've been told, we should avoid try builds when they aren't really needed (which is the case here IMO).
If you insist, I can push to try, but I don't think it's needed.
Keywords: checkin-needed
Keywords: leave-open
Comment on attachment 8391248 [details] [diff] [review]
Change accept behavior for input type="file"

As discussed on IRC some time ago, this would require a fair bit of time for me to work through the issues to the extent I'd be able to help decide on the behavior and do reviews. Unfortunately I don't have time for that in the medium term, so cancelling review for now to more accurately reflect the prospects of this getting review from me.
Attachment #8391248 - Flags: review?(jwatt)
Comment on attachment 8391248 [details] [diff] [review]
Change accept behavior for input type="file"

Olli might want to review this.
Attachment #8391248 - Flags: review?(bugs)
Comment on attachment 8391248 [details] [diff] [review]
Change accept behavior for input type="file"

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

::: content/html/content/src/HTMLInputElement.cpp
@@ +7142,5 @@
>  }
>  
> +// static
> +nsString
> +HTMLInputElement::GetFilterNameFromMimeType(const nsCOMPtr<nsIMIMEInfo>& aMimeInfo) {

* Don't return nsString; take nsAString& instead.
* Take nsIMIMEInfo*, not const nsCOMPtr<nsIMIMEInfo>&.
* { goes on the next line

@@ +7157,5 @@
> +}
> +
> +// static
> +nsString
> +HTMLInputElement::GetFilterExtensionsFromMimeType(const nsCOMPtr<nsIMIMEInfo>& aMimeInfo) {

Ditto

@@ +7261,5 @@
> +      // all (*) files.
> +      allFilterAreValid = false;
> +      filterName = token;
> +      nsDependentString allFilterStr;
> +      allFilterStr.AssignASCII(nsFilePickerFilter::allFilter);

AssignLiteral

::: content/html/content/src/HTMLInputElement.h
@@ +1351,5 @@
>      bool operator== (const nsFilePickerFilter& other) const {
>        if ((mFilter == other.mFilter) && (mFilterMask == other.mFilterMask)) {
> +        nsDependentString allFilterStr;
> +        allFilterStr.AssignASCII(nsFilePickerFilter::allFilter);
> +        if (!mFilterMask && mFilter.Equals(allFilterStr)) {

EqualsLiteral
Though before spending more time on this patch, I'd recommend finding a DOM peer that can actually do a full review.
It is in my queue, and I'm happy that Ms2ger skimmed it over.
(My review queue isn't too short atm.)
Comment on attachment 8391248 [details] [diff] [review]
Change accept behavior for input type="file"

But actually, perhaps baku could take a look.
Please put the patch back to my queue if you don't feel comfortable reviewing this.
Attachment #8391248 - Flags: review?(bugs) → review?(amarchesini)
Note that the main thing we need review of is not the code itself. But rather we need review of the behavior that it implements.
(In reply to Jonas Sicking (:sicking) from comment #43)
> Note that the main thing we need review of is not the code itself. But
> rather we need review of the behavior that it implements.

Indeed, the main problem here is to agree on the behavior we want.

I will try to summarize the current situation. Please correct me if I'm wrong.

When I implemented MIME type support in accept attribute (bug 565274), I used nsIMIMEService that mainly uses OS mapping<->extension mapping to convert mime types to extension list, because we can only filter based on extension most of the time (one exception is Android).
On the other hand, image/* video/* audio/* (previously implemented) use a hard coded list of extensions.
I was in favor of using nsIMIMEService (then OS resolver) to avoid reinventing the wheel, and to avoid the pain of maintaining our own MIME type <-> extensions resolver, while the OS is already doing this for us.
Jonas disagreed to let the OS resolver do everything, because the resolver might be wrong, and because some mime types might be missing (e.g. on Windows application/ms-word will not have any extension associated until Office is installed).
So, in a first step, we finally landed my patch, but with those filters not being selected by default (unlike video/image/audio).
The feedback we're getting (see all duplicate of this bug) clearly suggest that having mime types filter behaving differently is not what users expect.

I believe the main reason why Jonas didn't want to rely on OS mapping was that we might end up unnecessarily restricting user input in case the mapping is incomplete.
So what I suggest is to always make the mime type filter the default, but to map unknown mime types to "*" extension list to not unnecessarily restrict user input (currently unknown mime types are just ignored).
Another thing I think Jonas was worried about is that the accept will not behave similar everywhere. For instance, accept="application/ms-word" will correctly add a filter with ".doc,docx" extension list for someone, while it will not work for someone else. With my change, all users will have a "application/ms-word" filter, but some of them will have it associated to "*". Even if the file picker will not filter out anything, they will have a hint about the type of file to select. Also, note that those users will be the one who don't have Office installed, so they are less likely to want to upload a Word document IMHO.

Btw, the spec suggests it's fine to use OS mime type resolver to get files extensions: http://www.w3.org/html/wg/drafts/html/master/forms.html#attr-input-accept
"MIME types can be used with the system's type registration table (mapping MIME types to extensions used by the system), if any, to determine any other extensions to allow"

But, to be complete about this, if you're really worried about the mapping not working the same way for everyone, alternatives would be either to:
- drop mime type for accept support. If we support bug 826176, that might make sense. But IMHO it's nicer to let author not to worry about which extensions they should have (it's easier and less error prone to just write accept="image/jpeg" instead of accept=".jpeg,jpg", so you don't miss any extension). Also, that would not be compliant with the spec.
- or hard code MIME types <-> extension list mapping, like we currently do for image/video/audio. More work to do for us, mainly because we will be asked to update this list regularly I guess. However having a good set of common mime types should fit main users' needs.

Well, that was a bit long, sorry :/
Hope it's clear anyway.
Happy to discuss this in more details, and to go implement which solution sounds the best, even if that means rewriting the feature completely.
Comment on attachment 8391248 [details] [diff] [review]
Change accept behavior for input type="file"

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

I would like to review it again when all these comments are applied. Thanks!

::: content/html/content/src/HTMLInputElement.cpp
@@ +7143,5 @@
>  
> +// static
> +nsString
> +HTMLInputElement::GetFilterNameFromMimeType(const nsCOMPtr<nsIMIMEInfo>& aMimeInfo) {
> +  nsString filterName;

nsAutoString

@@ +7146,5 @@
> +HTMLInputElement::GetFilterNameFromMimeType(const nsCOMPtr<nsIMIMEInfo>& aMimeInfo) {
> +  nsString filterName;
> +  // Get a name for the filter: first try the description, then the mime type
> +  // name if there is no description
> +  aMimeInfo->GetDescription(filterName);

what about the error code?

@@ +7148,5 @@
> +  // Get a name for the filter: first try the description, then the mime type
> +  // name if there is no description
> +  aMimeInfo->GetDescription(filterName);
> +  if (filterName.IsEmpty()) {
> +    nsCString mimeTypeName;

nsAutoCString

@@ +7149,5 @@
> +  // name if there is no description
> +  aMimeInfo->GetDescription(filterName);
> +  if (filterName.IsEmpty()) {
> +    nsCString mimeTypeName;
> +    aMimeInfo->GetType(mimeTypeName);

error code.

@@ +7158,5 @@
> +
> +// static
> +nsString
> +HTMLInputElement::GetFilterExtensionsFromMimeType(const nsCOMPtr<nsIMIMEInfo>& aMimeInfo) {
> +  nsString extensionListStr;

nsAutoString

@@ +7160,5 @@
> +nsString
> +HTMLInputElement::GetFilterExtensionsFromMimeType(const nsCOMPtr<nsIMIMEInfo>& aMimeInfo) {
> +  nsString extensionListStr;
> +  nsCOMPtr<nsIUTF8StringEnumerator> extensions;
> +  aMimeInfo->GetFileExtensions(getter_AddRefs(extensions));

error code.

@@ +7164,5 @@
> +  aMimeInfo->GetFileExtensions(getter_AddRefs(extensions));
> +
> +  bool hasMore;
> +  while (NS_SUCCEEDED(extensions->HasMore(&hasMore)) && hasMore) {
> +    nsCString extension;

nsCAutoString

@@ +7165,5 @@
> +
> +  bool hasMore;
> +  while (NS_SUCCEEDED(extensions->HasMore(&hasMore)) && hasMore) {
> +    nsCString extension;
> +    if (NS_FAILED(extensions->GetNext(extension))) {

A comment is needed here. Why we want to continue if 'getNext' fails?

::: content/html/content/src/HTMLInputElement.h
@@ +789,5 @@
> +   * @param aMimeInfo a valid mime info
> +   * @return a name for the filter: the mime type's description if there is one;
> +   * the mime type name otherwise
> +   */
> +  static nsString GetFilterNameFromMimeType(const nsCOMPtr<nsIMIMEInfo>& aMimeInfo);

This should be:
static nsresult GetFilterNameFromMimeType(nsIMIMEInfo* aMimeInfo, nsAString& aFilterName);

@@ +798,5 @@
> +   * @param aMimeInfo a valid mime info
> +   * @return a string representation of the extension list: ";" separated
> +   * values, with value being like *.ext
> +   */
> +  static nsString GetFilterExtensionsFromMimeType(const nsCOMPtr<nsIMIMEInfo>& aMimeInfo);

Same here.

@@ +1329,5 @@
>    }
>  
>    struct nsFilePickerFilter {
> +
> +    static const char* allFilter;

sAllFilter;
Attachment #8391248 - Flags: review?(amarchesini) → review-
Comment on attachment 8391248 [details] [diff] [review]
Change accept behavior for input type="file"

Please have a look at previous comments: before reviewing the code, we need to agree on the behavior we want. The patch is just here to illustrate what I meant, but before going further, we need to know if this is the direction we want to go or not.
Attachment #8391248 - Flags: review- → review?(amarchesini)
> Please put the patch back to my queue if you don't feel comfortable
> reviewing this.

Yeah. I'll do it.
Attachment #8391248 - Flags: review?(amarchesini) → review?(bugs)
(In reply to Arnaud Bienner from comment #45)
> So what I suggest is to always make the mime type filter the default, but to
> map unknown mime types to "*" extension list to not unnecessarily restrict
> user input (currently unknown mime types are just ignored).
I think I like this approach


> Another thing I think Jonas was worried about is that the accept will not
> behave similar everywhere. For instance, accept="application/ms-word" will
> correctly add a filter with ".doc,docx" extension list for someone, while it
> will not work for someone else. With my change, all users will have a
> "application/ms-word" filter, but some of them will have it associated to
> "*". Even if the file picker will not filter out anything, they will have a
> hint about the type of file to select.
What hint? Where?


> Btw, the spec suggests it's fine to use OS mime type resolver to get files
> extensions:
> http://www.w3.org/html/wg/drafts/html/master/forms.html#attr-input-accept
Random note, we're not trying to implement W3 HTML spec, but WhatWG's HTML spec.
So I'm not that worried about supporting different filters based on what the OS knows.
In case the OS doesn't know about the mimetype, nothing should be filtered out, and I guess that is what the patch is doing (but I don't know what that "hint" is about. Does Filepicker in some OSes show the expected mimetype or what?).
Comment on attachment 8391248 [details] [diff] [review]
Change accept behavior for input type="file"

Could you explain and update the patch per baku's comments and ask review again.
Attachment #8391248 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #49)
> What hint? Where?
What I meant is, for example, if we cannot map "image/jpeg" to a list of extension, we will create a *.* filter, and it will named "image/jpeg". While this filter not filter anything, at least user will see "image/jpeg (*.*)" instead of "All files (*.*)" in the filter section of the file picker.
The filter being named "image/jpeg" is what I called "hint".

> Random note, we're not trying to implement W3 HTML spec, but WhatWG's HTML spec.
Thanks for the clarification. WhatWG's spec is similar regarding this though (not sure if you noticed, or if you just want to make things clear): https://html.spec.whatwg.org/multipage/forms.html#attr-input-accept
(In reply to Arnaud Bienner from comment #52)
> (In reply to Olli Pettay [:smaug] from comment #49)
> > What hint? Where?
> What I meant is, for example, if we cannot map "image/jpeg" to a list of
> extension, we will create a *.* filter, and it will named "image/jpeg".
> While this filter not filter anything, at least user will see "image/jpeg
> (*.*)" instead of "All files (*.*)" in the filter section of the file picker.
> The filter being named "image/jpeg" is what I called "hint".
That sounds super weird to me. In the filepicker UI you have a "filter" enabled, but 
actually that filter doesn't do anything.
Why not just ignore the filter if Gecko nor OS have mapping to the
mimetype.
(In reply to Olli Pettay [:smaug] from comment #51)
> Could you explain and update the patch per baku's comments and ask review
> again.

Sure. Actually my patch also refactored some part of the code, to (hopefully) make things easier to read. I believe I should leave this for a second step and just provide a patch that implement the discussed behavior, to start with (+ updated tests, which will help to understand how the behavior will change hopefully).
(In reply to Olli Pettay [:smaug] from comment #53)
> That sounds super weird to me. In the filepicker UI you have a "filter"
> enabled, but 
> actually that filter doesn't do anything.
> Why not just ignore the filter if Gecko nor OS have mapping to the
> mimetype.

That would be OK if there is only one filter.
But author can provide multiple mime type in the accept attribute. So if one of them is unknown, IMO we should not simply ignore it, because we might unnecessarily restrict user input.
In that case we can indeed ignore it, but we should default to "All files" filter I think.
Also, if for example we have accept="image/jpeg,image/png" but "image/jpeg" is unknown, the filter list will contain only "image/png" and "All files". Maybe it would be better to also have "image/jpeg" in the list. Then even if it doesn't filter anything, users can see what kind of files are expected.

Note: Chrome simply ignored unknown mime type last time I tried, and don't default to "All files" filter. So maybe it's OK to just ignore unknown mime type.
(In reply to Arnaud Bienner from comment #55)
> (In reply to Olli Pettay [:smaug] from comment #53)
> > That sounds super weird to me. In the filepicker UI you have a "filter"
> > enabled, but 
> > actually that filter doesn't do anything.
> > Why not just ignore the filter if Gecko nor OS have mapping to the
> > mimetype.
> 
> That would be OK if there is only one filter.
> But author can provide multiple mime type in the accept attribute. So if one
> of them is unknown, IMO we should not simply ignore it, because we might
> unnecessarily restrict user input.
> In that case we can indeed ignore it, but we should default to "All files"
> filter I think.
> Also, if for example we have accept="image/jpeg,image/png" but "image/jpeg"
> is unknown, the filter list will contain only "image/png" and "All files".
> Maybe it would be better to also have "image/jpeg" in the list.
But if that doesn't filter anything, it is rather odd UI.

> Then even if
> it doesn't filter anything, users can see what kind of files are expected.
That depends on the file types, and whether the user is tech-savvy enough.
Some mimetypes aren't too clear.


> Note: Chrome simply ignored unknown mime type last time I tried, and don't
> default to "All files" filter. So maybe it's OK to just ignore unknown mime
> type.
Hmm, ignoring unknown mime type sounds bad.
Does Chrome ignore unknown mime types if there is some mime type in the list which is known?
And if all the mime types are unknown, nothing is filtered out?

It would be safer, I think, to just not filter anything if there is some unknown mime type.
(In reply to Olli Pettay [:smaug] from comment #56)
> Hmm, ignoring unknown mime type sounds bad.
> Does Chrome ignore unknown mime types if there is some mime type in the list
> which is known?
Yes.

> And if all the mime types are unknown, nothing is filtered out?
Indeed.

> It would be safer, I think, to just not filter anything if there is some
> unknown mime type.
You mean:
- creating a filter anyway, but not selecting it by default?
- or also ignored others valid mime types i.e. having no filter at all?
The latter sounds a bit harsh to me.
(In reply to Olli Pettay [:smaug] from comment #56)
> Hmm, ignoring unknown mime type sounds bad.

Thinking again about this, I think that might be acceptable actually if we implement bug 826176 (blocked by this bug, but easy to implement, and which I will probably implement right after getting this bug fixed).
The spec says accept attribute can also accept file extension (this is what bug 826176 is about), and that "authors are encouraged to specify both any MIME types and any corresponding extensions when looking for data in a specific format" and this is exactly to solve the problem we have here IMO: if authors use non-common mimetype, they should also specify file type extension, precisely in case the browser would not be able to resolve this mimetype.
So we might just ignore unknown mimetype and select the filter we constructed anyway, assuming authors will have specify file extension for that non-common mimetype anyway.

This is probably why it is implemented like this in Chrome.

What do you think?

If that sounds like a good idea for you, I think we should then land both bugs' patches together (or at least to target the same Firefox release).
Flags: needinfo?(bugs)
bug 826176 doesn't help with unknown mime types in general. It helps when there is both mime type and
file extension defined, but if the latter isn't there, it just doesn't affect to the issue at all.
(Getting bug 826176 sounds good, but that is rather separate bug to this one.)
I guess in case there are some file extensions in the attribute value, then not caring about unknown mime types is fine.
Flags: needinfo?(bugs)
Attached patch bug826185.patch (obsolete) — — Splinter Review 
I've implemented the behavior we discussed: filter constructed from accept attribute will be selected as the default one, unless some unknown/invalid mime type are found.
We decide which filter to select by default using this new |allFilterAreValid| variable, so we don't need anymore the |mIsTrusted|.

I've updated the tests as well: hopefully that will help you to understand more quickly the impact of this change.
As expected, mime types filters are now selected by default (filter position == 1) but invalid/unknown values in accept results in filter position == 0, which wasn't the case before.
Attachment #8391248 - Attachment is obsolete: true
Attachment #8511672 - Flags: review?(bugs)
Comment on attachment 8511672 [details] [diff] [review]
bug826185.patch


> 
>+  bool allFilterAreValid = true;
Shouldn't the name be allFiltersAreValid,
(I first thought this was about the * filter)


Looks good. This needs obviously some real world testing too to make sure the behavior is good.
Attachment #8511672 - Flags: review?(bugs) → review+
Attached patch bug826185-2.patch (obsolete) — — Splinter Review 

        Attachment #8511672 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #61)
> Shouldn't the name be allFiltersAreValid,
> (I first thought this was about the * filter)

ah, indeed, thanks. Fixed.

> Looks good. This needs obviously some real world testing too to make sure
> the behavior is good.

You mean waiting for feedback, during the aurora/beta phases, once the patch will be landed, right? (not sure what we can test more for now).

    
    

    
  
Yeah, it if lands now, there is quite some time before it will be in beta.
(and if there are issues, better to backout before it ends up to a beta build.)

    
    

    
  


  
Update patch with commit message.
Marking it as r+ as per comment 61.

        Attachment #8513715 -
        Attachment is obsolete: true

        Attachment #8515450 -
        Flags: review+

    
    

    
  
I wanted to push this to try, just to be sure, but I can't do it myself atm because of bug 1089418.
Tests forms tests were OK for me locally, and I don't see what this change can break else, and why it shouldn't work on other platforms, so I believe it's OK to ask for check-in.
But if someone with access to try server thinks it's necessary to push this patch to try first, I would be glad if he/she can do it.
Keywords: leave-open → checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/23c0b0d9d09a
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1095628

    
    

    
  
Arnaud, I'm trying to update Fx 36 for developers with what this change is doing, but I can't understand by the comments here (there were tentative solutions, so it is complex to understand what has been chosen).

Can you summarize it for me? It would be nice.

Also do you think it is worth a regular release note?
Flags: needinfo?(arnaud.bienner)

    
    

    
  
(In reply to Jean-Yves Perrier [:teoli] from comment #70)
> Can you summarize it for me? It would be nice.

Sure.

Previously:
when specifying a mime type filter in accept attribute for <input type="file">, a filter was created but wasn't selected by default in the file picker opened ("All files" filter remained selected by default and users have to change the filter themselves). Only the "special" image/*,video/* and audio/* values would create a filter which will be selected by default.
If we mixed regular mimetypes and image/*,video/*,/audio/* filter, the "All supported filters" (concatenation of all valid filters specified in accept attribute) wasn't selected by default.

Now:
filters specified in the accept attribute will always be selected by default, unless there is a unknown value (unknown mime type, bad formatted value) in the accept attribute.

There are some notes in the code in the function that creates the filters:
http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.h#328
(note: this mentions file extension currently, but file extension support was added later, and will be included only in FF 37)

> Also do you think it is worth a regular release note?

I'm not sure, but I will let you decide.
I don't think we mentioned the behavior in too much details in the release notes when bug 565274 was implemented (adding mimetype support for accept attribute) so I would be inclined to say it's not worth it.

However, bug 826176 (file extension support) would be worth a release note IMO (but that's another story, as this will be part of FF 37).

Hope it's clearer now. If not, don't hesitate to ask me to clarify.
Flags: needinfo?(arnaud.bienner)

    
    

    
  
Cool, thx: https://developer.mozilla.org/en-US/Firefox/Releases/36#HTML
(I agree with you wrt the release notes.)
Keywords: dev-doc-needed → dev-doc-complete
QA Whiteboard: [good first verify]

    
    

    
  
I think bug is not fixed because if web application need accept several types mime, expected that by default browser would use in dialog "All supported types" but instead we see in dialog "All files"  

Demonstration is here: http://jsfiddle.net/4JSsJ/50/

    
    

    
  
Your demo works fine for firefox 39. I think the bug is fixed.

    
    

    
  
(In reply to lembas from comment #74)
> Your demo works fine for firefox 39. I think the bug is fixed.

May be regression, please check on nightly 42.0a1

    
    

    
  
(In reply to Mikhail from comment #73)
> I think bug is not fixed because if web application need accept several
> types mime, expected that by default browser would use in dialog "All
> supported types" but instead we see in dialog "All files"  

Your demo doesn't "work" for me, but it's normal:

Please have a look at this comment to understand the logic about how this attribute is working:
http://lxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.h#329
The source code isn't ideal place to describe how this is working; maybe having a blog post or something describing how this works will make it easier to find for web developers.

So, in my case, Firefox doesn't know about "image/pjpeg". Thus, it doesn't know what should the file extensions be for this mimetype. In this case, we prefer to not select "All Supported Filters" as the default filter because that means Firefox will filter out some files (the image/pjpeg files it doesn't know about).

As Firefox uses (among other mechanism) the operating system mime type register to find mimetype <-> file extensions mapping, it explains why it's not working for but might work for someone else.

    
    

    
  
Thanks, I understood that the Web application is wrong to rely on the mime type, better always use the correct extension.
Below is variant for solving the problem by using explicitly specify extension.


http://jsfiddle.net/4JSsJ/51/

    
    

    
  
(In reply to Mikhail from comment #77)

My recommendation would be to not use uncommon mimetypes, since they are likely to cause this issue.
This is true for Firefox, but also for also browsers, which are probably unlikely to resolve those uncommon mimetypes as well.

    
    

    
  
(In reply to Arnaud Bienner from comment #78)
> (In reply to Mikhail from comment #77)
> 
> My recommendation would be to not use uncommon mimetypes, since they are
> likely to cause this issue.
> This is true for Firefox, but also for also browsers, which are probably
> unlikely to resolve those uncommon mimetypes as well.

You do not recommend directly specify extension without mime type? 
I'm afraid I have no choice because need allow specific files some images and office documents.

    
    

    
  
(In reply to Mikhail from comment #79)
> You do not recommend directly specify extension without mime type? 

This is not what I meant (sorry if I confused you): I just said uncommon mime types might not work correctly, so better not use them.
But if you're in doubt, yes, also specifying file extension is the way to go.
And this is way BTW what the standard encourage you to do [1]: "Authors are encouraged to specify both any MIME types and any corresponding extensions when looking for data in a specific format."

In Firefox, duplicate filters will be removed so it doesn't harm to have accept="image/jpeg,.jpeg": this will create only one filter since "image/jpeg" already map to .jpeg.

If you specified a mime type Firefox doesn't know anything about (e.g. image/pjpeg in your example) but also a file extension (.pjpeg) Firefox will assume you have specified a "fallback" file extension for the unknown mime type, so "All Supported Types" filter will be selected by default.
Otherwise, as I said previously, "All files" will be selected by default to avoid the filtering being too restrictive.

[1]: https://html.spec.whatwg.org/multipage/forms.html#attr-input-accept

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: