Bug 49245 - <input type=file multiple /> button should say "Choose File(s)" instead of "Choose File"
: <input type=file multiple /> button should say "Choose File(s)" instead of "C...
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-11-09 03:45 PST by
Modified: 2011-07-02 03:47 PST (History)


Attachments
Patch (21.58 KB, patch)
2011-06-28 07:35 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
Patch (47.89 KB, patch)
2011-06-29 04:44 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
Patch (48.37 KB, patch)
2011-06-30 00:55 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.80 MB, application/zip)
2011-06-30 02:18 PST, WebKit Review Bot
no flags Details
Patch (48.68 KB, patch)
2011-06-30 02:18 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
Patch (50.01 KB, patch)
2011-06-30 02:40 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 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 From 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 From 2011-06-28 07:35:58 PST -------
Created an attachment (id=98917) [details]
Patch
------- Comment #4 From 2011-06-28 07:37:31 PST -------
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 From 2011-06-28 07:41:16 PST -------
(From update of attachment 98917 [details])
Attachment 98917 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8945849
------- Comment #6 From 2011-06-28 08:25:25 PST -------
(From update of attachment 98917 [details])

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 From 2011-06-28 08:42:01 PST -------
(From update of attachment 98917 [details])
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 From 2011-06-28 09:00:10 PST -------
(From update of attachment 98917 [details])
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 From 2011-06-28 09:11:50 PST -------
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 From 2011-06-28 11:39:39 PST -------
(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 From 2011-06-29 04:44:10 PST -------
Created an attachment (id=99072) [details]
Patch
------- Comment #12 From 2011-06-29 04:44:44 PST -------
(From update of attachment 98917 [details])
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 From 2011-06-29 04:47:16 PST -------
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 From 2011-06-29 04:49:55 PST -------
(From update of attachment 99072 [details])
Attachment 99072 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8957281
------- Comment #15 From 2011-06-29 18:15:56 PST -------
(From update of attachment 99072 [details])
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 From 2011-06-30 00:55:07 PST -------
Created an attachment (id=99251) [details]
Patch
------- Comment #17 From 2011-06-30 00:59:44 PST -------
(From update of attachment 99072 [details])
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 From 2011-06-30 01:54:51 PST -------
(From update of attachment 99251 [details])
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 From 2011-06-30 02:18:15 PST -------
(From update of attachment 99251 [details])
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 From 2011-06-30 02:18:22 PST -------
Created an attachment (id=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 From 2011-06-30 02:18:39 PST -------
Created an attachment (id=99265) [details]
Patch
------- Comment #22 From 2011-06-30 02:19:26 PST -------
(From update of attachment 99251 [details])
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 From 2011-06-30 02:23:29 PST -------
(From update of attachment 99072 [details])
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 From 2011-06-30 02:40:40 PST -------
Created an attachment (id=99269) [details]
Patch
------- Comment #25 From 2011-06-30 02:41:17 PST -------
(From update of attachment 99072 [details])
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 From 2011-06-30 02:54:30 PST -------
(From update of attachment 99269 [details])
ok.
Thank you for fixing this!
------- Comment #27 From 2011-06-30 03:40:46 PST -------
(From update of attachment 99269 [details])
Clearing flags on attachment: 99269

Committed r90101: <http://trac.webkit.org/changeset/90101>
------- Comment #28 From 2011-06-30 03:40:56 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #29 From 2011-07-01 23:23:41 PST -------
It seems like you need GTK rebaseline?
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r90311%20(16162)/fast/forms/input-file-re-render-pretty-diff.html
------- Comment #30 From 2011-07-02 03:47:26 PST -------
(In reply to comment #29)
> It seems like you need GTK rebaseline?

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