Closed Bug 592802 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: sheppy, Assigned: mounir)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

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.
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.
blocking2.0: --- → betaN+
Component: Layout: Form Controls → DOM
QA Contact: layout.form-controls → general
Tracking documentation readiness for click() on file inputs off this bug instead of the original one.
Keywords: dev-doc-needed
This should block.
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → mounir.lamouri
This only happens if the input file is has display:none. visibility:hidden or even visible works.
Status: NEW → ASSIGNED
Summary: Calling click() on file input elements does not invoke onchange → Calling click() on display:none file input elements does not invoke onchange
Ah, though I think display:none is a major use case for fileinput.click()
(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 :/
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.
bug 592835 makes testing harder so I'm marking it as a dependency and will fix it.
Depends on: 592835
Actually, this patch will fix bug 592835
Blocks: 592835
No longer depends on: 592835
Attached patch Patch v1Splinter Review
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...
Attachment #473454 - Flags: review?(jonas)
Would be nice to have this pushed for beta6. Seems like it would fix some crashes (at least 2 bug reports).
Blocks: 595377
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?
(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.
CCing people who could review the patch instead of sicking.
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.
Attachment #473454 - Flags: review?(jonas) → review+
Oh, but do get rid of the other usage of frames. Either in this bug or a separate one.
Thank you for the review :)
Duplicate of this bug: 596159
Pushed:
http://hg.mozilla.org/mozilla-central/rev/5f2250cb58c9

This should fix this bug, bug 595377 and bug 592835.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
Depends on: 597811
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.