Closed
Bug 592802
Opened 15 years ago
Closed 15 years ago
Calling click() on display:none file input elements does not invoke onchange
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla2.0b7
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: sheppy, Assigned: mounir)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file)
11.51 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
Tracking documentation readiness for click() on file inputs off this bug instead of the original one.
Keywords: dev-doc-needed
Comment 3•15 years ago
|
||
This should block.
Assignee | ||
Updated•15 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → mounir.lamouri
Assignee | ||
Comment 4•15 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•15 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•15 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•15 years ago
|
||
Actually, this patch will fix bug 592835
Assignee | ||
Comment 10•15 years ago
|
||
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•15 years ago
|
||
Would be nice to have this pushed for beta6. Seems like it would fix some crashes (at least 2 bug reports).
Comment 12•15 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•15 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•15 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•15 years ago
|
||
Thank you for the review :)
Assignee | ||
Comment 19•15 years ago
|
||
Pushed:
http://hg.mozilla.org/mozilla-central/rev/5f2250cb58c9
This should fix this bug, bug 595377 and bug 592835.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
Reporter | ||
Comment 20•15 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
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•