Bug 49245 - <input type=file multiple /> button should say "Choose File(s)" instead of "Choose File"
Summary: <input type=file multiple /> button should say "Choose File(s)" instead of "C...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-09 03:45 PST by Dai Mikurube
Modified: 2011-07-02 03:47 PDT (History)
10 users (show)

See Also:


Attachments
Patch (21.58 KB, patch)
2011-06-28 07:35 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (47.89 KB, patch)
2011-06-29 04:44 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (48.37 KB, patch)
2011-06-30 00:55 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.80 MB, application/zip)
2011-06-30 02:18 PDT, WebKit Review Bot
no flags Details
Patch (48.68 KB, patch)
2011-06-30 02:18 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (50.01 KB, patch)
2011-06-30 02:40 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 Dai Mikurube 2010-11-09 03:45:29 PST
The caption of a HTML5 file chooser for multiple files should be different from that of a file chooser for a maximum of 1 file.
http://code.google.com/p/chromium/issues/detail?id=50301
Comment 1 Alexey Proskuryakov 2010-11-09 17:03:37 PST
It certainly shouldn't say it "Choose File(s)" though. That is not proper language.

We should also consider localization impact, even though localizations for most ports are not in webkit.org.
Comment 2 Kent Tamura 2010-11-09 17:17:20 PST
(In reply to comment #1)
> It certainly shouldn't say it "Choose File(s)" though. That is not proper language.

I also don't like "File(s)".  Probably, "Choose one or more files"?

> We should also consider localization impact, even though localizations for most ports are not in webkit.org.

If we added fileButtonChooseMultipleFilesLabel() to LocalizedStrings.h, its implementation for unready port would be just calling existing fileButtonChooseFileLabel().
Comment 3 Kentaro Hara 2011-06-28 07:35:58 PDT
Created attachment 98917 [details]
Patch
Comment 4 Kentaro Hara 2011-06-28 07:37:31 PDT
I inserted a tricky macro WE_WILL_REMOVE_THIS_MACRO_SOON in WebKit/Source/WebKit/chromium/public/WebLocalizedString.h. Please see http://codereview.chromium.org/7273024/ for more details.
Comment 5 WebKit Review Bot 2011-06-28 07:41:16 PDT
Comment on attachment 98917 [details]
Patch

Attachment 98917 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8945849
Comment 6 Kent Tamura 2011-06-28 08:25:25 PDT
Comment on attachment 98917 [details]
Patch


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

r- because of Chromium build failure.


> LayoutTests/ChangeLog:5
> +        Change a label of the HTML5 file chooser button to "Choose One or More Files"

Do you have other ideas of shorter label?
"Choose One or More Files" looks too long, and makes the file name area very short.

> LayoutTests/fast/forms/input-file-label.html:15
> +function print(s) {
> +    document.getElementById('console').textContent += s + '\n';
> +}

You don't need this function.  You can use debug(), which is defined in js-test-pre.js.

> LayoutTests/fast/forms/input-file-label.html:18
> +    layoutTestController.dumpAsText();

You don't need to call dumpAsText(). js-test-pre.js already called it.

> LayoutTests/fast/forms/input-file-label.html:23
> +    print((label == 'Choose File' ? 'PASS' : 'FAIL') + ': The label of a single file chooser was "' + label + '".');

You can use testPassed() and testFailed() defined in js-test-pre.js.

> LayoutTests/fast/forms/input-file-label.html:28
> +    print((label == 'Choose One or More Files' ? 'PASS' : 'FAIL') + ': The label of a multiple file chooser was "' + label + '".');

ditto.

> Source/WebCore/html/FileInputType.cpp:49
> +    static PassRefPtr<UploadButtonElement> create(Document*, bool multiple);

bool parameter is not good.
 - Define an enum, or
 - Introduce another factory method, like "createForMultiple()"

> Source/WebKit/chromium/public/WebLocalizedString.h:47
> +// This macro is a temporal macro to resolve the timing dependency
> +// between the following Chromium patch and WebKit patch:
> +// Chromium patch: http://codereview.chromium.org/7273024/
> +// WebKit patch: https://bugs.webkit.org/show_bug.cgi?id=49245
> +//
> +// This macro guarantees that
> +// WebLocalizedString::FileButtonChooseMultipleFilesLabel exists
> +// in WebKit if and only if
> +// WebLocalizedString::FileButtonChooseMultipleFilesLabel exists
> +// in Chromium.
> +// See http://codereview.chromium.org/7273024/ for more details.
> +#define WE_WILL_REMOVE_THIS_MACRO_SOON

See my comment for the Chromium change.
Comment 7 Dimitri Glazkov (Google) 2011-06-28 08:42:01 PDT
Comment on attachment 98917 [details]
Patch

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

>> Source/WebCore/html/FileInputType.cpp:49
>> +    static PassRefPtr<UploadButtonElement> create(Document*, bool multiple);
> 
> bool parameter is not good.
>  - Define an enum, or
>  - Introduce another factory method, like "createForMultiple()"

It's not a construction-time concept, right? This value may change during the button's lifetime. Should we instead change the value as the attribute is added/removed?
Comment 8 Alexey Proskuryakov 2011-06-28 09:00:10 PDT
Comment on attachment 98917 [details]
Patch

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

> Source/WebCore/html/FileInputType.cpp:61
> +    multiple ? button->setValue(fileButtonChooseMultipleFilesLabel()) : button->setValue(fileButtonChooseFileLabel());

The ternary operator should be inside setValue() for clarity.
Comment 9 Kentaro Hara 2011-06-28 09:11:50 PDT
Thank you very much for the dedicated reviews.

> Do you have other ideas of shorter label?
> "Choose One or More Files" looks too long, and makes the file name area very short.

I would like to clarify this first (since I need to write the label here and there). How about "Choose Files"? Is it misleading?
Comment 10 Tony Chang 2011-06-28 11:39:39 PDT
(In reply to comment #9)
> Thank you very much for the dedicated reviews.
> 
> > Do you have other ideas of shorter label?
> > "Choose One or More Files" looks too long, and makes the file name area very short.
> 
> I would like to clarify this first (since I need to write the label here and there). How about "Choose Files"? Is it misleading?

"Choose Files" is not great, but it seems better than the alternatives.  I don't think it's misleading.
Comment 11 Kentaro Hara 2011-06-29 04:44:10 PDT
Created attachment 99072 [details]
Patch
Comment 12 Kentaro Hara 2011-06-29 04:44:44 PDT
Comment on attachment 98917 [details]
Patch

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

>> LayoutTests/ChangeLog:5
>> +        Change a label of the HTML5 file chooser button to "Choose One or More Files"
> 
> Do you have other ideas of shorter label?
> "Choose One or More Files" looks too long, and makes the file name area very short.

I changed it to "Choose Files".

>> LayoutTests/fast/forms/input-file-label.html:15
>> +}
> 
> You don't need this function.  You can use debug(), which is defined in js-test-pre.js.

Done.

>> LayoutTests/fast/forms/input-file-label.html:18
>> +    layoutTestController.dumpAsText();
> 
> You don't need to call dumpAsText(). js-test-pre.js already called it.

Done.

>> LayoutTests/fast/forms/input-file-label.html:23
>> +    print((label == 'Choose File' ? 'PASS' : 'FAIL') + ': The label of a single file chooser was "' + label + '".');
> 
> You can use testPassed() and testFailed() defined in js-test-pre.js.

Done.

>> LayoutTests/fast/forms/input-file-label.html:28
>> +    print((label == 'Choose One or More Files' ? 'PASS' : 'FAIL') + ': The label of a multiple file chooser was "' + label + '".');
> 
> ditto.

Done.

>>> Source/WebCore/html/FileInputType.cpp:49
>>> +    static PassRefPtr<UploadButtonElement> create(Document*, bool multiple);
>> 
>> bool parameter is not good.
>>  - Define an enum, or
>>  - Introduce another factory method, like "createForMultiple()"
> 
> It's not a construction-time concept, right? This value may change during the button's lifetime. Should we instead change the value as the attribute is added/removed?

Kent: Introduced createForMultiple().

Dimitri: I added destroyShadowSubtree() and createShadowSubtree() in HTMLInputElement::parseMappedAttribute(), so that the shadow tree is re-constructed whenever 'multiple' attribute is changed. I also added its test in input-file-label.html.

>> Source/WebCore/html/FileInputType.cpp:61
>> +    multiple ? button->setValue(fileButtonChooseMultipleFilesLabel()) : button->setValue(fileButtonChooseFileLabel());
> 
> The ternary operator should be inside setValue() for clarity.

Done.

>> Source/WebKit/chromium/public/WebLocalizedString.h:47
>> +#define WE_WILL_REMOVE_THIS_MACRO_SOON
> 
> See my comment for the Chromium change.

Done. Removed this tricky macro.
Comment 13 Kentaro Hara 2011-06-29 04:47:16 PDT
We cannot see the change of Localizable.strings in the review page since the file is recognized as a binary file. Actually, the change is just adding the following two lines at line 73:

/* title for file button used in HTML5 file chooser */                                                            
"Choose Files" = "Choose Files";
Comment 14 WebKit Review Bot 2011-06-29 04:49:55 PDT
Comment on attachment 99072 [details]
Patch

Attachment 99072 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8957281
Comment 15 Kent Tamura 2011-06-29 18:15:56 PDT
Comment on attachment 99072 [details]
Patch

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

> LayoutTests/platform/chromium/test_expectations.txt:227
> +BUGWK49245 SKIP : fast/forms/input-file-label.html = FAIL
> +
> +// This test fails because the label of a multiple file chooser button is updated.
> +// We need to update expected.txt and expected.png.
> +BUGWK49245 SKIP : fast/forms/input-file-re-render.html = IMAGE+TEXT FAIL

Do not SKIP them.  We should run failing tests unless they are very slow because we'd like to confirm they cause no crashes and let buildbots generate test results.

> LayoutTests/platform/gtk/Skipped:1538
> +# This test fails because the label of a multiple file chooser button is updated.
> +# We need to update expected.txt and expected.png.
> +fast/forms/input-file-re-render.html

I don't think we should skip a test which needs rebaseline.
If we skip it, we can't get test results from buildbots and one who has no failing platforms can't correct the expectations.

> Source/WebCore/ChangeLog:9
> +        Changed a label of the HTML5 file chooser button to "Choose Files",
> +        since it enables us to choose multiple files.

We don't need to repeat the summary sentence.

"since ..." is unclear.  We should write reasons/benefit of the change.
"We should notify capability of multiple files to users."?

> Source/WebCore/ChangeLog:36
> +        * English.lproj/Localizable.strings:
> +        * html/FileInputType.cpp:
> +        (WebCore::UploadButtonElement::create):
> +        (WebCore::UploadButtonElement::createForMultiple):
> +        (WebCore::FileInputType::createShadowSubtree):
> +        * html/HTMLInputElement.cpp:
> +        (WebCore::HTMLInputElement::parseMappedAttribute):
> +        * platform/DefaultLocalizationStrategy.cpp:
> +        (WebCore::DefaultLocalizationStrategy::fileButtonChooseMultipleFilesLabel):
> +        * platform/DefaultLocalizationStrategy.h:
> +        * platform/LocalizationStrategy.h:
> +        * platform/LocalizedStrings.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):
> +        * platform/LocalizedStrings.h:
> +        * platform/brew/LocalizedStringsBrew.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):
> +        * platform/efl/LocalizedStringsEfl.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):
> +        * platform/gtk/LocalizedStringsGtk.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):
> +        * platform/haiku/LocalizedStringsHaiku.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):
> +        * platform/wx/LocalizedStringsWx.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):

You had better write summaries of changes fo each of files/functions.

> Source/WebCore/html/HTMLInputElement.cpp:734
> +    } else if (attr->name() == multipleAttr) {
> +        m_inputType->destroyShadowSubtree();
> +        m_inputType->createShadowSubtree();
> +        setNeedsValidityCheck();

Type=email doesn't need to destroy and create the shadow tree for 'multiple' attribute change. This code is inefficient.
So, I recommend you introduce a member function for InputType, and override it in FileInputType.

> Source/WebCore/platform/DefaultLocalizationStrategy.cpp:131
> +    return WEB_UI_STRING("Choose Files", "title for file button used in HTML5 file chooser");

The description string should mention:
 - The message is for multiple files.
 - The message should be short as possible.

> Source/WebKit/chromium/ChangeLog:15
> +        * public/WebLocalizedString.h:
> +        * src/LocalizedStrings.cpp:
> +        (WebCore::fileButtonChooseMultipleFilesLabel):

You need to update WebKit/chromium/DEPS so that chromium_rev >= 91051.

> Source/WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp:177
> +    return QCoreApplication::translate("QWebPage", "Choose Files", "title for file button used in HTML5 file chooser");

ditto for the description.

> Source/WebKit/wince/WebCoreSupport/PlatformStrategiesWinCE.cpp:144
> +    return UI_STRING("Choose Files", "title for file button used in HTML5 file chooser");

ditto.
Comment 16 Kentaro Hara 2011-06-30 00:55:07 PDT
Created attachment 99251 [details]
Patch
Comment 17 Kentaro Hara 2011-06-30 00:59:44 PDT
Comment on attachment 99072 [details]
Patch

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

>> LayoutTests/platform/chromium/test_expectations.txt:227
>> +BUGWK49245 SKIP : fast/forms/input-file-re-render.html = IMAGE+TEXT FAIL
> 
> Do not SKIP them.  We should run failing tests unless they are very slow because we'd like to confirm they cause no crashes and let buildbots generate test results.

OK. Removed this SKIP. input-file-re-render.html will fail on Chromium, gtk and MacLeopard, and input-file-label.html will fail on Chromium. After this patch lands, I will upload a patch to add the expected images generated by buildbots for gtk and MacLeopard. Then, after our planned Chromium patch landed, I will also upload a patch to add the expected images and texts for Chromium.

>> LayoutTests/platform/gtk/Skipped:1538
>> +fast/forms/input-file-re-render.html
> 
> I don't think we should skip a test which needs rebaseline.
> If we skip it, we can't get test results from buildbots and one who has no failing platforms can't correct the expectations.

Done.

>> Source/WebCore/ChangeLog:9
>> +        since it enables us to choose multiple files.
> 
> We don't need to repeat the summary sentence.
> 
> "since ..." is unclear.  We should write reasons/benefit of the change.
> "We should notify capability of multiple files to users."?

Done.

>> Source/WebCore/ChangeLog:36
>> +        (WebCore::fileButtonChooseMultipleFilesLabel):
> 
> You had better write summaries of changes fo each of files/functions.

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:734
>> +        setNeedsValidityCheck();
> 
> Type=email doesn't need to destroy and create the shadow tree for 'multiple' attribute change. This code is inefficient.
> So, I recommend you introduce a member function for InputType, and override it in FileInputType.

I introduced InputType::multipleAttributeChanged() and FileInputType::multipleAttributeChanged(), which changes only the label of the file chooser button.

>> Source/WebCore/platform/DefaultLocalizationStrategy.cpp:131
>> +    return WEB_UI_STRING("Choose Files", "title for file button used in HTML5 file chooser");
> 
> The description string should mention:
>  - The message is for multiple files.
>  - The message should be short as possible.

Done.

>> Source/WebKit/chromium/ChangeLog:15
>> +        (WebCore::fileButtonChooseMultipleFilesLabel):
> 
> You need to update WebKit/chromium/DEPS so that chromium_rev >= 91051.

Done.

>> Source/WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp:177
>> +    return QCoreApplication::translate("QWebPage", "Choose Files", "title for file button used in HTML5 file chooser");
> 
> ditto for the description.

Done.

>> Source/WebKit/wince/WebCoreSupport/PlatformStrategiesWinCE.cpp:144
>> +    return UI_STRING("Choose Files", "title for file button used in HTML5 file chooser");
> 
> ditto.

Done.
Comment 18 Kent Tamura 2011-06-30 01:54:51 PDT
Comment on attachment 99251 [details]
Patch

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

> Source/WebCore/html/FileInputType.cpp:266
> +        button->setValue(element()->multiple() ? fileButtonChooseMultipleFilesLabel() : fileButtonChooseFileLabel());

This is good!

> Source/WebCore/platform/DefaultLocalizationStrategy.cpp:126
> -    return WEB_UI_STRING("Choose File", "title for file button used in HTML forms");
> +    return WEB_UI_STRING("Choose File", "title for a single file chooser button");

Do not remove "used in HTML forms"

> Source/WebCore/platform/DefaultLocalizationStrategy.cpp:131
> +    return WEB_UI_STRING("Choose Files", "title for a multiple file chooser button used in HTML5. This title should be as short as possible.");

Using a specific version of HTML is unnecessary.
"used in HTML5" -> "used in HTML forms"

> Source/WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp:177
> +    return QCoreApplication::translate("QWebPage", "Choose Files", "title for a multiple file chooser button used in HTML5. This title should be as short as possible.");

ditto.

> Source/WebKit/wince/WebCoreSupport/PlatformStrategiesWinCE.cpp:144
> +    return UI_STRING("Choose Files", "title for a multiple file chooser button used in HTML5. This title should be as short as possible.");

ditto.
Comment 19 WebKit Review Bot 2011-06-30 02:18:15 PDT
Comment on attachment 99251 [details]
Patch

Attachment 99251 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8957661

New failing tests:
fast/forms/input-file-re-render.html
fast/forms/input-file-label.html
Comment 20 WebKit Review Bot 2011-06-30 02:18:22 PDT
Created attachment 99264 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 21 Kentaro Hara 2011-06-30 02:18:39 PDT
Created attachment 99265 [details]
Patch
Comment 22 Kentaro Hara 2011-06-30 02:19:26 PDT
Comment on attachment 99251 [details]
Patch

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

>> Source/WebCore/platform/DefaultLocalizationStrategy.cpp:126
>> +    return WEB_UI_STRING("Choose File", "title for a single file chooser button");
> 
> Do not remove "used in HTML forms"

Done.

>> Source/WebCore/platform/DefaultLocalizationStrategy.cpp:131
>> +    return WEB_UI_STRING("Choose Files", "title for a multiple file chooser button used in HTML5. This title should be as short as possible.");
> 
> Using a specific version of HTML is unnecessary.
> "used in HTML5" -> "used in HTML forms"

Done.

>> Source/WebKit/qt/WebCoreSupport/WebPlatformStrategies.cpp:177
>> +    return QCoreApplication::translate("QWebPage", "Choose Files", "title for a multiple file chooser button used in HTML5. This title should be as short as possible.");
> 
> ditto.

Done.

>> Source/WebKit/wince/WebCoreSupport/PlatformStrategiesWinCE.cpp:144
>> +    return UI_STRING("Choose Files", "title for a multiple file chooser button used in HTML5. This title should be as short as possible.");
> 
> ditto.

Done.
Comment 23 Kent Tamura 2011-06-30 02:23:29 PDT
Comment on attachment 99072 [details]
Patch

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

>>> LayoutTests/platform/chromium/test_expectations.txt:227
>>> +BUGWK49245 SKIP : fast/forms/input-file-re-render.html = IMAGE+TEXT FAIL
>> 
>> Do not SKIP them.  We should run failing tests unless they are very slow because we'd like to confirm they cause no crashes and let buildbots generate test results.
> 
> OK. Removed this SKIP. input-file-re-render.html will fail on Chromium, gtk and MacLeopard, and input-file-label.html will fail on Chromium. After this patch lands, I will upload a patch to add the expected images generated by buildbots for gtk and MacLeopard. Then, after our planned Chromium patch landed, I will also upload a patch to add the expected images and texts for Chromium.

We should not skip them, but you need to write failure expectations for them.

BUGWK49245 : fast/forms/input-file-label.html = FAIL
BUGWK49245 : fast/forms/input-file-re-render.html = FAIL
Comment 24 Kentaro Hara 2011-06-30 02:40:40 PDT
Created attachment 99269 [details]
Patch
Comment 25 Kentaro Hara 2011-06-30 02:41:17 PDT
Comment on attachment 99072 [details]
Patch

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

>>>> LayoutTests/platform/chromium/test_expectations.txt:227
>>>> +BUGWK49245 SKIP : fast/forms/input-file-re-render.html = IMAGE+TEXT FAIL
>>> 
>>> Do not SKIP them.  We should run failing tests unless they are very slow because we'd like to confirm they cause no crashes and let buildbots generate test results.
>> 
>> OK. Removed this SKIP. input-file-re-render.html will fail on Chromium, gtk and MacLeopard, and input-file-label.html will fail on Chromium. After this patch lands, I will upload a patch to add the expected images generated by buildbots for gtk and MacLeopard. Then, after our planned Chromium patch landed, I will also upload a patch to add the expected images and texts for Chromium.
> 
> We should not skip them, but you need to write failure expectations for them.
> 
> BUGWK49245 : fast/forms/input-file-label.html = FAIL
> BUGWK49245 : fast/forms/input-file-re-render.html = FAIL

Done.
Comment 26 Kent Tamura 2011-06-30 02:54:30 PDT
Comment on attachment 99269 [details]
Patch

ok.
Thank you for fixing this!
Comment 27 WebKit Review Bot 2011-06-30 03:40:46 PDT
Comment on attachment 99269 [details]
Patch

Clearing flags on attachment: 99269

Committed r90101: <http://trac.webkit.org/changeset/90101>
Comment 28 WebKit Review Bot 2011-06-30 03:40:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Kent Tamura 2011-07-02 03:47:26 PDT
(In reply to comment #29)
> It seems like you need GTK rebaseline?

Indeed.
I have done by https://trac.webkit.org/changeset/90314