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
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.
(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().
Created attachment 98917 [details] Patch
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 on attachment 98917 [details] Patch Attachment 98917 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8945849
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 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 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.
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?
(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.
Created attachment 99072 [details] Patch
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.
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 on attachment 99072 [details] Patch Attachment 99072 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8957281
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.
Created attachment 99251 [details] Patch
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 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 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
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
Created attachment 99265 [details] Patch
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 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
Created attachment 99269 [details] Patch
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 on attachment 99269 [details] Patch ok. Thank you for fixing this!
Comment on attachment 99269 [details] Patch Clearing flags on attachment: 99269 Committed r90101: <http://trac.webkit.org/changeset/90101>
All reviewed patches have been landed. Closing bug.
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
(In reply to comment #29) > It seems like you need GTK rebaseline? Indeed. I have done by https://trac.webkit.org/changeset/90314