Bug 63929 – A 'change' event is not triggered on a multiple file form when selected files are changed

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kentaro Hara]

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.

[Kent Tamura]

http://code.google.com/p/chromium/issues/detail?id=86909

[Kentaro Hara]

Created attachment 99694 [details]
Patch

[Kent Tamura]

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.

[Kentaro Hara]

Created attachment 99774 [details]
Patch

[Kentaro Hara]

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.

[Kent Tamura]

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();

[Kentaro Hara]

Created attachment 99779 [details]
Patch

[Kent Tamura]

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.

[Kentaro Hara]

Created attachment 99781 [details]
Patch

[Kent Tamura]

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'.

[Kentaro Hara]

Created attachment 99787 [details]
Patch

[Kent Tamura]

Comment on attachment 99787 [details]
Patch

ok
Thank you for fixing this!

[WebKit Review Bot]

Comment on attachment 99787 [details]
Patch

Clearing flags on attachment: 99787

Committed r90438: <http://trac.webkit.org/changeset/90438>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

*** Bug 66907 has been marked as a duplicate of this bug. ***