Bug 166712 - [Form Validation] "character" in maxlength validation message should be singular when maxlength is 1
Summary: [Form Validation] "character" in maxlength validation message should be singu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-04 20:19 PST by Chris Dumez
Modified: 2017-01-06 10:56 PST (History)
7 users (show)

See Also:


Attachments
Patch (7.58 KB, patch)
2017-01-04 20:23 PST, Chris Dumez
ap: review-
Details | Formatted Diff | Diff
WIP Patch (13.63 KB, patch)
2017-01-05 08:28 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (does not build) (14.29 KB, patch)
2017-01-05 08:45 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (builds but does not work) (13.12 KB, patch)
2017-01-05 11:07 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (17.35 KB, patch)
2017-01-05 20:37 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (450.03 KB, application/zip)
2017-01-05 22:00 PST, Build Bot
no flags Details
Patch (17.06 KB, patch)
2017-01-06 09:40 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (17.05 KB, patch)
2017-01-06 09:49 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-01-04 20:19:57 PST
"character" in maxlength validation message should be singular when maxlength is 1.
Comment 1 Chris Dumez 2017-01-04 20:20:15 PST
<rdar://problem/29872292>
Comment 2 Chris Dumez 2017-01-04 20:23:39 PST
Created attachment 298064 [details]
Patch
Comment 3 Alexey Proskuryakov 2017-01-04 21:00:26 PST
Comment on attachment 298064 [details]
Patch

Special casing 1 is incorrect for other languages. These string differences can and should be implemented in localized strings themselves. I can try to dig up the references tomorrow.
Comment 4 Darin Adler 2017-01-05 00:48:49 PST
The can be implemented in the localized strings themselves on iOS and macOS. I don’t believe that feature necessarily exists in the localization frameworks for the other platforms.
Comment 5 Chris Dumez 2017-01-05 08:28:08 PST
Created attachment 298104 [details]
WIP Patch

Attaching WIP patch using suggested approach which unfortunately does not work for some reason. Please tell me if you see what I am doing wrong.
I used the following documentation as reference:
https://developer.apple.com/library/content/documentation/MacOSX/Conceptual/BPInternational/LocalizingYourApp/LocalizingYourApp.html
Comment 6 Chris Dumez 2017-01-05 08:45:52 PST
Created attachment 298106 [details]
WIP Patch (does not build)
Comment 7 Alexey Proskuryakov 2017-01-05 10:07:56 PST
Comment on attachment 298106 [details]
WIP Patch (does not build)

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

I think that the warning needs to be silenced. Alternatively, we could add NS_FORMAT_ARGUMENT to WEB_UI_STRING, similar to how -[NSBundle localizedStringForKey: value: table:] has it.

> Source/WebCore/English.lproj/Localizable.stringsdict:8
> +		<string>use no more than %#@characters@</string>

This is a change in capitalization - did you want to have a lower case "use"?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28519
> +				ja,

Seems like an accidental change.

> Source/WebCore/ja.lproj/Localizable.stringsdict:1
> +<?xml version="1.0" encoding="UTF-8"?>

This file should not be added.

> Source/WebCore/platform/cocoa/LocalizedStringsCocoa.mm:30
> +#include <Foundation/Foundation.h>

I don't think that this change is needed. Foundation is included via WebCorePrefix.h.
Comment 8 Chris Dumez 2017-01-05 11:07:42 PST
Created attachment 298115 [details]
WIP patch (builds but does not work)
Comment 9 Chris Dumez 2017-01-05 20:37:14 PST
Created attachment 298169 [details]
Patch
Comment 10 Build Bot 2017-01-05 22:00:45 PST
Comment on attachment 298169 [details]
Patch

Attachment 298169 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2841836

Number of test failures exceeded the failure limit.
Comment 11 Build Bot 2017-01-05 22:00:49 PST
Created attachment 298174 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 12 Darin Adler 2017-01-05 22:14:55 PST
Comment on attachment 298169 [details]
Patch

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

> Source/WebCore/English.lproj/Localizable.strings:749
>  "Use no more than %d characters" = "Use no more than %d characters";

This string is not going to be used so it should not be generated, but I am not sure how.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:31939
> +			path = ..;

I don’t understand why this is needed. Localizable.strings doesn’t have it.

> Source/WebCore/platform/LocalizedStrings.cpp:1153
>      return formatLocalizedString(WEB_UI_STRING("Use no more than %d characters", "Validation message for form control elements with a value shorter than maximum allowed length"), maxLength);

Our script isn’t smart enough to know this string is not used on Cocoa platforms and so we have a localized string that will never be used.

> Source/WebCore/platform/LocalizedStrings.h:34
> +#ifdef __OBJC__
> +@class NSString;
> +#endif

Can we use our OBJC_CLASS macro instead?

> Source/WebCore/platform/LocalizedStrings.h:320
> +    WEBCORE_EXPORT NSString* localizedNString(NSString* key) NS_FORMAT_ARGUMENT(1);

I think this function should be named localizedNSString, not localizedNString.

This doesn’t follow WebKit formatting where we write NSString * instead of NSString* because this is an Objective-C class.

> Source/WebCore/platform/cocoa/LocalizedStringsCocoa.mm:36
> +NSString* localizedNString(NSString* key)

Ditto.

> Source/WebCore/platform/cocoa/LocalizedStringsCocoa.mm:38
> +    NSBundle *bundle = [NSBundle bundleWithIdentifier:@"com.apple.WebCore"];

The old code cached the bundle. This new code fetches it every time. Is that OK?

> Source/WebCore/platform/cocoa/LocalizedStringsCocoa.mm:46
>  #if !PLATFORM(IOS)
>      // Can be called on a dispatch queue when initializing strings on iOS.
>      // See LoadWebLocalizedStrings and <rdar://problem/7902473>.

I think we should move this assertion into localizedNSString.

> Source/WebCore/platform/cocoa/LocalizedStringsCocoa.mm:51
>      RetainPtr<CFStringRef> keyString = adoptCF(CFStringCreateWithCStringNoCopy(0, key, kCFStringEncodingUTF8, kCFAllocatorNull));
> -    CFStringRef notFound = CFSTR("localized string not found");
> -    RetainPtr<CFStringRef> result;
> -    if (bundle) {
> -        result = adoptCF(CFBundleCopyLocalizedString(bundle, keyString.get(), notFound, 0));
> -        ASSERT_WITH_MESSAGE(result.get() != notFound, "could not find localizable string %s in bundle", key);
> -    } else
> -        result = notFound;
> -
> -    return String(result.get());
> +    return localizedNString((NSString *)keyString.get());

I suggest we write this more simply:

    return localizedString([NSString stringWithUTF8String:key]);
Comment 13 mitz 2017-01-05 22:22:59 PST
Comment on attachment 298169 [details]
Patch

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

>> Source/WebCore/platform/cocoa/LocalizedStringsCocoa.mm:51
>> +    return localizedNString((NSString *)keyString.get());
> 
> I suggest we write this more simply:
> 
>     return localizedString([NSString stringWithUTF8String:key]);

It’s simpler, but doesn’t it involve a string copy?
Comment 14 Darin Adler 2017-01-05 22:56:09 PST
Comment on attachment 298169 [details]
Patch

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

>>> Source/WebCore/platform/cocoa/LocalizedStringsCocoa.mm:51
>>> +    return localizedNString((NSString *)keyString.get());
>> 
>> I suggest we write this more simply:
>> 
>>     return localizedString([NSString stringWithUTF8String:key]);
> 
> It’s simpler, but doesn’t it involve a string copy?

Oops, yes, I did not notice the NoCopy!
Comment 15 Chris Dumez 2017-01-06 09:32:58 PST
Comment on attachment 298169 [details]
Patch

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

>> Source/WebCore/English.lproj/Localizable.strings:749
>>  "Use no more than %d characters" = "Use no more than %d characters";
> 
> This string is not going to be used so it should not be generated, but I am not sure how.

Yes, this is unfortunate but I do not know how to fix it.

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:31939
>> +			path = ..;
> 
> I don’t understand why this is needed. Localizable.strings doesn’t have it.

I will check. This was generated by Xcode but maybe I did not do it properly in Xcode.

>> Source/WebCore/platform/LocalizedStrings.h:320
>> +    WEBCORE_EXPORT NSString* localizedNString(NSString* key) NS_FORMAT_ARGUMENT(1);
> 
> I think this function should be named localizedNSString, not localizedNString.
> 
> This doesn’t follow WebKit formatting where we write NSString * instead of NSString* because this is an Objective-C class.

Right, I renamed the macro but forgot to rename the function. I will fix this and the style.

>> Source/WebCore/platform/cocoa/LocalizedStringsCocoa.mm:36
>> +NSString* localizedNString(NSString* key)
> 
> Ditto.

Will fix.

>> Source/WebCore/platform/cocoa/LocalizedStringsCocoa.mm:38
>> +    NSBundle *bundle = [NSBundle bundleWithIdentifier:@"com.apple.WebCore"];
> 
> The old code cached the bundle. This new code fetches it every time. Is that OK?

Oh, good catch. This is an unintentional change. I'll restore the static.

>> Source/WebCore/platform/cocoa/LocalizedStringsCocoa.mm:46
>>      // See LoadWebLocalizedStrings and <rdar://problem/7902473>.
> 
> I think we should move this assertion into localizedNSString.

Will do.
Comment 16 Alexey Proskuryakov 2017-01-06 09:37:38 PST
Comment on attachment 298169 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:3021
> +		837A80131E1E127300026B9F /* Localizable.stringsdict in Resources */ = {isa = PBXBuildFile; fileRef = 837A80111E1E127300026B9F /* Localizable.stringsdict */; };

This one confuses me - are there two stringsdict files being added, one in Resources, and another (below) in English.lproj?

>> Source/WebCore/platform/LocalizedStrings.h:34
>> +#endif
> 
> Can we use our OBJC_CLASS macro instead?

I think that the forward declaration is not needed, because the macro is only expected to be used in Objective-C files (and doesn't currently work in C++ anyway).

It would probably be appropriate to move the macro under #ifdef __OBJC__.
Comment 17 Chris Dumez 2017-01-06 09:40:49 PST
Created attachment 298210 [details]
Patch
Comment 18 Chris Dumez 2017-01-06 09:45:15 PST
(In reply to comment #16)
> Comment on attachment 298169 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298169&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:3021
> > +		837A80131E1E127300026B9F /* Localizable.stringsdict in Resources */ = {isa = PBXBuildFile; fileRef = 837A80111E1E127300026B9F /* Localizable.stringsdict */; };
> 
> This one confuses me - are there two stringsdict files being added, one in
> Resources, and another (below) in English.lproj?

The project.pbxproj looks exactly the same for Localizable.stringsdict and Localizable.strings.

> >> Source/WebCore/platform/LocalizedStrings.h:34
> >> +#endif
> > 
> > Can we use our OBJC_CLASS macro instead?
> 
> I think that the forward declaration is not needed, because the macro is
> only expected to be used in Objective-C files (and doesn't currently work in
> C++ anyway).
> 
> It would probably be appropriate to move the macro under #ifdef __OBJC__.

Indeed.
Comment 19 Chris Dumez 2017-01-06 09:49:55 PST
Created attachment 298211 [details]
Patch
Comment 20 WebKit Commit Bot 2017-01-06 10:56:34 PST
Comment on attachment 298211 [details]
Patch

Clearing flags on attachment: 298211

Committed r210447: <http://trac.webkit.org/changeset/210447>
Comment 21 WebKit Commit Bot 2017-01-06 10:56:40 PST
All reviewed patches have been landed.  Closing bug.