Last Comment Bug 377624 - Implement the accept attribute for the form and file upload controls form "image/*"
: Implement the accept attribute for the form and file upload controls form "im...
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- enhancement with 3 votes (vote)
: mozilla1.9.3a5
Assigned To: Mounir Lamouri (:mounir)
:
: Andrew Overholt [:overholt]
Mentors:
http://www.whatwg.org/specs/web-forms...
: 379892 536984 (view as bug list)
Depends on: 379189 643576
Blocks: html5forms 83749 215889 565272 565274 566348
  Show dependency treegraph
 
Reported: 2007-04-16 03:14 PDT by Ryan Jones
Modified: 2014-08-02 09:08 PDT (History)
26 users (show)
mounir: in‑testsuite+
ehsan: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Toolkit & Widget part (v1) (4.92 KB, patch)
2007-05-16 14:33 PDT, Ryan Jones
roc: review+
roc: superreview+
Details | Diff | Splinter Review
Patch v1 (23.54 KB, patch)
2007-05-22 09:13 PDT, Ryan Jones
no flags Details | Diff | Splinter Review
Patch v1 (9.17 KB, patch)
2010-04-26 07:18 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2 (5.19 KB, patch)
2010-04-27 15:54 PDT, Mounir Lamouri (:mounir)
roc: review+
Details | Diff | Splinter Review
Patch v2.1 (5.26 KB, patch)
2010-04-29 12:52 PDT, Mounir Lamouri (:mounir)
enndeakin: review+
Details | Diff | Splinter Review
Litmus test (1.33 KB, text/html)
2010-05-05 08:54 PDT, Mounir Lamouri (:mounir)
no flags Details
Tests v1 (6.60 KB, patch)
2010-05-10 04:05 PDT, Mounir Lamouri (:mounir)
ehsan: feedback-
Details | Diff | Splinter Review
Tests v2 (7.09 KB, patch)
2010-05-10 10:01 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v3 (7.32 KB, patch)
2010-05-10 14:19 PDT, Mounir Lamouri (:mounir)
jonas: review+
ehsan: feedback+
Details | Diff | Splinter Review
Tests v3.1 (7.51 KB, patch)
2010-05-11 03:07 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v3.2 (7.87 KB, patch)
2010-05-11 15:54 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v3.3 (7.44 KB, patch)
2010-05-19 16:52 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description Ryan Jones 2007-04-16 03:14:07 PDT
Implementing the accept attribute for the form and file upload controls from the Web Forms 2.0 specification.

I'm currently working on this so I'll assign it to myself for now.
Comment 1 Tuukka Tolvanen (sp3000) 2007-05-06 14:04:36 PDT
*** Bug 379892 has been marked as a duplicate of this bug. ***
Comment 2 Ryan Jones 2007-05-16 14:33:23 PDT
Created attachment 265043 [details] [diff] [review]
Toolkit & Widget part (v1)

First part of patch v1, dealing with the changes to toolkit and widget.
Comment 3 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-05-21 17:18:17 PDT
Comment on attachment 265043 [details] [diff] [review]
Toolkit & Widget part (v1)

mpa=mano for the locale changes.
Comment 4 Ryan Jones 2007-05-22 09:13:35 PDT
Created attachment 265675 [details] [diff] [review]
Patch v1

This is a first try at a working patch.

I'd like to thank BZ and Alex for their help and advice on this one,l I've also tried added the web-forms idl stuff in relation to the other patches proposed by Alex in order to maintain his naming structure.
Comment 5 Ryan Jones 2007-05-22 16:32:01 PDT
Comment on attachment 265043 [details] [diff] [review]
Toolkit & Widget part (v1)

Asking roc to review the widget changes here.
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2008-09-25 07:21:35 PDT
Jonas, can you do this review?
Comment 7 Ryan Jones 2008-09-25 07:55:59 PDT
Unfortunately if this patch does not pass review I can no longer maintain it as my build trees are gone with my old computer and until I can get a computer that can actually compile and run local builds myself, anyone wishing to take up this patch & bug may do so at their leisure.

Cheers!
Comment 8 Serge Gautherie (:sgautherie) 2008-09-25 08:42:42 PDT
Comment on attachment 265043 [details] [diff] [review]
Toolkit & Widget part (v1)

Tried to apply this patch with '-p0':
{
abort: bad hunk #1 @@ -39,35 +39,37 @@
 (35 35 39 37)
}

Please, attach an updated/Hg patch.
Comment 9 Ryan Jones 2010-03-21 06:58:25 PDT
Anyone looking to take over this bug may do so. I currently have no time to work on this.

Thanks everyone!
Comment 10 Mounir Lamouri (:mounir) 2010-04-20 07:53:36 PDT
I'm taking this bug per previous comment.
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-04-21 16:03:32 PDT
Hrm.. Sorry for not commenting here previously.

My concern with this patch is that I don't think the OS mimetype->extension mapping is going to be good enough. For example if someone does

<input type=file accept="plain/text">

The user would be prevented from attaching a javascript file, although that is arguably a plaintext file.

And worse, what if an author does

<input type=file accept="application/msword">

This might work fine on most peoples computers, but not on a linux machine that doesn't have word installed.

I really have no idea how this feature can be implemented for generic mime types. It seems like most of the time it'll get in the way of the user, rather than help.

If I'm missing something, please let me know.

What I would like to see us implement is support for things like

<input type=file accept="image/*">

Here we could "image/*" hardcoded and do things like bring up platform specific ways of choosing images. For example on Mac we could let you bring up iPhoto, on mobile we could let you browse the camera pictures stored on the phone. We can also do awesome things like let you use any connected or built in cameras to take a picture on the fly.

Similar things can be done for "video/*" and "audio/*".

However that is different from what the current patch does. And from what I think this bug is aiming to do. If anyone wants to work on that I think that would be more than awesome and I'd encourage you to file a separate bug specifically for any mimetype that you want to add support for.
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-04-21 16:04:07 PDT
Comment on attachment 265675 [details] [diff] [review]
Patch v1

Resetting review request per above comment. Feel free to rerequest if you disagree.
Comment 13 Mounir Lamouri (:mounir) 2010-04-22 10:54:38 PDT
So, after our discussion on IRC, we've decided to implement accept="image/*" as a first step then "audio/*", "video/*" and customized mime types. We've decided to filter "image", "audio" and "video" with arbitrary file extensions.

I think we should support this list of image extensions:
*.jpe; *.jpg; *.jpeg; *.gif; *.png; *.bmp; *.ico; *.svg; *.svgz, *.tif; *.tiff

These extensions are supported by Opera in addition of the these ones: "cpr, g3f, pict, rf, rp, wbmp and xbm" but I've never heard about most of them so I'm not sure we need to add them even if that shouldn't heart. Actually, even Gimp does not know them.

I'm wondering if we could not rely on nsIMimeService instead of a list of extensions. Indeed, "image/*" could be all MIME types with the "image" major type. I think this could be more powerful and integrated to the platform (even future-proof). However, this would be seriously harder to add the 'wildcard as a joker' functionality in MIME types management than extending the image extensions.

Jonas, do you think using nsIMIMEService worth being tried ?
Comment 14 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-04-22 15:18:37 PDT
(In reply to comment #13)
> So, after our discussion on IRC, we've decided to implement accept="image/*" as
> a first step then "audio/*", "video/*" and customized mime types. We've decided
> to filter "image", "audio" and "video" with arbitrary file extensions.
> 
> I think we should support this list of image extensions:
> *.jpe; *.jpg; *.jpeg; *.gif; *.png; *.bmp; *.ico; *.svg; *.svgz, *.tif; *.tiff

How did you arrive at this set of types?

> These extensions are supported by Opera in addition of the these ones: "cpr,
> g3f, pict, rf, rp, wbmp and xbm" but I've never heard about most of them so
> I'm not sure we need to add them even if that shouldn't heart.

I'm very wary of basing any lists on what you and I have heard about.

> I'm wondering if we could not rely on nsIMimeService instead of a list of
> extensions. Indeed, "image/*" could be all MIME types with the "image" major
> type. I think this could be more powerful and integrated to the platform (even
> future-proof). However, this would be seriously harder to add the 'wildcard as
> a joker' functionality in MIME types management than extending the image
> extensions.
> 
> Jonas, do you think using nsIMIMEService worth being tried ?

*If* we can ask the OS for all extensions with a mimetype that starts with "image", then I think it would be worth using that to *add* to the above list. I do definitely want to keep a minimum list though.
Comment 15 Mounir Lamouri (:mounir) 2010-04-26 04:41:05 PDT
I've used this link to get the mime types:
http://www.iana.org/assignments/media-types/image/

To know the file extensions, I used my mime file and looked to the documents when my mime file wasn't mentioning an extension for a mime type. Some mime types have no extension.

I've also added this from my mime file:
image/x-cmu-raster				ras
image/x-cmx				cmx
image/x-coreldraw				cdr
image/x-coreldrawpattern				pat
image/x-coreldrawtemplate				cdt
image/x-corelphotopaint				cpt
image/x-freehand				fh fh4 fh5 fh7 fhc
image/x-icon				ico
image/x-jg				art
image/x-jng				jng
image/x-ms-bmp				bmp
image/x-pcx				pcx
image/x-photoshop				psd
image/x-pict				pct pic
image/x-portable-anymap				pnm
image/x-portable-bitmap				pbm
image/x-portable-graymap				pgm
image/x-portable-pixmap				ppm
image/x-rgb				rgb
image/x-xbitmap				xbm
image/x-xpixmap				xpm
image/x-xwindowdump				xwd

Because there were not present in the list but I think they are valid.

So, the final list is:
imageFilter=*.jpe; *.jpg; *.jpeg; *.gif; *.png; *.bmp; *.ico
moreImageFilter=*.svg; *.svgz; *.tif; *.tiff; *.cgm; *.g3; *.ief; *.pcx; *.btiff; *.psd; *.djv; *.dvju; *.dwg; *.dxf; *.fbs; *.fst; *.mmr; *.rlc; *.mdi; *.npx; *.wbmp; *.xif; *.ras; *.cmx; *.cdr; *.pat; *.cdt; *.cpt; *.fh; *.fh4; *.fh5; *.fh6; *.fh7; *.fhc; *.art; *.jng; *.pct; *.pic; *.pnm; *.pbm; *.pgm; *.ppm; *.rgb; *.xbm; *.xpm; *.xwd; *.fits; *.jp2; *.jpg2; *.jpm; *.jpgm; *.jpf; *.jpx; *.pti; *.t38; *.tfx *.pgb; *.pic; *.hdr; *.rgbe; *.xyze; *.spng; *.spn; *.s1n; *.sgif; *.sgi; *.s1g; *.sjpg; *.sjp; *.s1j; *.dxf

For information, all extensions after *.xwd were not in my mime file.

I really think we can't keep that mechanism in the long term. We really should use the user mime type to get the image file extensions and using |imageFilter| (ie. "*.jpe; *.jpg; *.jpeg; *.gif; *.png; *.bmp; *.ico") in case of the user mime type file is broken/missing. This would be future proof and enough in most usages. Indeed, we can consider most user will not need more than the |imageFilter| list and most users will have a working mime type file.
Comment 16 Mounir Lamouri (:mounir) 2010-04-26 07:18:34 PDT
Created attachment 441487 [details] [diff] [review]
Patch v1

This patch is doing what we chose to do on IRC. This should be a working first step.
Comment 17 Mounir Lamouri (:mounir) 2010-04-26 08:58:00 PDT
Builds with this patch are available here for testing:
https://build.mozilla.org/tryserver-builds/mlamouri@mozilla.com-1272291643/
Comment 18 Mounir Lamouri (:mounir) 2010-04-26 10:28:59 PDT
On Windows and GTK2 platforms, it's working as expected.
Unfortunately, it doesn't work on MacOS X but it looks like our file picker doesn't filter files on MacOS X like Opera's file picker.
Comment 19 Mounir Lamouri (:mounir) 2010-04-26 10:30:10 PDT
Josh, do you have more information about the MacOS X file picker filtering files ? (see previous comment)
Comment 20 Mounir Lamouri (:mounir) 2010-04-27 15:54:48 PDT
Created attachment 441966 [details] [diff] [review]
Patch v2
Comment 21 Mounir Lamouri (:mounir) 2010-04-27 16:00:43 PDT
So this new patch is only extending file extension list linked to |filterImage|. Boris told me |filterImage| wasn't only related to file extensions so it's better to use it than creating a new filter. The only side effect is if you filter per "Image Files" when doing "File -> Open" in Firefox, when using a widget filtering with file extensions, it will accept images that firefox can't show. That's not so bad I suppose as you can already open a file firefox doesn't handle with "All files" filter.
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-04-29 10:55:11 PDT
I just realized that we might want to add .raw as well.
Comment 23 Mounir Lamouri (:mounir) 2010-04-29 12:52:29 PDT
Created attachment 442486 [details] [diff] [review]
Patch v2.1

r=roc

I've updated the list to add *.raw.
Comment 24 Neil Deakin 2010-04-29 13:14:53 PDT
Comment on attachment 442486 [details] [diff] [review]
Patch v2.1

>+PRInt32
>+nsFileControlFrame::GetFileFilterFromAccept() const
>+{
>+  nsAutoString accept;
>+  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::accept, accept);
>+
>+  if (accept.EqualsLiteral("image/*")) {

You could also just use mContent->AttrValueIs(...), but I'm guessing other values could be supported in the future?
Comment 25 Mounir Lamouri (:mounir) 2010-04-29 13:20:55 PDT
(In reply to comment #24)
> (From update of attachment 442486 [details] [diff] [review])
> >+PRInt32
> >+nsFileControlFrame::GetFileFilterFromAccept() const
> >+{
> >+  nsAutoString accept;
> >+  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::accept, accept);
> >+
> >+  if (accept.EqualsLiteral("image/*")) {
> 
> You could also just use mContent->AttrValueIs(...), but I'm guessing other
> values could be supported in the future?

Exact ;)

I assume there is no need for a super-review so I'm marking this 'checkin-needed'. Don't hesitate to remove the keyword if you think a sr is needed.

r=roc,enndeakin
Comment 26 Mikko Rantalainen 2010-04-30 07:23:59 PDT
(In reply to comment #22)
> I just realized that we might want to add .raw as well.

Is that for raw digital photos (as in, raw sensor data from digital camera)?

In that case, you will also want .cr2 (canon raw format 2) and .nef (nikon's raw format).
Comment 27 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-04-30 13:39:58 PDT
Indeed! What reminded me was the announcement that Ubuntu was gonna stop shipping GIMP and start shipping F-Spot instead. Here is the list of file types that F-Spot supports:

http://f-spot.org/File_Types
Comment 28 :Ehsan Akhgari 2010-04-30 15:58:52 PDT
I think we should get a test for this before we can take this in.  Removing checkin-needed for now.
Comment 29 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-04-30 16:34:30 PDT
Unfortunately I don't know of a way to automate the filepicker. However we could write a litmus test. One example of how to do this is in bug 523771.
Comment 30 :Ehsan Akhgari 2010-04-30 16:44:45 PDT
(In reply to comment #29)
> Unfortunately I don't know of a way to automate the filepicker. However we
> could write a litmus test. One example of how to do this is in bug 523771.

One could replace the file picker service in the test with something which doesn't open up native file picker dialogs, and proceed from there.

I've done this once, so this might serve as a sample code:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/unit/test_privatebrowsing_downloadLastDir.js#162
Comment 31 Mounir Lamouri (:mounir) 2010-05-05 08:04:26 PDT
(In reply to comment #30)
> (In reply to comment #29)
> > Unfortunately I don't know of a way to automate the filepicker. However we
> > could write a litmus test. One example of how to do this is in bug 523771.
> 
> One could replace the file picker service in the test with something which
> doesn't open up native file picker dialogs, and proceed from there.
> 
> I've done this once, so this might serve as a sample code:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/unit/test_privatebrowsing_downloadLastDir.js#162

Can we do that this way ? nsIFilePicker do not let us know which files are displayed to the user only which ones are selected. So, as far as I understand it, your method can't be used.
I'm going to write a litmus test.

For the new requested extensions, I will open a follow-up. Let's start with the one in the patch for the moment.
Comment 32 Mounir Lamouri (:mounir) 2010-05-05 08:54:07 PDT
Created attachment 443624 [details]
Litmus test

Is something else needed for the test ? Who should I bug to have it added ?
Comment 33 :Ehsan Akhgari 2010-05-05 16:47:53 PDT
(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #29)
> > > Unfortunately I don't know of a way to automate the filepicker. However we
> > > could write a litmus test. One example of how to do this is in bug 523771.
> > 
> > One could replace the file picker service in the test with something which
> > doesn't open up native file picker dialogs, and proceed from there.
> > 
> > I've done this once, so this might serve as a sample code:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/unit/test_privatebrowsing_downloadLastDir.js#162
> 
> Can we do that this way ? nsIFilePicker do not let us know which files are
> displayed to the user only which ones are selected. So, as far as I understand
> it, your method can't be used.

That's not true.  You can implement nsIFilePicker::AppendFilter and nsIFilePicker::AppendFilters youself, and then check to see if the expected filters exist in your implementation of nsIFilePicker::Show.  Those filters are what is passed to the underlying platform APIs in order to only show some types of files to the user, therefore, if you write the test the way I described, you will be testing the correct thing.

> I'm going to write a litmus test.

As a general rule, we don't use Litmus tests for things which can be tested automatically in a unit test.  So, let's not rely on manual testing here.
Comment 34 Mounir Lamouri (:mounir) 2010-05-06 03:57:13 PDT
(In reply to comment #33)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > (In reply to comment #29)
> > > > Unfortunately I don't know of a way to automate the filepicker. However we
> > > > could write a litmus test. One example of how to do this is in bug 523771.
> > > 
> > > One could replace the file picker service in the test with something which
> > > doesn't open up native file picker dialogs, and proceed from there.
> > > 
> > > I've done this once, so this might serve as a sample code:
> > > 
> > > http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/unit/test_privatebrowsing_downloadLastDir.js#162
> > 
> > Can we do that this way ? nsIFilePicker do not let us know which files are
> > displayed to the user only which ones are selected. So, as far as I understand
> > it, your method can't be used.
> 
> That's not true.  You can implement nsIFilePicker::AppendFilter and
> nsIFilePicker::AppendFilters youself, and then check to see if the expected
> filters exist in your implementation of nsIFilePicker::Show.  Those filters are
> what is passed to the underlying platform APIs in order to only show some types
> of files to the user, therefore, if you write the test the way I described, you
> will be testing the correct thing.

How could I get the filters from nsIFilePicker::Show ? The only information which isn't platform dependent is |filterIndex| but we can't check anything with |filterIndex|. I must be missing something :-/
Comment 35 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-05-06 06:43:42 PDT
You can't automatically test the filepicker implementation, no. I.e. you can't automatically test that the nsIFilePicker fulfills the interface and displays an OS filepicker where only images are selected.

However you can test the accept attribute implementation in that you check that we request an image filter on nsIFilePicker when the accept attribute has the correct value.

So testing the implementation of nsIFilePicker can't be done automatically. Testing the usage of nsIFilePicker can.

Does that make sense?
Comment 36 :Ehsan Akhgari 2010-05-06 10:36:49 PDT
(In reply to comment #34)
> How could I get the filters from nsIFilePicker::Show ? The only information
> which isn't platform dependent is |filterIndex| but we can't check anything
> with |filterIndex|. I must be missing something :-/

You implement appendFilter(s), and store the filters set on the file picker object in an array, or something.  Then, when show is called, you check the elements inside that array to make sure that they're what you expect them to be.

As Jonas said in comment 35, you're providing your own implementation of nsIFilePicker, so that you can make sure that the expected filters are set on the file picker object.  If you ensure that, and if the real file picker object interprets the filters correctly and sets them on the OS level widget correctly, then you'll see the correct filtered OS widget.  But you don't need to test the implementation of the real file picker object in this test.
Comment 37 Mounir Lamouri (:mounir) 2010-05-10 04:05:29 PDT
Created attachment 444382 [details] [diff] [review]
Tests v1

I've been trying to simulate a click on the file picker but I think I've lost the fight and the only working event is "space". I'm not sure that's cross platform so I will send this to the try server. If anyone knows how to handle this correctly with a mouse event, please, do not hesitate :)
Comment 38 :Ehsan Akhgari 2010-05-10 08:30:12 PDT
Have you tried:

synthesizeMouse(i, 5, 5, {});
Comment 39 :Ehsan Akhgari 2010-05-10 08:39:10 PDT
Comment on attachment 444382 [details] [diff] [review]
Tests v1

>diff -r 7f6f3c0b527c content/html/content/test/test_bug377624.html
>+var input = document.createElement('input');
>+ok("accept" in input, "'accept' is a valid input attribute");

Nit: s/attribute/property/.

>diff -r 7f6f3c0b527c layout/forms/test/test_bug377624.html
>+  // methods
>+  appendFilter: function() {},
>+  appendFilters: function(val)
>+  {
>+    this._obs.notifyObservers(null, "TEST_FILEPICKER_APPENDFILTERS", val);
>+  },

This part of the test seems fragile.  You have left the implementation of appendFilter empty, and you also don't handle the case where appendFilters would be called twice correctly.

>+document.getElementById('i').addEventListener("focus", function (aEvent) {
>+    aEvent.target.removeEventListener("focus", arguments.callee, false);
>+    gFocusHandled = true;
>+    synthesizeKey('VK_SPACE', {});
>+  }, false);

I don't understand this part.  Why do you need to send the event on focus?  And anyway, I think using synthesizeMouse, you won't need this anyway.

>+function runTests()
>+{
>+  var i = document.getElementById('i');
>+
>+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+
>+  Cm.registerFactory(FILE_PICKER_ID,
>+                     FILE_PICKER_DESCRIPTION,
>+                     FILE_PICKER_CID,
>+                     factory);

You should unregister the factory when you're done, otherwise you'll be breaking other tests which may rely on this service.

>+  var obs = Cc["@mozilla.org/observer-service;1"].
>+            getService(Ci.nsIObserverService);
>+
>+  var observer = {
>+    observe: function(aSubject, aTopic, aData) {
>+      switch (aTopic) {
>+        case "TEST_FILEPICKER_APPENDFILTERS":
>+          this.filters = aData;
>+          break;
>+        case "TEST_FILEPICKER_SHOW":
>+          this.shown = true;
>+          this.filterIndex = aData;
>+          break;
>+      }
>+    },
>+    shown: false,
>+    filters: 0,
>+    filterIndex: 0
>+  };
>+
>+  obs.addObserver(observer, "TEST_FILEPICKER_APPENDFILTERS", false);
>+  obs.addObserver(observer, "TEST_FILEPICKER_SHOW", false);
>+
>+  i.focus();
>+
>+  setTimeout(function () {
>+      ok(gFocusHandled, "Focus should have been handled by the file element");
>+      ok(observer.shown, "File picker show method should have been called");
>+      is(observer.filterIndex, 1,
>+         "File picker should show the second filter index (first is zero)");
>+      is(observer.filters, FilePickerService.prototype.filterAll +
>+         FilePickerService.prototype.filterImages,
>+         "All and Images filters should have been added");
>+      SimpleTest.finish();
>+    }, 1000);

Why do you need a 1s timeout here?  Relying on timeouts this way will make your test subject to random failures.
Comment 40 Mounir Lamouri (:mounir) 2010-05-10 10:00:27 PDT
(In reply to comment #39)
> >diff -r 7f6f3c0b527c layout/forms/test/test_bug377624.html
> >+  // methods
> >+  appendFilter: function() {},
> >+  appendFilters: function(val)
> >+  {
> >+    this._obs.notifyObservers(null, "TEST_FILEPICKER_APPENDFILTERS", val);
> >+  },
> 
> This part of the test seems fragile.  You have left the implementation of
> appendFilter empty, and you also don't handle the case where appendFilters
> would be called twice correctly.

I've added a check to be sure |appendFilter| isn't called. |appendFilters| is correct because it is overwriting the previous value so we only have to check the one set just before |show| is called.

> >+document.getElementById('i').addEventListener("focus", function (aEvent) {
> >+    aEvent.target.removeEventListener("focus", arguments.callee, false);
> >+    gFocusHandled = true;
> >+    synthesizeKey('VK_SPACE', {});
> >+  }, false);
> 
> I don't understand this part.  Why do you need to send the event on focus?  And
> anyway, I think using synthesizeMouse, you won't need this anyway.

Because we need to have to focus on the element to have the space event sent to it.

> >+function runTests()
> >+{
> >+  var i = document.getElementById('i');
> >+
> >+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> >+
> >+  Cm.registerFactory(FILE_PICKER_ID,
> >+                     FILE_PICKER_DESCRIPTION,
> >+                     FILE_PICKER_CID,
> >+                     factory);
> 
> You should unregister the factory when you're done, otherwise you'll be
> breaking other tests which may rely on this service.

Oups, I didn't notice that, sorry :(

> >+  i.focus();
> >+
> >+  setTimeout(function () {
> >+      ok(gFocusHandled, "Focus should have been handled by the file element");
> >+      ok(observer.shown, "File picker show method should have been called");
> >+      is(observer.filterIndex, 1,
> >+         "File picker should show the second filter index (first is zero)");
> >+      is(observer.filters, FilePickerService.prototype.filterAll +
> >+         FilePickerService.prototype.filterImages,
> >+         "All and Images filters should have been added");
> >+      SimpleTest.finish();
> >+    }, 1000);
> 
> Why do you need a 1s timeout here?  Relying on timeouts this way will make your
> test subject to random failures.

It was in case the element isn't focused but I suppose you prefer to have a test timeout instead of random orange. It's fixed.

For the |synthesizeMouse| I did try what you've suggested in comment 38 and a lot of combinations and nothing seem to work. I've also tried ENTER instead of SPACE and it's not working. I'm a bit confused.
Comment 41 Mounir Lamouri (:mounir) 2010-05-10 10:01:33 PDT
Created attachment 444413 [details] [diff] [review]
Tests v2

This should fix every comments except the mouse event...
Comment 42 :Ehsan Akhgari 2010-05-10 10:17:37 PDT
(In reply to comment #40)
> (In reply to comment #39)
> > >diff -r 7f6f3c0b527c layout/forms/test/test_bug377624.html
> > >+  // methods
> > >+  appendFilter: function() {},
> > >+  appendFilters: function(val)
> > >+  {
> > >+    this._obs.notifyObservers(null, "TEST_FILEPICKER_APPENDFILTERS", val);
> > >+  },
> > 
> > This part of the test seems fragile.  You have left the implementation of
> > appendFilter empty, and you also don't handle the case where appendFilters
> > would be called twice correctly.
> 
> I've added a check to be sure |appendFilter| isn't called. |appendFilters| is
> correct because it is overwriting the previous value so we only have to check
> the one set just before |show| is called.

No, appendFilters does what it says: it appends filters to the previously added ones, and it doesn't (shouldn't) overwrite the previously appended filters.

My main concern here is that you're relying on an implementation detail here (that you only call appendFilters, and you do it once.)  If someone changes that for some reason, then this test will fail needlessly.  What I'd have done is to store the filters as an array, and just make appendFilter(s) push filters at the end of that array, and get the contents of the array whenever I needed them.

> > Why do you need a 1s timeout here?  Relying on timeouts this way will make your
> > test subject to random failures.
> 
> It was in case the element isn't focused but I suppose you prefer to have a
> test timeout instead of random orange. It's fixed.

Well, if you need to do something when the element is really focused, doing it inside the focus event handler seems like a better option.

> For the |synthesizeMouse| I did try what you've suggested in comment 38 and a
> lot of combinations and nothing seem to work. I've also tried ENTER instead of
> SPACE and it's not working. I'm a bit confused.

Hmm.  I find that really strange.  Maybe you can try setting a breakpoint in nsFileControlFrame::MouseListener::MouseClick and seeing what happens there?
Comment 43 Mounir Lamouri (:mounir) 2010-05-10 14:19:05 PDT
Created attachment 444477 [details] [diff] [review]
Tests v3
Comment 44 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-05-10 16:48:10 PDT
Comment on attachment 444477 [details] [diff] [review]
Tests v3

It seems somewhat scary to call SimpleTest.finish() from inside the filepicker implementation. You might want to do a simple setTimeout(0) and call your finish(aObs, aObserver) from there.

r=me with that.
Comment 45 Jeff Walden [:Waldo] (remove +bmo to email) 2010-05-10 16:51:07 PDT
s/setTimeout\(0\)/SimpleTest.executeSoon/
Comment 46 Mounir Lamouri (:mounir) 2010-05-11 03:07:18 PDT
Created attachment 444626 [details] [diff] [review]
Tests v3.1

r=sicking
Comment 47 Mounir Lamouri (:mounir) 2010-05-11 03:22:41 PDT
The try server doesn't like the new test:
MacOS: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1273528358.1273542217.18079.gz
GNU/Linux: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1273526967.1273539392.8604.gz
Windows: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1273526967.1273539063.7420.gz

It looks like layout/forms/test/test_bug536567.html is working if launched alone but not after the new test.
I've seen this assertion is thrown in both tests:
###!!! ASSERTION: null script filename: 'flags != JSFILENAME_NULL', file /home/volkmar/projects/mozilla/mozilla-central/src/js/src/xpconnect/src/xpcconvert.cpp, line 1307

And this is the error I got when layout/forms/test/test_bug536567.html is launched after the new one:
JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED) [nsIComponentManager.getClassObjectByContractID]"  nsresult: "0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)"  location: "JS frame :: chrome://mochikit/content/browser/toolkit/content/tests/browser/common/mockObjects.js :: MOR_register :: line 83"  data: no]

If you have an idea on top of your head, please shoot. Otherwise, I will investigate this later.
Comment 48 :Ehsan Akhgari 2010-05-11 11:23:44 PDT
Hmm, I should have also mentioned in comment 39 that you also need to re-register the old factory in order to restore things to their original status when you're done.  I guess that should fix the problem in comment 47.
Comment 49 Mounir Lamouri (:mounir) 2010-05-11 12:08:26 PDT
(In reply to comment #48)
> Hmm, I should have also mentioned in comment 39 that you also need to
> re-register the old factory in order to restore things to their original status
> when you're done.  I guess that should fix the problem in comment 47.

That's probably a stupid question but how do I get the old factory ? I've only seen |nsIComponentManagerObsolete.findFactory()| which could help here but I assume nsIComponentManagerObsolete should not be used anymore.
Comment 50 :Ehsan Akhgari 2010-05-11 13:01:54 PDT
Using nsIComponentManager::getClassObject before you register your factory?
Comment 51 Mounir Lamouri (:mounir) 2010-05-11 15:54:58 PDT
Created attachment 444770 [details] [diff] [review]
Tests v3.2

Thank you Ehsan :)
Comment 52 Mounir Lamouri (:mounir) 2010-05-12 02:59:38 PDT
Tests in the try server went fine this time, let's land this patch.
Comment 53 Mounir Lamouri (:mounir) 2010-05-12 03:26:45 PDT
Comment on attachment 265675 [details] [diff] [review]
Patch v1

Marking this patch obsolete. See bug 565274.
Comment 54 Mounir Lamouri (:mounir) 2010-05-12 03:27:16 PDT
Comment on attachment 265043 [details] [diff] [review]
Toolkit & Widget part (v1)

Marking this patch obsolete, see bug 565272.
Comment 55 Mounir Lamouri (:mounir) 2010-05-12 03:31:15 PDT
Btw, r=roc,enndeakin,sicking

Follow-up filed: bug 565272 and bug 565274
Comment 56 Mounir Lamouri (:mounir) 2010-05-19 16:52:13 PDT
Created attachment 446392 [details] [diff] [review]
Tests v3.3

New patch applied against the current tip.
Comment 58 Justin Wood (:Callek) 2010-05-20 00:04:27 PDT
(In reply to comment #47)
> The try server doesn't like the new test:
...
> JavaScript error: , line 0: uncaught exception: [Exception... "Component
> returned failure code: 0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)
> [nsIComponentManager.getClassObjectByContractID]"  nsresult: "0x80040154
> (NS_ERROR_FACTORY_NOT_REGISTERED)"  location: "JS frame ::
> chrome://mochikit/content/browser/toolkit/content/tests/browser/common/mockObjects.js
> :: MOR_register :: line 83"  data: no]

We hit this on the system, looks like you rebased the v3.1 test file, not the v3.2 file. I checked in the v3.2 changes in:
http://hg.mozilla.org/mozilla-central/rev/cdd4ec63ff26

And now the tree looks good.
Comment 59 Brad Lassey [:blassey] (use needinfo?) 2010-05-25 17:35:33 PDT
*** Bug 536984 has been marked as a duplicate of this bug. ***
Comment 60 Ammar 2010-06-21 06:49:32 PDT
Sorry if it is wrong time or place to raise my concern, but as far as I can understand, theoretically the actual mime type sent to server in request for a file input element can be other than image/... even if accept="image/*" and user selects a file without changing the filter.

Because here, for filter, we are using hard coded list, but for mime type for the selected file while submitting is picked through nsExternalHelperAppService::GetTypeFromExtension, which has complete algorithm to determine the mime type.

Wouldn't it be better if inverse of the same algorithm is used, so the results remain consistent.

(In reply to comment #11)
> Hrm.. Sorry for not commenting here previously.
> 
> My concern with this patch is that I don't think the OS mimetype->extension
> mapping is going to be good enough. For example if someone does
> 
> <input type=file accept="plain/text">
> 
> The user would be prevented from attaching a javascript file, although that is
> arguably a plaintext file.
> 
> And worse, what if an author does
> 
> <input type=file accept="application/msword">
> 
> This might work fine on most peoples computers, but not on a linux machine that
> doesn't have word installed.
> 

I understand the problem pointed out here, but it is very probable that mime type is also conformed at server end (at least it is recommended). so the submission will be rejected on the server end anyway.

Sorry if these things are already discussed and resolved on IRC or someplace else.
Comment 61 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-08-23 13:06:24 PDT
Hmm.. another thing I realized. It would make sense to implement form="" to mean that the element doesn't belong to *any* <form>. Even if it has a <form> ancestor.

That would allow putting a <input> that isn't intended for submission to be put inside the DOM without worrying about messing up ancestor <form>s.
Comment 62 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-08-23 13:07:28 PDT
Eeep, wrong bug, ignore previous comment.
Comment 63 brunoais 2011-07-02 03:01:11 PDT
This is resolved and fixed.
Where is it implemented?
Comment 65 Jack 2012-02-01 17:48:32 PST
It's been almost two years since it was decided that "image/*" was going to be the first step, but I yet have to see support for audio and video filter ;-)

Also, the Firefox implementation doesn't play nice in a comma separated list, such as "image/*,image/jpeg" which should work cross-browser or even "image/*,video/*" which should work in Firefox even if it didn't recognize the latter filter.

Personally I don't like the idea of mime-types either, I'd rather see a mix of extensions and mime-types (e.g. "jpg,png,video/*") which technically doesn't conflict ;-)
Comment 66 Stephan Sokolow 2012-06-02 21:14:42 PDT
A little something for the records, since I didn't see any clear answers.

Re: Comment 13

Here's what I can do to clear up some of those image types:

WBMP is a mobile image format (it's short for Wireless Bitmap) and XBM is a very simple, 8-bit grayscale, textual format commonly used by X11 (Linux, BSD, etc.) for embedding things like masks in C source code.

Both are supported by ImageMagick and PHP's version of libGD. http://ca.php.net/manual/en/function.gd-info.php

.pict is the non-MacOS extension for QuickDraw images, which set PICT as their type code in the HFS Resource Fork.

.g3f and .g3n are just proprietary file extensions used by Zetafax for storing fine-resolution and normal-resolution images in TIFF format so they can be hooked into the file associations system separately from normal TIFF images.

I'm not actually sure what CPR, RF, and RP are. They're used by several different formats and none of them really scream out "a browser should recognize me as an image" aside from maybe RF sometimes being used for "Sun Raster Graphics".

Re: Comment 15

You also forgot the JB2 extension that's sometimes used for JBIG2-format faxes.

Re: Comment 65

Either that, or you could do it how ~/.local/share/applications/mimeapps.list does it on Linux and discuss standardizing a syntax for placeholder mimetypes with other browser projects.

application/x-extension-mht

...of course, that may be overkill since you don't also need stuff like x-scheme-handler/desura and inode/directory.
Comment 67 Mounir Lamouri (:mounir) 2012-06-04 08:36:06 PDT
Stephan, could you open a follow-up bugs to add/remove those extensions?

For the rest, please see bug 565274.
Comment 68 Stephan Sokolow 2012-06-16 15:10:04 PDT
Re: Comment 67:

I finally caught up to the backlog exam-cramming left me with. Could you clarify which parts you think merit their own bugs?
Comment 69 Mounir Lamouri (:mounir) 2012-06-19 07:28:20 PDT
(In reply to Stephan Sokolow from comment #68)
> Re: Comment 67:
> 
> I finally caught up to the backlog exam-cramming left me with. Could you
> clarify which parts you think merit their own bugs?

The parts where you say we should add and remove some extensions from the list.
Comment 70 Stephan Sokolow 2012-06-19 08:50:58 PDT
(In reply to Mounir Lamouri (:mounir) from comment #69)
> (In reply to Stephan Sokolow from comment #68)
> > Re: Comment 67:
> > 
> > I finally caught up to the backlog exam-cramming left me with. Could you
> > clarify which parts you think merit their own bugs?
> 
> The parts where you say we should add and remove some extensions from the
> list.

Ok. Where would I find the current list of recognized extensions?
Comment 71 Mounir Lamouri (:mounir) 2012-06-19 08:55:39 PDT
(In reply to Stephan Sokolow from comment #70)
> (In reply to Mounir Lamouri (:mounir) from comment #69)
> > (In reply to Stephan Sokolow from comment #68)
> > > Re: Comment 67:
> > > 
> > > I finally caught up to the backlog exam-cramming left me with. Could you
> > > clarify which parts you think merit their own bugs?
> > 
> > The parts where you say we should add and remove some extensions from the
> > list.
> 
> Ok. Where would I find the current list of recognized extensions?

Here: toolkit/content/filepicker.properties
Comment 72 Stephan Sokolow 2012-06-19 09:03:33 PDT
(In reply to Mounir Lamouri (:mounir) from comment #71)
> Here: toolkit/content/filepicker.properties

The closest I've come to working under the hood was the little bit of experimental extension development I did nearly a decade ago when the docs were horrible and the need to babysit maxVersion quickly drove me away.

What's the fastest way to resolve that to a URL in something fresher than the copy of Aurora I use for day-to-day browsing?
Comment 73 Mounir Lamouri (:mounir) 2012-06-19 09:05:28 PDT
I'm not sure I understood. Are you looking for that?
https://mxr.mozilla.org/mozilla-central/source/toolkit/content/filepicker.properties
Comment 74 Stephan Sokolow 2012-06-19 09:11:25 PDT
(In reply to Mounir Lamouri (:mounir) from comment #73)
> I'm not sure I understood. Are you looking for that?
> https://mxr.mozilla.org/mozilla-central/source/toolkit/content/filepicker.
> properties

Yeah. Thanks. I remembered there being something like that, but I couldn't remember the URL and I wasn't sure whether the one I was thinking of and the password-protected one I occasionally see linked were one and the same.
Comment 75 Stephan Sokolow 2012-06-19 14:55:07 PDT
(In reply to Mounir Lamouri (:mounir) from comment #67)
> Stephan, could you open a follow-up bugs to add/remove those extensions?
> 
> For the rest, please see bug 565274.

One last question. When you say "bugs", do you mean that I should open an individual bug for each file extension?

If not, what approach would you recommend?
Comment 76 Mounir Lamouri (:mounir) 2012-06-20 01:39:29 PDT
You can open a bug and list the changes that you think should be made.

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