Assume the following form: <input type="file" name="file" id="file" multile="multiple" onchange="alert(100)" /> (1) Click "Choose Files", select 1.jpg and 2.jpg, and click "OK". (2) The 'onchange' event is triggered. (3) Again, click "Choose Files", select 1.jpg and 3.jpg, and click "OK". Expected behavior: The 'onchange' event is triggered. Actual behavior: Nothing happens. Accurately, the 'onchage' event seems to be triggered only if the first item of newly selected files is same as the first item of previously selected files.
http://code.google.com/p/chromium/issues/detail?id=86909
Created attachment 99694 [details] Patch
Comment on attachment 99694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99694&action=review > LayoutTests/fast/forms/file-input-change-event.html:19 > + var single_file_input = document.getElementById("single_file"); > + var multiple_files_input = document.getElementById("multiple_files"); We don't have a formal JavaScript style guide, but we usually use rules similar to C++. So variable names and function names should be like singleFileInput. > Source/WebCore/ChangeLog:19 > + * html/HTMLFormControlElement.cpp: > + (WebCore::HTMLTextFormControlElement::dispatchFileInputFormControlChangeEvent): Compares the previously selected files and the newly selected files > + * html/HTMLFormControlElement.h: > + (WebCore::HTMLTextFormControlElement::setPathsAsOfLastFormControlChangeEvent): Records the newly selected files This approach is not acceptable. We don't want to add type-specific data members to HTMLInputElement. FileInputType::filesChosen() knows both of the previously-selected files (m_fileList) and newly-selected files. We can check the need to dispatch 'change' in filesChosen(). > Source/WebCore/html/HTMLFormControlElement.cpp:690 > + if (pathsChanged) { > + HTMLElement::dispatchChangeEvent(); > + setPathsAsOfLastFormControlChangeEvent(paths); > + } > + setChangedSinceLastFormControlChangeEvent(false); dispatchChangeEvent() invokes a JavaScript code, and it can delete 'this' object. So you can't access 'this' after dispatchChangeEvent(). You can avoid such problem by the protector idiom.
Created attachment 99774 [details] Patch
Comment on attachment 99694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99694&action=review >> LayoutTests/fast/forms/file-input-change-event.html:19 >> + var multiple_files_input = document.getElementById("multiple_files"); > > We don't have a formal JavaScript style guide, but we usually use rules similar to C++. So variable names and function names should be like singleFileInput. Done. >> Source/WebCore/ChangeLog:19 >> + (WebCore::HTMLTextFormControlElement::setPathsAsOfLastFormControlChangeEvent): Records the newly selected files > > This approach is not acceptable. We don't want to add type-specific data members to HTMLInputElement. > FileInputType::filesChosen() knows both of the previously-selected files (m_fileList) and newly-selected files. We can check the need to dispatch 'change' in filesChosen(). Done. >> Source/WebCore/html/HTMLFormControlElement.cpp:690 >> + setChangedSinceLastFormControlChangeEvent(false); > > dispatchChangeEvent() invokes a JavaScript code, and it can delete 'this' object. So you can't access 'this' after dispatchChangeEvent(). > You can avoid such problem by the protector idiom. I see. Anyway, I removed this method.
Comment on attachment 99774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99774&action=review > LayoutTests/fast/forms/file-input-change-event.html:93 > +function SingleFileSelected() { > + onChangeCalled = true; > +} > + > +function MultipleFilesSelected() { > + onChangeCalled = true; > +} > + > +function moveMouseToCenterOfElement(element) { Inconsistent names. SingleFileSelected -> singleFileSelected MultipleFileSelected -> multipleFileSelected > Source/WebCore/html/FileInputType.cpp:310 > + // This call may cause destruction of this instance and thus must always be last in the function. > + input->HTMLElement::dispatchChangeEvent(); > + } > + input->setChangedSinceLastFormControlChangeEvent(false); > } The comment is lame. "This call" isn't on the last of the function. In this case, you don't use 'this' after the dispatchChangeEvent() call. So it's safe about 'this'. However 'input' might be deleted by dispatchChangeEvent(). You need to protect 'input' by replacing: HTMLInputElement* input = element(); at the beginning of the function with RefPtr<HTMLInputElement> input = element();
Created attachment 99779 [details] Patch
Comment on attachment 99779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99779&action=review > Source/WebCore/html/FileInputType.cpp:307 > + // This call may cause destruction of |this| instance. > + // |input| instance is safe since it is ref-counted. We don't use the || notation in WebKit.
Created attachment 99781 [details] Patch
Comment on attachment 99781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99781&action=review > LayoutTests/ChangeLog:8 > + the newly selected files, we trigger the 'onchange' event. nit: 'onchange' is an attribute name. The event name is 'change'. > LayoutTests/ChangeLog:11 > + > + Reviewed by Kent Tamura. > + The recent ChangeLog format is: <summary> <bug URL> <Reviewd by> <details> Also, don't fill this field while you're asking to review. > LayoutTests/fast/forms/file-input-change-event.html:15 > +var onChangeCalled; nit: onChange is an attribute name. It should be handlerCalled, changeHandlerCalled, changeDispatched, etc. > Source/WebCore/ChangeLog:12 > + https://bugs.webkit.org/show_bug.cgi?id=63929 > + > + Record previously selected files and then compare them with newly selected files. > + If the previously selected files and their order are not equal to those of > + the newly selected files, we trigger the 'change' event. > + > + Reviewed by Kent Tamura. > + > + Test: fast/forms/file-input-change-event.html Should be bug-URL - Reviewd by - Detail - Test: order. > Source/WebCore/html/FileInputType.cpp:286 > + for (unsigned i = 0; i < paths.size(); i++) { We prefer ++i for 'for'.
Created attachment 99787 [details] Patch
Comment on attachment 99787 [details] Patch ok Thank you for fixing this!
Comment on attachment 99787 [details] Patch Clearing flags on attachment: 99787 Committed r90438: <http://trac.webkit.org/changeset/90438>
All reviewed patches have been landed. Closing bug.
*** Bug 66907 has been marked as a duplicate of this bug. ***