Closed Bug 826176 Opened 11 years ago Closed 10 years ago

File extension support for <input accept=...>

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla37

People

(Reporter: keremonal, Assigned: arnaud.bienner)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 7 obsolete files)

bug826176-1stpart.patch
7.83 KB, patch
arnaud.bienner
: review+
Details | Diff | Splinter Review
bug826176-2ndpart.patch
9.23 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:

This is a request for implementation of "file extension filter" mentioned in http://www.w3.org/html/wg/drafts/html/master/forms.html#attr-input-accept
Was this not done in bug 565274?
Sorry, I just saw bug 773985 comment 4:
>The main idea is to select a filter by default only if it comes from Mozilla's 
>hard-coded set of values (something we can handle).
>Currently, as the mime service resolver also uses OS information, this might 
>result in different mime-types<->file extensions mapping across OSs so it was 
>decided it's more desirable to not select this filter as default, as we can't 
>"trust" it.
>Moreover, from what I see in the W3C link you provided, this is compliant with 
>the specification, as the spec only specifies that input should support 
>mime-types in accept attribute when type="file", which is what we do currently.
>But maybe we can discuss the current behavior if you think something can be improved.
Carry on.
I'm adding Jonas and Mounir to the CC list, as they were part of the discussion in bug 565274.
In comment bug 565274, comment 36, Jonas stated this might not stayed in the specification.
So, should we implement this? Wait a bit more, in case the specification will change?

And, if we decide to implement this, how?

IMHO it's worth to implement this, as it's complementary to mimetypes: some people will want to filter based on extensions, other on mimetypes, so I believe Firefox should offer both possibilities.

Few remarks:

Chrome seems to be the only browser to implement this.
It respects actual file extensions (i.e. |accept=".jpg"| will not be converted to |accept=".jpg,.jpeg"|) but it maps the extension back to the corresponding mime type, so the |accept=".jpg"| is displayed as "JPEG Image".
This seems good, as it is more meaningful for user, but, on the other hand, |accept=".doc"| will lead to a "Microsoft Word" filter, even if ".doc" might be something else actually (e.g. a plain text file named like this because of some particular convention). In that case the filter name will be misleading.

Btw, this reverse mapping can be done easily in FireFox I believe, as the mime type service allows it, if I remember well.

Also, webmasters will be probably uses something like |accept="application/jpeg,.jpg,.jpeg"| to be sure it will work, so we should also think about handling duplicated filters in a nice way.
Yeah, I don't mind adding support for extensions.

I'm not really worried about duped entries in the list given the short list of mimetypes that we support.

But I think there is a question of if we do a separate filter for each extension, or if we do one filter that holds all extensions.
> I'm not really worried about duped entries in the list given the short list
> of mimetypes that we support.
I'm not sure to understand you: as we also use OS mimetypes mapping, our actual mimetypes list isn't so short.

> But I think there is a question of if we do a separate filter for each
> extension, or if we do one filter that holds all extensions.
IMHO we should keep a similar behavior to what we currently have for mimetypes: one filter by extension, and a "All filters" at top, which will be the concatenation of all valid filters (extension filters + mimetypes filters).

The next questions are:
- can we do something clever? e.g. find out that |.jpg,.jpeg| actually correspond to the same mimetype, so we group them together in a single filter. Also, if we have |image/jpeg,.jpg,.jpeg| in the accept attribute, have only one filter.
Having multiples filters for similar things might add confusion to users IMO.

- should we consider extension filter to be "trusted" i.e. should we make them selected by default, as we currently do only for |video/*|, |image/*| and |audio/*|?
A web developer may want to accept *.jpg and not accept *.jpeg
So he/she codes it as <input accept='.jpg'>. Please let's stay as simple as possible.

> IMHO we should keep a similar behavior to what we currently have for
> mimetypes: one filter by extension, and a "All filters" at top, which will
> be the concatenation of all valid filters (extension filters + mimetypes
> filters).

"all valid filters" is NOT the all valid filters of Firefox's database or *.*
Am I right, Arnaud? 

To be clear, <input accept='.png,.pdf,.txt'> means there will be 4 filters. First one will be all 3 of them (all png's, pdf's and txt's will be listed at the same time) and this filter is the default one. And then 3 separate filters for *.png, *.pdf and *.txt.
(In reply to lembas from comment #6)
> "all valid filters" is NOT the all valid filters of Firefox's database or *.*
> Am I right, Arnaud? 
Something like |fake/mimetype| isn't valid, because we will not find any extension list associated with it; |image/jpeg| is valid, even if it's not "trusted", so it will not be selected as default.

> To be clear, <input accept='.png,.pdf,.txt'> means there will be 4 filters.
> First one will be all 3 of them (all png's, pdf's and txt's will be listed
> at the same time) and this filter is the default one. And then 3 separate
> filters for *.png, *.pdf and *.txt.
Yes,  that was my idea.
Actually, a "All File (*.*)" filter should also always remains available.
> Actually, a "All File (*.*)" filter should also always remains available.
But as a 5th option. Not the default one. IMHO, this approach is even better than Chrome's.

This bug is about file extension support. Please open a separate bug to discuss about mimetypes.
(In reply to lembas from comment #8)
> But as a 5th option. Not the default one. IMHO, this approach is even better
> than Chrome's.
Currently "All files" filter is always the first filter, and I see no reason to change this. But we should discuss which filter should be selected by default (not necessary the "All files" one). 

> This bug is about file extension support. Please open a separate bug to
> discuss about mimetypes.
??
I'm not talking about mimetypes: mimetypes' based filter is already implemented. I'm explaining you what I meant by "valid filter".
A web developer uses "accept" attribute for an ordinary user to see only the filtered results. 

And also that developer will probably re-check it on the client-side even if the user selects an unacceptable file type and shows a warning message; for example a "please select pdf files" message if the user selects a png file. 

And again that developer checks it for the third time on the server-side, which expects a pdf file, and sends back an error code to the client-side if the user somehow selects an unacceptable file type.

As a result, the default filter should be the acceptable file types which is determined by the developer. Chrome's default filter is the right one. We should prevent user to accidentally selects an unacceptable file type with making "All files" as the default filter. Firefox already does the right thing when the developer uses image/* or video/* or audio/* by selecting them as the default filter.
Severity: normal → enhancement
OS: Windows 7 → All
Are there any plans of implementing it soon?
(In reply to lembas from comment #11)
> Are there any plans of implementing it soon?
We should first agree on how to implement this.
Mounir: If you have opinions here I'm happy to go with whatever you think.
Waiting for Mounir's feedback, here is my idea about how to implement this:

I think we should add one new filter for each extension we encounter while processing |accept| attribute (and ignore duplicated filter if there are).
This is the simplest way to go IMO.

But, at the end, we can an extra step to merge similar filters.
For instance, if there is |image/jpeg| specified (which is very likely to be mapped to |.jpeg,jpg| file extensions) and then |.jpg| or |.jpeg| extension (or both). We check if |.jpg| extension is already present in one of the mimetype filters we just created and, if so, remove the filter for this extension.
If not, we create a filter for that extension.

We can name this filter by getting a mimetype from the extension (I quite sure mime service can do this), and also add the extension in the filter title.
Or, more simply, maybe just name the filter with the extension?

One remaining question: should we consider extension filters as "trusted" i.e. selected by default?
Having these filters be trusted sounds ok to me. Though for something like accept=".jpeg, .doc" we couldn't default to either just ".jpeg" or ".doc" files. The default would have to be both.
(In reply to Jonas Sicking (:sicking) from comment #15)
> Having these filters be trusted sounds ok to me. Though for something like
> accept=".jpeg, .doc" we couldn't default to either just ".jpeg" or ".doc"
> files. The default would have to be both.
Indeed: I think we should keep the current behavior: an additional filter which is the concatenation of all filters.
Attached patch Patch: first draft (obsolete) — — Splinter Review 
A simple patch will be something like this: it creates a new filter for each extension seen, whose name is "*.extension". Not "trusted" so not selected by default (for the sake of simplicity in this first draft).

However, I think we can improve this, at least by merging similar filters, because I think webmaster are likely to specify both mime-type and extension filter, and the spec suggest so [1] (and this seems reasonable).
But I see one drawback: how to deal with both trusted and untrusted filters which represents the same thing? I think the "trusted/untrusted" behavior is really over-complicating things and will not help us if we want to do something smart here.
I know this has already been discussed in bug 826185 and bug 565274, but I think we should get rid of the "trusted" logic: after all, we already have few bugs opened to request to change this, and none to complain about mimetype filtering working poorly because of a bad mapping, which was the reason (if I'm right) why we decided to not trust mimetype filters in bug 565274.

Sorry for the long speech, which was a little bit out of scope at the end...

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html#file-upload-state-%28type=file%29
If we have to implement this, I think we should do what Arnaud described in comment 14 except that I believe we should show the file extensions instead of the mime type names. I think the mime type name would be misleading in too much cases.

This said, I'm not really convinced that file extensions are a great idea. It's implementable but what would be the pros compared to mime types? I understand that we can't really "trust" mime types because the users might have them broken but I've never heard how important that was. Are we speaking of a few users or a big chunk?
So, I wonder if we shouldn't try to improve our mime type support in @accept before considering adding the file extension support. I'm afraid that given our crappy (by design) mime type support, developers will fallback en masse to using file extensions.
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Hardware: x86_64 → All
(In reply to Mounir Lamouri (:mounir) from comment #18)
> This said, I'm not really convinced that file extensions are a great idea.
> It's implementable but what would be the pros compared to mime types?

A web application we're building has a lot of data import and export, 90% of which is data types specific to this very application. As long as OS doesn't learn mime types when a file is being downloaded, we can't expect user's OS to know those mime types when they upload the file. Extensions work perfectly.

Apologies if this comment is an unhelpful noise.
(In reply to Mounir Lamouri (:mounir) from comment #18)
> I think the mime type name would be misleading in
> too much cases.
Yep, probably. That was just an idea, but indeed it doesn't sound so great.

> This said, I'm not really convinced that file extensions are a great idea.
> It's implementable but what would be the pros compared to mime types?
IMHO we should have both: in some cases it's more convenient to have mimetypes (i.e. user doesn't have to keep a full mapping by himself) and in some other cases, extensions are better (uncommon mimetype or application's specific data which have no mimetype, as stated by Radek).

> I understand that we can't really "trust" mime types because the users might
> have them broken but I've never heard how important that was. Are we
> speaking of a few users or a big chunk?
IMO few users for specific, uncommon mime types: in that case, we will not found any file extensions associated to this mime type and default to *.* but I think it's fine.
The only problem would be if the mime type are "corrupted" on the OS: we will use them anyway. But I'm not sure this will really happen and that we should consider this case anyway.

> So, I wonder if we shouldn't try to improve our mime type support in @accept
> before considering adding the file extension support. I'm afraid that given
> our crappy (by design) mime type support, developers will fallback en masse
> to using file extensions.
Indeed: if we "trust" extension filters (and it makes sense to do so) developers will probably prefer extensions over mimetypes (even if mimetypes will be more appropriate in their case) because extension filters will be selected by default.
any progress?
Depends on: 826185, 565274
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 (see comment 22).
Flags: needinfo?(jonas)
I just realized my comment 22 should belong to bug 826185, not this one :(
Clearing the needinfo flag as I moved my request to bug 826185.
Flags: needinfo?(jonas)
(In reply to Radek 'sysKin' Czyz from comment #19)
> (In reply to Mounir Lamouri (:mounir) from comment #18)
> > This said, I'm not really convinced that file extensions are a great idea.
> > It's implementable but what would be the pros compared to mime types?
> 
> A web application we're building has a lot of data import and export, 90% of
> which is data types specific to this very application. As long as OS doesn't
> learn mime types when a file is being downloaded, we can't expect user's OS
> to know those mime types when they upload the file. Extensions work
> perfectly.

I agree with Radek's comment. We are developing a very specific web application in the geotechnical field (to replace some existing desktop applications). There's a lot of file import from our electronic recorders with various extensions (.dvl in our case for example, but there are many others). As OSes could not have MIME types for every specific extensions, we have currently no mean in Firefox to filter on these extensions for our customers (it is working fine with Chrome/Chromium but we try to keep our app features on par with Firefox).

I am sure that filtering on MIME types first should be used as much as possible but it should be the responsability of the developer to be able to filter with file extensions when needed.

Thanks for your work.
any progress?
These docs suggest that FF already supports file extensions in the accept attribute.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input

If you look at this test case:
http://jsfiddle.net/sw2tgd36/3/

I have copy-pasted the accept filter straight from Mozilla's docs... Ironically enough this works in Internet Explorer 11 (and also 10 I think) as well as Chrome 38 (but probably also lots of earlier versions), but not in Firefox.

I can CONFIRM this bug.
(In reply to Stijn de Witt from comment #28)
> These docs suggest that FF already supports file extensions in the accept
> attribute.
> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input
> 
> If you look at this test case:
> http://jsfiddle.net/sw2tgd36/3/
> 
> I have copy-pasted the accept filter straight from Mozilla's docs...
> Ironically enough this works in Internet Explorer 11 (and also 10 I think)
> as well as Chrome 38 (but probably also lots of earlier versions), but not
> in Firefox.
> 
> I can CONFIRM this bug.

I Second the motion. Also don't work with CSV files. Really need the filter to work for the site we are developing. 

When can this be fixed? :/
Now bug 826185 is fixed, I think we can advance on this bug.

IMHO there is several ways to implement this.

1: The simplest one is to go with my attachment 720481 [details] [diff] [review]: simple create a filter with the extension. Naming it like the extension it filters sounds like the most natural option to me.

But we can improve things:

2: According to the spec:
"Authors are encouraged to specify both any MIME types and any corresponding extensions when looking for data in a specific format."
which makes sense as some mime type might not be supported by the user's platform.
But if author carefully specify both, we will end up showing too many filters.
e.g.
accept="image/jpeg,.jpg,.jpeg"
we will end up with 3 filters, while it's very likely that "image/jpeg" will already be resolved to ".jpg,jpeg".
So it might nice to have an extra step at the end to remove "useless" filters i.e. those which filter files that are already filtered by other filters.
foreach (filterTocheck, filters):
  foreach (filter, filters):
    if (filterToCheck != filter &&
        filterToCheck.extensionsList is a subtring of filter)
        // We already have a similar, less restrictive filter: get rid of this one
        filters.remove(filterToCheck)

I know it's not perfect code (we should iterate over copies of filters list, etc.) but you got the point, this is just pseudo code.

Note that this will also benefit to mimetypes filter. For example with this code,
accept="image/*,image/jpeg"
will lead to 1 filter (image/jpeg will be removed) instead of 2 currently (the second one being useless IMHO).

3. One last thing: as discussed in bug 826185, as per the spec, if both mime types and file extension are specified, we should not worry if we find an unknown mime type (i.e. no need to default to "All files (*.*)" filter in this case, as we currently do).
Olli, as you worked with me on bug 826185, I would like to have your opinion about my proposal in comment 30.
And if you have any other ideas about how to implement this, please let me know :)
Flags: needinfo?(bugs)
2. and 3. sound pretty good to me.
Flags: needinfo?(bugs)
Attached patch bug826176-1stpart.patch (obsolete) — — Splinter Review 
1 and 3 implemented (+ tests)
Please let me know what you think about this.

Will do 2 as a separate patch.
Attachment #8530496 - Flags: review?(bugs)
Attached patch bug826176-2nd.patch (obsolete) — — Splinter Review 
The "remove similar filters" enhancement we discussed about.

About this patch, few things I'm not sure about:
1) here we simply check if one extensionList string is a substring of another filter's extensionList. In practice, this will work for us, as the filters that might be similar would be file extension filter (e.g. ".jpg" in accept attribute) which have only one extension. But, in theory, this would not work if two filters are similar but extension aren't ordered the same way. I'm not sure we might face this case in real life, but if so it would be better to change the nsFilePickerFilter struct to store a real list of extension, and create the string only at the end, when actually adding filters to the file picker.
Do you think it's worth it? In a first version of the patch? Or maybe in another "clean up things" patch?

2) for the three special mime types image/*, video/*, audio/* we store only an int (called "mask"), not the full title/extension list. This would be super easy to add this info to nsFilePickerFilter then also checking those mimetypes when removing similar filters. If we do so, we might be more likely to face problem mentioned in 1) though.
For example: accept="image/*,image/jpeg,image/png" will create 3 filters, while we should probably ignore image/jpeg and image/png and they are already part of image/*.

One last thing, about the test: I wanted a test with one mime type filter mapping to several file extensions, + few file extensions filters, so I can test my code was called and worked correctly.
However, I couldn't find a mimetype mapping exactly to the same list of extension on all platforms (see [1]).
So I believe we can simply ignore the extension list check in this case.
An alternative would be to change the extension list to check, according to  the platform, as Mounir did in bug 565274 comment 78 (at the end, we decided to keep only simple mime types for tests, which we were sure will always map to a small set of extensions).

[1]: https://tbpl.mozilla.org/?tree=Try&rev=53033416aa55
Assignee: nobody → arnaud.bienner
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8530616 - Flags: review?(bugs)
Attachment #8530496 - Flags: review?(bugs) → review+
(In reply to Arnaud Bienner from comment #34)
> About this patch, few things I'm not sure about:
> 1) here we simply check if one extensionList string is a substring of
> another filter's extensionList. In practice, this will work for us, as the
> filters that might be similar would be file extension filter (e.g. ".jpg" in
> accept attribute) which have only one extension.
Does it work with .jpeg and other variants too? 

> But, in theory, this would
> not work if two filters are similar but extension aren't ordered the same
> way.
Not sure I understand this.


> 2) for the three special mime types image/*, video/*, audio/* we store only
> an int (called "mask"), not the full title/extension list. This would be
> super easy to add this info to nsFilePickerFilter then also checking those
> mimetypes when removing similar filters. If we do so, we might be more
> likely to face problem mentioned in 1) though.
> For example: accept="image/*,image/jpeg,image/png" will create 3 filters,
> while we should probably ignore image/jpeg and image/png and they are
> already part of image/*..
Not sure this is too bad issue.
Comment on attachment 8530616 [details] [diff] [review]
bug826176-2nd.patch

>+  // Remove similar filters
>+  // Iterate over a copy, as we might modify the original filters list
>+  nsTArray<nsFilePickerFilter> filtersCopy;
>+  filtersCopy = filters;
>+  for (uint32_t i = 0; i < filtersCopy.Length(); ++i) {
>+    const nsFilePickerFilter& filterToCheck = filtersCopy[i];
>+    if (filterToCheck.mFilterMask)
>+      continue;
always {} with 'if'

>+    for (uint32_t j = 0; j < filtersCopy.Length(); ++j) {
>+      if (i == j)
>+        continue;
>+      if (FindInReadable(filterToCheck.mFilter, filtersCopy[j].mFilter)) {
>+        // We already have a similar, less restrictive filter: remove this one
This removes duplicates, sure, but I don't understand "similar, less restrictive"

>+                ["q", 1, "*.gif", 1],
>+                ["r", 1, undefined, 1], // NOTE: this test case is handled in a particular way
Please explain how, don't just say "handled in a particular way". So, change the comment to explain the test is
handled specially elsewhere in the test file.

Could we possibly have platform specific checks for 'r'.

And why does the tryserver run give
'Correct filters should have been added (r) - got *.jpg, expected *.jpeg; *.jpg; *.jpe'? Doesn't that hint that something goes wrong.
Attachment #8530616 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #36)
> This removes duplicates, sure, but I don't understand "similar, less
> restrictive"

Indeed, this will remove duplicate, but not only. For example:
accept="image/jpeg,.jpeg,.jpg"
image/jpeg will maps to .jpeg,.jpg,.jpe extensions on Linux.
So we will have 3 filters:
1) "*.jpeg; *.jpg; *.jpe" (mime type filter)
2) "*.jpeg" (first extension filter)
3) "*.jpg" (second extension filter)

"*.jpeg" and "*.jpg" are substrings of the first filter, thus first filter is "similar, but less restrictive" i.e. it already contains the same list of extension + other extensions. So we don't need filters 2) and 3), as there extension lists are a subset of filter 1) extension list.

I've added a note about this in the .h file, but I can also add a similar comment here, if you think it's not clear enough.

> Could we possibly have platform specific checks for 'r'.

Sure.

> And why does the tryserver run give
> 'Correct filters should have been added (r) - got *.jpg, expected *.jpeg;
> *.jpg; *.jpe'? Doesn't that hint that something goes wrong.

That was a try run I did before adding the special check for "r" test case.
The list of extensions "image/jpeg" mimetype maps to is slightly different from one platform to another (AFAIK "*.jpeg; *.jpg; *.jpe" on Linux, and ".*jpeg" everywhere else).
But as previously said, we can simply handle "r" test case differently depending on the platform (something like attachment 630472 [details] [diff] [review]).
(In reply to Olli Pettay [:smaug] from comment #35)
> > But, in theory, this would
> > not work if two filters are similar but extension aren't ordered the same
> > way.
> Not sure I understand this.

This is theoretical, and will not happen currently.
But if we had two filters with the following extensions list:
"*.jpeg; *.jpg;"
"*.jpg; *.jpeg"

They will look different, while they actually filter the same things. So checking the if one is substring of the other will not work.
That's what I meant.

In practice, we will only want to remove filters with only one extension (those we can now create thanks to my previous patch attachment 8530496 [details] [diff] [review]), so this is not an issue.
We might face the problem when mixing image/*,... and other mimetypes, but I you said in comment 35, we probably don't need to worry about this usecase.
Commit message updated.
Attachment #8530496 - Attachment is obsolete: true
Attachment #8531310 - Flags: review+
(In reply to Olli Pettay [:smaug] from comment #36) 
> Could we possibly have platform specific checks for 'r'.

Actually, this is more tricky than I expected...
image/jpeg maps to a slightly different list of file extensions depending on the platform, but it might be different across Windows versions, so a check like the one in attachment 630472 [details] [diff] [review] isn't enough.
I tried other mimetypes, but they are not so many common mimetypes mapping to more than one file extension (which I need for my test).
The most "stable" one I found is "video/mpeg", which maps to "*.mpeg; *.mpg; *.mpe" on Linux, nothing on Android, and "*.mpeg" everywhere else.
But, once again, it wasn't able to write a check specific enough, as it seems that Android is considered like a Linux (i.e. navigator.platform also returns Linux for Android).

Also, even if I manage to write a test that works for now, I'm worried it might be easily broken for new platforms or new version of operating systems.

Anyway, my goal here was to test that if we have a mimetype mapping to several file extensions, adding a file extension filter corresponding to one of the extension this mimetype is mapping to will not work (i.e. the second filter will be correctly considered as a duplicate and removed).

So, would you be OK to handle "r" test case differently? i.e. not check extension list == expected value for this test? Checking if the expected extension is a substring of the retrieved extension list sounds like a good compromise, the important thing in this test being that "number of filter" value is the expected one.
Attached patch bug826176-2nd-2.patch (obsolete) — — Splinter Review 
To illustrate what I meant in comment 40.
I've also updated the patch regarding comments you made on the previous version.
Attachment #8530616 - Attachment is obsolete: true
Attachment #8532845 - Flags: review?(bugs)
Attached patch bug826176-2nd-3.patch (obsolete) — — Splinter Review 
Just notice I missed another "if without {}" issue.
Attachment #8532845 - Attachment is obsolete: true
Attachment #8532845 - Flags: review?(bugs)
Attachment #8532847 - Flags: review?(bugs)
(In reply to Arnaud Bienner from comment #40)

> Also, even if I manage to write a test that works for now, I'm worried it
> might be easily broken for new platforms or new version of operating systems.
That is not really a reason to not test something. If new platforms are added, all the
platform specific tests may need some tweaking.


> Anyway, my goal here was to test that if we have a mimetype mapping to
> several file extensions, adding a file extension filter corresponding to one
> of the extension this mimetype is mapping to will not work (i.e. the second
> filter will be correctly considered as a duplicate and removed).
right


> So, would you be OK to handle "r" test case differently? i.e. not check
> extension list == expected value for this test? Checking if the expected
> extension is a substring of the retrieved extension list sounds like a good
> compromise, the important thing in this test being that "number of filter"
> value is the expected one.
Sounds good to me.
Comment on attachment 8532847 [details] [diff] [review]
bug826176-2nd-3.patch

>+      if (testData[currentTest][0] == "r") {
>+        ok(filters[0].contains(testData[currentTest][2]),
>+           "Correct filters should have been added (" + testName + ")");
Couldn't you still check here that filters[0] contains also something else than just
testData[currentTest][2].
I assume all the platforms map image/jpeg to also something else than just .jpeg

with that, r+
Attachment #8532847 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #44)
> I assume all the platforms map image/jpeg to also something else than just
> .jpeg

Actually no: seems that Android maps image/jpeg to "*.jpeg" only.
We might add a check for all platforms but Android, but that sounds a bit overkill to me and, as I said previously navigator.platform might return "Android" or "Linux", making the check a bit unreliable.
But we can add a check when (platform != "Android" && platform != "Linux") if you think it's worth it anyway.

What do you think is best?
Flags: needinfo?(bugs)
I think the 'r' test shouldn't only test that .jpeg does something, but also that image/jpeg works in that case. So if Android uses only *.jpeg, sounds like we need to special case it and allow
ok(filters[0].contains(testData[currentTest][2])) there.

So maybe
ok(filters[0].contains(testData[currentTest][2]), "..")
if (platform.indexOf("Android") < 0) {
  ... test that filters[0] contains also something else.
}
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #46)
Of course, but the problem here is that I'm not sure how to know if the platform is Android or not i.e. I'm not sure the platform.indexOf("Android") check will always work on Android. According to [1], this might return "Linux" on an Android device.

But I'm fine to do the change anyway and push to try to see if it works.

[1]: http://stackoverflow.com/questions/19877924/what-is-the-list-of-possible-values-for-navigator-platform-as-of-today
Attached patch bug826176-4.patch (obsolete) — — Splinter Review 
I've updated the patch the way we discussed on IRC.
Instead of testing an exact list of extensions (as it might change from one platform to another, including from one version of an OS to another, making the check tricky to do) we test that adding a file extension filter (".jpg" here) in accept attribute when a mime type is already present ("image/jpeg" here) doesn't restrict the extension list, but only extend it if needed.
I've added an extra "mix-ref" reference element for that purpose, with only "image/jpeg" in accept attribute.

Maybe you should have a quick look, as the test is now different from what you already r+?  (the other parts didn't changed)

FYI I pushed to try and everything looks OK: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71ec6fc8a601
Attachment #8532847 - Attachment is obsolete: true
Attachment #8537730 - Flags: review?(bugs)
Attachment #8537730 - Flags: review?(bugs) → review+
Attached patch bug826176-4.patch (obsolete) — — Splinter Review 
Add proper title
Attachment #8537730 - Attachment is obsolete: true
Attachment #8537988 - Flags: review+
Two patches to be landed here: attachment 8531310 [details] [diff] [review] and attachment 8537988 [details] [diff] [review].
Keywords: checkin-needed
Attachment #720481 - Attachment is obsolete: true
"Go ahead and land them, even though the Android builds were busted," he said. "I'm sure it'll be fine," he said...

Backed out for Android test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c118faced18

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4752794&repo=mozilla-inbound
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #52)
> Backed out for Android test failures.

:(

Probably because "image/jpeg" doesn't map to ".jpg" on Android but to something else, we end up having many filters, unlike other platforms.
But actually that's not a big deal: what "mix" test should check is that the list isn't too restrictive. Testing the number of filters isn't relevant here. So I will just move the failing test under the non-"mix" related part.
Updated patch.
Attachment #8537988 - Attachment is obsolete: true
Attachment #8538815 - Flags: review+
Looks like Android builds are still busted.
I believe you can safely land those patches now, but if you prefer that I wait for Android builds to be back to normal first, just let me know and I will try again to push to try later.
Keywords: checkin-needed
Your bustage points to pushing on top of a really old rev of mozilla-central. How old of a head are you pushing on top of? Any current checkout of m-c will have the right manifests you need. In general, you should always make sure you're pushign to Try off a current m-c.

And no, there's no way I'm landing this until I see green on Try for Android.
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #56)
> Your bustage points to pushing on top of a really old rev of
> mozilla-central. How old of a head are you pushing on top of? Any current
> checkout of m-c will have the right manifests you need. In general, you
> should always make sure you're pushign to Try off a current m-c.

Maybe some time indeed. I didn't notice this was the root cause. Thanks for pointing this out.

> And no, there's no way I'm landing this until I see green on Try for Android.

Sure: now I know why it failed, I will do a proper push to try.
Pushed to try, everything looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1b32d01cd62
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/104ae3137652
https://hg.mozilla.org/mozilla-central/rev/edf2031cc7e5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Iam anxious that file whith extendsion .tif doesnt supported by firefox.
And When upload dialog appear. It focus to "All file" instead of my Extendsion settings.

I hope Mozila can support accept file filter like ".myExt" instead of format "image/png" like Chrome and IE does.
Because many developer want filter by they file extendsion for they app. Ex: ".abc", ".xyz"


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

Attachment

General

Creator:
Created:
Updated:
Size: