Bug 67567 - REGRESSION (Safari 5.1 - ToT): File input retains its file icon when the value is reset
Summary: REGRESSION (Safari 5.1 - ToT): File input retains its file icon when the valu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.7
: P1 Normal
Assignee: Kent Tamura
URL: http://felixcloutier.com/documents/we...
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2011-09-03 11:29 PDT by Félix Cloutier
Modified: 2011-09-06 22:37 PDT (History)
4 users (show)

See Also:


Attachments
An HTML page that demonstrates the problem (597 bytes, text/html)
2011-09-03 11:35 PDT, Félix Cloutier
no flags Details
Patch (15.14 KB, patch)
2011-09-05 23:28 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (15.10 KB, patch)
2011-09-05 23:30 PDT, Kent Tamura
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Félix Cloutier 2011-09-03 11:29:27 PDT
Empty file inputs visually consist of a button and the text 'no file selected'. File inputs with a value, instead, have a button, a file icon, and a file label.

However, when an <input type="file"> is reset through Javascript (input.value = ''), the file label reverts to 'no file selected', but the file icon stays there.
Comment 1 Félix Cloutier 2011-09-03 11:35:11 PDT
Created attachment 106265 [details]
An HTML page that demonstrates the problem
Comment 2 Dimitri Glazkov (Google) 2011-09-05 08:31:21 PDT
How recent is this? Can you try bisecting. I can't reproduce on Chromium using WebKit @94469.
Comment 3 Félix Cloutier 2011-09-05 11:14:50 PDT
It doesn't happen with the version of Safari that ships with Mac OS Lion (5.1, 7534.48.3), but it happens with r94508, again using Safari. I'm sorry, I can't really be any more precise.
Comment 4 Kent Tamura 2011-09-05 19:28:39 PDT
It seems no one clears FileInputType::m_icon.
This issue doesn't affect Chromium because Chromium doesn't support icons for file upload controls.
Comment 5 Kent Tamura 2011-09-05 23:28:54 PDT
Created attachment 106391 [details]
Patch
Comment 6 Kent Tamura 2011-09-05 23:30:06 PDT
Created attachment 106392 [details]
Patch 2
Comment 7 Dimitri Glazkov (Google) 2011-09-06 08:46:20 PDT
Comment on attachment 106392 [details]
Patch 2

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

Thanks for fixing this -- and the fix is right. I have a small naming nit.

> Source/WebCore/html/HTMLInputElement.cpp:1127
> +void HTMLInputElement::setValueInternal(const String& sanitizedValue, bool sendChangeEvent)

This should be something like valueChanged.
Comment 8 Kent Tamura 2011-09-06 22:32:59 PDT
Committed r94639: <http://trac.webkit.org/changeset/94639>
Comment 9 Kent Tamura 2011-09-06 22:37:22 PDT
Comment on attachment 106392 [details]
Patch 2

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

>> Source/WebCore/html/HTMLInputElement.cpp:1127
>> +void HTMLInputElement::setValueInternal(const String& sanitizedValue, bool sendChangeEvent)
> 
> This should be something like valueChanged.

I don't agree with it because
- The value is changed in this function. "valueChanged" sounds like "didChangeValue".
- InputType already has valueChanged(), of which purpose is different from this.

So, I landed this without changes.