Bug 63929 - A 'change' event is not triggered on a multiple file form when selected files are changed
Summary: A 'change' event is not triggered on a multiple file form when selected files...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 66907 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-07-05 03:24 PDT by Kentaro Hara
Modified: 2011-09-07 22:33 PDT (History)
4 users (show)

See Also:


Attachments
Patch (12.21 KB, patch)
2011-07-05 03:59 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (10.09 KB, patch)
2011-07-05 18:45 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (10.15 KB, patch)
2011-07-05 19:12 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (10.15 KB, patch)
2011-07-05 19:17 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (10.22 KB, patch)
2011-07-05 21:12 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2011-07-05 03:24:44 PDT
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.
Comment 2 Kentaro Hara 2011-07-05 03:59:35 PDT
Created attachment 99694 [details]
Patch
Comment 3 Kent Tamura 2011-07-05 05:02:10 PDT
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.
Comment 4 Kentaro Hara 2011-07-05 18:45:13 PDT
Created attachment 99774 [details]
Patch
Comment 5 Kentaro Hara 2011-07-05 18:45:39 PDT
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 6 Kent Tamura 2011-07-05 19:01:03 PDT
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();
Comment 7 Kentaro Hara 2011-07-05 19:12:53 PDT
Created attachment 99779 [details]
Patch
Comment 8 Kent Tamura 2011-07-05 19:14:55 PDT
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.
Comment 9 Kentaro Hara 2011-07-05 19:17:57 PDT
Created attachment 99781 [details]
Patch
Comment 10 Kent Tamura 2011-07-05 19:25:06 PDT
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'.
Comment 11 Kentaro Hara 2011-07-05 21:12:56 PDT
Created attachment 99787 [details]
Patch
Comment 12 Kent Tamura 2011-07-05 21:39:57 PDT
Comment on attachment 99787 [details]
Patch

ok
Thank you for fixing this!
Comment 13 WebKit Review Bot 2011-07-05 23:37:12 PDT
Comment on attachment 99787 [details]
Patch

Clearing flags on attachment: 99787

Committed r90438: <http://trac.webkit.org/changeset/90438>
Comment 14 WebKit Review Bot 2011-07-05 23:37:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Kent Tamura 2011-09-07 22:33:46 PDT
*** Bug 66907 has been marked as a duplicate of this bug. ***