377624 - Implement the accept attribute for the form and file upload controls form "image/*"

Status: RESOLVED FIXED

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

Description

[Ryan Jones-Ward [:sciguyryan]]

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 2

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Toolkit & Widget part (v1)

First part of patch v1, dealing with the changes to toolkit and widget.

Comment 3

[Asaf Romano (gone)]

Comment on attachment 265043 [details] [diff] [review]
Toolkit & Widget part (v1)

mpa=mano for the locale changes.

Comment 4

[Ryan Jones-Ward [:sciguyryan]]

Attachment: 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-Ward [:sciguyryan]]

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?)]

Jonas, can you do this review?

Comment 7

[Ryan Jones-Ward [:sciguyryan]]

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)]

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-Ward [:sciguyryan]]

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)]

I'm taking this bug per previous comment.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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]

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)]

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]

(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)]

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)]

Attachment: 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)]

Builds with this patch are available here for testing:
https://build.mozilla.org/tryserver-builds/[e-mail]-1272291643/

Comment 18

[Mounir Lamouri (:mounir)]

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)]

Josh, do you have more information about the MacOS X file picker filtering files ? (see previous comment)

Comment 20

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 21

[Mounir Lamouri (:mounir)]

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]

I just realized that we might want to add .raw as well.

Comment 23

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

r=roc

I've updated the list to add *.raw.

Comment 24

[Neil Deakin]

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)]

(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]

(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]

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

[(no longer active)]

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]

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

[(no longer active)]

(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)]

(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)]

Attachment: Litmus test

Is something else needed for the test ? Who should I bug to have it added ?

Comment 33

[(no longer active)]

(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)]

(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]

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

[(no longer active)]

(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)]

Attachment: 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

[(no longer active)]

Have you tried:

synthesizeMouse(i, 5, 5, {});

Comment 39

[(no longer active)]

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)]

(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)]

Attachment: Tests v2

This should fix every comments except the mouse event...

Comment 42

[(no longer active)]

(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)]

Attachment: Tests v3

Comment 44

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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]]

s/setTimeout\(0\)/SimpleTest.executeSoon/

Comment 46

[Mounir Lamouri (:mounir)]

Attachment: Tests v3.1

r=sicking

Comment 47

[Mounir Lamouri (:mounir)]

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

[(no longer active)]

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)]

(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

[(no longer active)]

Using nsIComponentManager::getClassObject before you register your factory?

Comment 51

[Mounir Lamouri (:mounir)]

Attachment: Tests v3.2

Thank you Ehsan :)

Comment 52

[Mounir Lamouri (:mounir)]

Tests in the try server went fine this time, let's land this patch.

Comment 53

[Mounir Lamouri (:mounir)]

Comment on attachment 265675 [details] [diff] [review]
Patch v1

Marking this patch obsolete. See bug 565274.

Comment 54

[Mounir Lamouri (:mounir)]

Comment on attachment 265043 [details] [diff] [review]
Toolkit & Widget part (v1)

Marking this patch obsolete, see bug 565272.

Comment 55

[Mounir Lamouri (:mounir)]

Btw, r=roc,enndeakin,sicking

Follow-up filed: bug 565272 and bug 565274

Comment 56

[Mounir Lamouri (:mounir)]

Attachment: Tests v3.3

New patch applied against the current tip.

Comment 57

[Justin Wood (:Callek)]

Pushed http://hg.mozilla.org/mozilla-central/rev/103d84b4b399 and http://hg.mozilla.org/mozilla-central/rev/cec477f51801

Comment 58

[Justin Wood (:Callek)]

(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 60

[Ammar]

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]

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]

Eeep, wrong bug, ignore previous comment.

Comment 63

[brunoais]

This is resolved and fixed.
Where is it implemented?

Comment 64

[Mounir Lamouri (:mounir)]

(In reply to comment #63)
> This is resolved and fixed.
> Where is it implemented?

See comment 57.

Otherwise, have a look at:
https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#267
and:
https://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/nsBaseFilePicker.cpp#94
and:
https://mxr.mozilla.org/mozilla-central/source/toolkit/content/filepicker.properties

Comment 65

[Jack]

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]

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)]

Stephan, could you open a follow-up bugs to add/remove those extensions?

For the rest, please see bug 565274.

Comment 68

[Stephan Sokolow]

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)]

(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]

(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)]

(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]

(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)]

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]

(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]

(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)]

You can open a bug and list the changes that you think should be made.