Bug 49245 – <input type=file multiple /> button should say "Choose File(s)" instead of "Choose File"

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dai Mikurube]

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

[Alexey Proskuryakov]

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.

[Kent Tamura]

(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().

[Kentaro Hara]

Created attachment 98917 [details]
Patch

[Kentaro Hara]

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.

[WebKit Review Bot]

Comment on attachment 98917 [details]
Patch

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

[Kent Tamura]

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.

[Dimitri Glazkov (Google)]

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?

[Alexey Proskuryakov]

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.

[Kentaro Hara]

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?

[Tony Chang]

(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.

[Kentaro Hara]

Created attachment 99072 [details]
Patch

[Kentaro Hara]

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.

[Kentaro Hara]

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";

[WebKit Review Bot]

Comment on attachment 99072 [details]
Patch

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

[Kent Tamura]

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.

[Kentaro Hara]

Created attachment 99251 [details]
Patch

[Kentaro Hara]

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.

[Kent Tamura]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kentaro Hara]

Created attachment 99265 [details]
Patch

[Kentaro Hara]

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.

[Kent Tamura]

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

[Kentaro Hara]

Created attachment 99269 [details]
Patch

[Kentaro Hara]

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.

[Kent Tamura]

Comment on attachment 99269 [details]
Patch

ok.
Thank you for fixing this!

[WebKit Review Bot]

Comment on attachment 99269 [details]
Patch

Clearing flags on attachment: 99269

Committed r90101: <http://trac.webkit.org/changeset/90101>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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

[Kent Tamura]

(In reply to comment #29)
> It seems like you need GTK rebaseline?

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