Closed
Bug 377624
Opened 18 years ago
Closed 15 years ago
Implement the accept attribute for the form and file upload controls form "image/*"
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: sciguyryan, Assigned: mounir)
References
(Depends on 1 open bug, )
Details
(Keywords: dev-doc-complete, html5)
Attachments
(2 files, 10 obsolete files)
5.26 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
7.44 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 2•18 years ago
|
||
First part of patch v1, dealing with the changes to toolkit and widget.
Attachment #265043 -
Flags: review?(mano)
Comment 3•18 years ago
|
||
Comment on attachment 265043 [details] [diff] [review]
Toolkit & Widget part (v1)
mpa=mano for the locale changes.
Attachment #265043 -
Flags: review?(mano)
Reporter | ||
Comment 4•18 years ago
|
||
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.
Attachment #265675 -
Flags: review?(jonas)
Reporter | ||
Comment 5•18 years ago
|
||
Comment on attachment 265043 [details] [diff] [review]
Toolkit & Widget part (v1)
Asking roc to review the widget changes here.
Attachment #265043 -
Flags: review?(roc)
Attachment #265043 -
Flags: superreview+
Attachment #265043 -
Flags: review?(roc)
Attachment #265043 -
Flags: review+
Reporter | ||
Comment 7•16 years ago
|
||
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•16 years ago
|
||
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.
Updated•16 years ago
|
Keywords: checkin-needed
Updated•16 years ago
|
Flags: wanted1.9.2?
Whiteboard: [needs review jonas]
Target Milestone: mozilla1.9alpha8 → ---
Reporter | ||
Comment 9•15 years ago
|
||
Anyone looking to take over this bug may do so. I currently have no time to work on this.
Thanks everyone!
Assignee | ||
Comment 10•15 years ago
|
||
I'm taking this bug per previous comment.
Assignee: sciguyryan → mounir.lamouri
Status: ASSIGNED → NEW
Whiteboard: [needs review jonas]
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 on attachment 265675 [details] [diff] [review]
Patch v1
Resetting review request per above comment. Feel free to rerequest if you disagree.
Attachment #265675 -
Flags: review?(jonas)
Assignee | ||
Comment 13•15 years ago
|
||
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 ?
(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.
Assignee | ||
Comment 15•15 years ago
|
||
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.
Status: NEW → ASSIGNED
Assignee | ||
Comment 16•15 years ago
|
||
This patch is doing what we chose to do on IRC. This should be a working first step.
Attachment #441487 -
Flags: review?(jonas)
Assignee | ||
Comment 17•15 years ago
|
||
Builds with this patch are available here for testing:
https://build.mozilla.org/tryserver-builds/mlamouri@mozilla.com-1272291643/
Assignee | ||
Comment 18•15 years ago
|
||
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.
Assignee | ||
Comment 19•15 years ago
|
||
Josh, do you have more information about the MacOS X file picker filtering files ? (see previous comment)
Assignee | ||
Comment 20•15 years ago
|
||
Attachment #441487 -
Attachment is obsolete: true
Attachment #441966 -
Flags: review?(roc)
Attachment #441487 -
Flags: review?(jonas)
Assignee | ||
Updated•15 years ago
|
Attachment #441966 -
Flags: review?(enndeakin)
Assignee | ||
Comment 21•15 years ago
|
||
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.
Attachment #441966 -
Flags: review?(roc) → review+
I just realized that we might want to add .raw as well.
Assignee | ||
Comment 23•15 years ago
|
||
r=roc
I've updated the list to add *.raw.
Attachment #441966 -
Attachment is obsolete: true
Attachment #442486 -
Flags: review?(enndeakin)
Attachment #441966 -
Flags: review?(enndeakin)
Updated•15 years ago
|
Attachment #442486 -
Flags: review?(enndeakin) → review+
Comment 24•15 years ago
|
||
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?
Assignee | ||
Comment 25•15 years ago
|
||
(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
Keywords: checkin-needed,
dev-doc-needed
Comment 26•15 years ago
|
||
(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).
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•15 years ago
|
||
I think we should get a test for this before we can take this in. Removing checkin-needed for now.
Flags: in-testsuite?
Keywords: checkin-needed
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•15 years ago
|
||
(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
Assignee | ||
Comment 31•15 years ago
|
||
(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.
Assignee | ||
Comment 32•15 years ago
|
||
Is something else needed for the test ? Who should I bug to have it added ?
Comment 33•15 years ago
|
||
(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.
Flags: in-litmus-
Assignee | ||
Comment 34•15 years ago
|
||
(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 :-/
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•15 years ago
|
||
(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.
Assignee | ||
Comment 37•15 years ago
|
||
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 :)
Attachment #443624 -
Attachment is obsolete: true
Attachment #444382 -
Flags: review?(jonas)
Attachment #444382 -
Flags: feedback?(ehsan)
Comment 38•15 years ago
|
||
Have you tried:
synthesizeMouse(i, 5, 5, {});
Comment 39•15 years ago
|
||
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.
Attachment #444382 -
Flags: feedback?(ehsan) → feedback-
Assignee | ||
Comment 40•15 years ago
|
||
(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.
Assignee | ||
Comment 41•15 years ago
|
||
This should fix every comments except the mouse event...
Attachment #444382 -
Attachment is obsolete: true
Attachment #444413 -
Flags: feedback?(ehsan)
Attachment #444382 -
Flags: review?(jonas)
Comment 42•15 years ago
|
||
(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?
Assignee | ||
Comment 43•15 years ago
|
||
Attachment #444413 -
Attachment is obsolete: true
Attachment #444477 -
Flags: feedback?(ehsan)
Attachment #444413 -
Flags: feedback?(ehsan)
Updated•15 years ago
|
Attachment #444477 -
Flags: feedback?(ehsan) → feedback+
Assignee | ||
Updated•15 years ago
|
Attachment #444477 -
Flags: review?(jonas)
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.
Attachment #444477 -
Flags: review?(jonas) → review+
Comment 45•15 years ago
|
||
s/setTimeout\(0\)/SimpleTest.executeSoon/
Assignee | ||
Comment 47•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 49•15 years ago
|
||
(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•15 years ago
|
||
Using nsIComponentManager::getClassObject before you register your factory?
Assignee | ||
Comment 51•15 years ago
|
||
Thank you Ehsan :)
Attachment #444626 -
Attachment is obsolete: true
Assignee | ||
Comment 52•15 years ago
|
||
Tests in the try server went fine this time, let's land this patch.
Keywords: checkin-needed
Assignee | ||
Comment 53•15 years ago
|
||
Comment on attachment 265675 [details] [diff] [review]
Patch v1
Marking this patch obsolete. See bug 565274.
Attachment #265675 -
Attachment is obsolete: true
Assignee | ||
Comment 54•15 years ago
|
||
Comment on attachment 265043 [details] [diff] [review]
Toolkit & Widget part (v1)
Marking this patch obsolete, see bug 565272.
Attachment #265043 -
Attachment is obsolete: true
Assignee | ||
Comment 55•15 years ago
|
||
Btw, r=roc,enndeakin,sicking
Follow-up filed: bug 565272 and bug 565274
Assignee | ||
Updated•15 years ago
|
Summary: Implement the accept attribute for the form and file upload controls → Implement the accept attribute for the form and file upload controls form "image/*"
Assignee | ||
Comment 56•15 years ago
|
||
New patch applied against the current tip.
Attachment #444770 -
Attachment is obsolete: true
Comment 57•15 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/103d84b4b399 and http://hg.mozilla.org/mozilla-central/rev/cec477f51801
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Comment 58•15 years ago
|
||
(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.
Assignee | ||
Updated•15 years ago
|
Flags: in-testsuite? → in-testsuite+
Comment 60•14 years ago
|
||
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.
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.
Eeep, wrong bug, ignore previous comment.
Updated•14 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•14 years ago
|
Flags: wanted1.9.2?
Depends on: 643576
Comment 63•13 years ago
|
||
This is resolved and fixed.
Where is it implemented?
Assignee | ||
Comment 64•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 67•13 years ago
|
||
Stephan, could you open a follow-up bugs to add/remove those extensions?
For the rest, please see bug 565274.
Comment 68•12 years ago
|
||
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?
Assignee | ||
Comment 69•12 years ago
|
||
(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•12 years ago
|
||
(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?
Assignee | ||
Comment 71•12 years ago
|
||
(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•12 years ago
|
||
(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?
Assignee | ||
Comment 73•12 years ago
|
||
I'm not sure I understood. Are you looking for that?
https://mxr.mozilla.org/mozilla-central/source/toolkit/content/filepicker.properties
Comment 74•12 years ago
|
||
(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•12 years ago
|
||
(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?
Assignee | ||
Comment 76•12 years ago
|
||
You can open a bug and list the changes that you think should be made.
You need to log in
before you can comment on or make changes to this bug.
Description
•