Bug 65008 – REGRESSION(r89535): Form reset doesn't redraw a file upload control

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

[Kent Tamura]

1. Open data:text/html,<form><input type=file><input type=reset></form>
2. Select a file for the file upload control
3. Click the reset button

Expected: The file upload control is cleared and shows "No file chosen"

Actual: The file upload control doesn't change. If the file upload control is repainted (e.g. inspect the file upload control), it shows "No file chosen".

[Kent Tamura]

Chrome 12 doesn't have this issue.

[Kentaro Hara]

I confirmed on Mac Safari that this is a regression caused by r89535 (https://bugs.webkit.org/show_bug.cgi?id=62069). I am making a patch.

[Kent Tamura]

Thank you for the investigation.

[Kentaro Hara]

Created attachment 101711 [details]
Patch

[Kent Tamura]

Comment on attachment 101711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101711&action=review

The code change looks ok.

> LayoutTests/fast/forms/file-input-reset-expected.html:16
> +<div id="console">
> +<p>
> +This tests whether the label next to the file chooser button becomes "No file chosen"
> +when we press the reset button.
> +To run this test manually, choose a file and then click the reset button.
> +If the label next to the file chooser button changes to "No file chosen",the test passes.
> +</p>

<div id=console> is not needed.

> LayoutTests/fast/forms/file-input-reset.html:10
> +<div id="console">

ditto.

> LayoutTests/fast/forms/file-input-reset.html:24
> +    var reset = document.getElementById("reset");
> +    click(reset.offsetLeft + reset.offsetWidth / 2, reset.offsetTop + reset.offsetHeight / 2);
> +    click(reset.offsetLeft + reset.offsetWidth + 10, reset.offsetTop + reset.offsetHeight + 10); // Move a cursor out of the reset button.

Why don't you use getElementById('form').reset()?

> Source/WebCore/ChangeLog:8
> +        This is a regression caused by r98535. This patch fixes the code

r98535 -> r89535.
Anyway, If you change the 1-list summary to "REGRESSION(r89535): Form reset doesn't redraw a file upload control", this sentence is not needed.

[Kent Tamura]

(In reply to comment #5)
> Anyway, If you change the 1-list summary to "REGRESSION(r89535): Form reset doesn't redraw a file upload control", this sentence is not needed.

1-list -> 1-line

[Kentaro Hara]

Created attachment 101719 [details]
Patch

[Kentaro Hara]

Comment on attachment 101711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101711&action=review

>> LayoutTests/fast/forms/file-input-reset-expected.html:16
>> +</p>
> 
> <div id=console> is not needed.

I added a selected file name here, in order to confirm that the file is really selected before being reset.

>> LayoutTests/fast/forms/file-input-reset.html:10
>> +<div id="console">
> 
> ditto.

Ditto.

>> LayoutTests/fast/forms/file-input-reset.html:24
>> +    click(reset.offsetLeft + reset.offsetWidth + 10, reset.offsetTop + reset.offsetHeight + 10); // Move a cursor out of the reset button.
> 
> Why don't you use getElementById('form').reset()?

I found that getElementById('form').reset() does not work. Even updateFromElement() is not invoked. This seems to be a regression too. Thus, I would like to commit this patch as it is, and then investigate the problem of reset() in the next patch.

>> Source/WebCore/ChangeLog:8
>> +        This is a regression caused by r98535. This patch fixes the code
> 
> r98535 -> r89535.
> Anyway, If you change the 1-list summary to "REGRESSION(r89535): Form reset doesn't redraw a file upload control", this sentence is not needed.

Done.

[Kent Tamura]

Comment on attachment 101719 [details]
Patch

Looks good.
Thank you for fixing this!

[Kentaro Hara]

Comment on attachment 101711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101711&action=review

>>> LayoutTests/fast/forms/file-input-reset.html:24
>>> +    click(reset.offsetLeft + reset.offsetWidth + 10, reset.offsetTop + reset.offsetHeight + 10); // Move a cursor out of the reset button.
>> 
>> Why don't you use getElementById('form').reset()?
> 
> I found that getElementById('form').reset() does not work. Even updateFromElement() is not invoked. This seems to be a regression too. Thus, I would like to commit this patch as it is, and then investigate the problem of reset() in the next patch.

Sorry, this is not a bug. When we write like this

<form id="form">
<input id="reset" type="reset" />
</form>

then getElementById("form").reset is overridden by the reset input element, and thus getElementById("form").reset() does not work as expected.

[WebKit Review Bot]

Comment on attachment 101719 [details]
Patch

Clearing flags on attachment: 101719

Committed r91566: <http://trac.webkit.org/changeset/91566>