Bug 166712 – [Form Validation] "character" in maxlength validation message should be singular when maxlength is 1

• Component: Forms
• Status: RESOLVED FIXED

[Chris Dumez]

"character" in maxlength validation message should be singular when maxlength is 1.

[Chris Dumez]

<rdar://problem/29872292>

[Chris Dumez]

Created attachment 298064 [details]
Patch

[Alexey Proskuryakov]

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.

[Darin Adler]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 298106 [details]
WIP Patch (does not build)

[Alexey Proskuryakov]

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.

[Chris Dumez]

Created attachment 298115 [details]
WIP patch (builds but does not work)

[Chris Dumez]

Created attachment 298169 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Darin Adler]

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 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?

[Darin Adler]

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!

[Chris Dumez]

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.

[Alexey Proskuryakov]

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

[Chris Dumez]

Created attachment 298210 [details]
Patch

[Chris Dumez]

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

[Chris Dumez]

Created attachment 298211 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 298211 [details]
Patch

Clearing flags on attachment: 298211

Committed r210447: <http://trac.webkit.org/changeset/210447>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.