592802 - Calling click() on display:none file input elements does not invoke onchange

Status: RESOLVED FIXED

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

Description

[Eric Shepherd [:sheppy]]

The onchange handler for file input elements does not get called if you use click() to open the file picker. This makes it hard for code to detect the user selecting files.

Comment 1

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

Yup, we should fix this.

David, if you have time, feel free to jump in and fix this. But since I know you're short on time these days giving this to Mounir for now.

Comment 2

[Eric Shepherd [:sheppy]]

Tracking documentation readiness for click() on file inputs off this bug instead of the original one.

Comment 3

[Christopher Blizzard (:blizzard)]

This should block.

Comment 4

[Mounir Lamouri (:mounir)]

This only happens if the input file is has display:none. visibility:hidden or even visible works.

Comment 5

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

Ah, though I think display:none is a major use case for fileinput.click()

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to comment #5)
> Ah, though I think display:none is a major use case for fileinput.click()

I think I have a fix. The bug happens because we have no frame and we rely on the text frame to send the event. We just need to send the event ourself.
However, writing test might be hard :/

Comment 7

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

You'll have to use a fake file-picker implementation, the same way we do for some the other .click() tests. To return an nsIFile, you can use code similar to this:

http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug543870.html?force=1#143

to get the filename, and then use that to create an nsIFile.

Comment 8

[Mounir Lamouri (:mounir)]

bug 592835 makes testing harder so I'm marking it as a dependency and will fix it.

Comment 9

[Mounir Lamouri (:mounir)]

Actually, this patch will fix bug 592835

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Two things:
+    NS_ENSURE_TRUE(iter, NS_ERROR_UNEXPECTED);
is because iter might be null, I got crash while testing if GetFiles was returning null.
and
-    while (NS_SUCCEEDED(iter->GetNext(getter_AddRefs(tmp)))) {
+    PRBool loop = PR_TRUE;
+    while (NS_SUCCEEDED(iter->HasMoreElements(&loop)) && loop) {
+      iter->GetNext(getter_AddRefs(tmp));
because HasMoreElements should be called before calling GetNext.

Those two changes are not really related to the patch but are needed (the tests are even failing without the second).

By the way, |InitFocusedValue| was needed for the change event (look at nsTextControlFrame.h), the comment before the call wasn't really helping...

Comment 11

[Mounir Lamouri (:mounir)]

Would be nice to have this pushed for beta6. Seems like it would fix some crashes (at least 2 bug reports).

Comment 12

[chris hofmann]

yeah bug 592835 is the #11 topcrash in beta5. jonas, can you review?  is that all thats needed to get this landed before b6 freeze?

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to comment #12)
> yeah bug 592835 is the #11 topcrash in beta5. jonas, can you review?  is that
> all thats needed to get this landed before b6 freeze?

Yes, I only need a review.

Comment 14

[Mounir Lamouri (:mounir)]

CCing people who could review the patch instead of sicking.

Comment 15

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

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

>   // Collect new selected filenames
>   nsTArray<nsString> newFileNames;
>   if (multi) {
>     nsCOMPtr<nsISimpleEnumerator> iter;
>     rv = filePicker->GetFiles(getter_AddRefs(iter));
>     NS_ENSURE_SUCCESS(rv, rv);
>+    NS_ENSURE_TRUE(iter, NS_ERROR_UNEXPECTED);

I would leave out this test, but it's up to you.

r=me.

Comment 16

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

Oh, but do get rid of the other usage of frames. Either in this bug or a separate one.

Comment 17

[Mounir Lamouri (:mounir)]

Thank you for the review :)

Comment 19

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/5f2250cb58c9

This should fix this bug, bug 595377 and bug 592835.

Comment 20

[Eric Shepherd [:sheppy]]

click() on file inputs now documented:

https://developer.mozilla.org/en/HTML/Element/input#Notes

https://developer.mozilla.org/en/Using_files_from_web_applications#Hiding_the_file_input_element