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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
DOM
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: sheppy, Assigned: mounir)

Tracking

({dev-doc-complete})

Trunk
mozilla2.0b7
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 2

7 years ago
Tracking documentation readiness for click() on file inputs off this bug instead of the original one.
Keywords: dev-doc-needed
This should block.
Blocks: 36619
(Assignee)

Updated

7 years ago
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → mounir.lamouri
(Assignee)

Comment 4

7 years ago
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()
(Assignee)

Comment 6

7 years ago
(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.
(Assignee)

Comment 8

7 years ago
bug 592835 makes testing harder so I'm marking it as a dependency and will fix it.
Depends on: 592835
(Assignee)

Comment 9

7 years ago
Actually, this patch will fix bug 592835
Blocks: 592835
No longer depends on: 592835
(Assignee)

Comment 10

7 years ago
Created attachment 473454 [details] [diff] [review]
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...
Attachment #473454 - Flags: review?(jonas)
(Assignee)

Comment 11

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

Updated

7 years ago
Blocks: 595377

Comment 12

7 years ago
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?
(Assignee)

Comment 13

7 years ago
(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.
(Assignee)

Comment 14

7 years ago
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.
(Assignee)

Comment 17

7 years ago
Thank you for the review :)

Updated

7 years ago
Duplicate of this bug: 596159
(Assignee)

Comment 19

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

This should fix this bug, bug 595377 and bug 592835.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
Depends on: 597811
(Reporter)

Comment 20

7 years ago
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.