Bug 214223

Summary: Intl.Locale maximize, minimize should return Intl.Locale instead of String
Product: WebKit Reporter: wopian
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Blocker CC: andy, ews-watchlist, keith_miller, mark.lam, mmaxfield, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer, wopian, ysuzuki
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: iPhone / iPad   
OS: Other   
URL: https://6f6bm.csb.app/
See Also: https://bugs.webkit.org/show_bug.cgi?id=209772
Attachments:
Description Flags
Spec compliant output of Intl.Locale implemented properties/methods. On Edge, Chrome and Firefox
none
Output in iOS 14 Dev 2
none
Patch
ross.kirsling: review+
Patch for landing none

Description wopian 2020-07-11 10:40:10 PDT 
Created attachment 404057 [details]
Spec compliant output of Intl.Locale implemented properties/methods. On Edge, Chrome and Firefox

The functions implemented in the Intl.Locale class do not function as expected in the iOS 14 Beta 2 version of Safari. 

When no property is accessed it returns the locale using the toString function instead of a Locale instance (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Locale).

When you attempt to get a property from what should be a regular Locale instance, it instead returns undefined instead of the property's expected value.

I came across this bug as I was polyfilling Intl.Locale for browsers that didn't support Intl.Locale in a feature release and it was suddenly breaking (used maximize().region) with these undefined return values for iOS users using our staging pre-release builds.

Example:
const locale = new Intl.Locale('en')
const max = locale.maximize().baseName// undefined in iOS 14 Dev 2. Correct behaviour is 'en-Latn-US'
const min = locale.minimize().baseName// undefined in iOS 14 Dev 2. Correct behaviour is 'en'

Example 2:
const locale = new Intl.Locale('ja-Jpan-JP-u-ca-japanese-hc-h23-kf-lower-co-emoji-nu-jpan')
const max = locale.maximize() 
// Returns 'ja-Jpan-JP-u-ca-japanese-hc-h23-kf-lower-co-emoji-nu-jpan' in iOS 14 Dev 2. 
// Correct behaviour is returning a Locale object

Live Demo:
https://6f6bm.csb.app/

Attached is the spec compliant output and the iOS 14 Dev 2 output. This is identical on:
- Edge 83, 
- Edge 85 (Dev), 
- Chrome 83
- Chrome 86 (Canary)
- Firefox 78
- Firefox 79 (Developer)
Comment 1 wopian 2020-07-11 10:40:59 PDT 
Created attachment 404058 [details]
Output in iOS 14 Dev 2
Comment 2 Radar WebKit Bug Importer 2020-07-11 17:34:18 PDT 
<rdar://problem/65413620>
Comment 4 Yusuke Suzuki 2020-07-11 18:12:29 PDT 
Created attachment 404078 [details]
Patch
Comment 5 Ross Kirsling 2020-07-11 18:34:25 PDT 
Comment on attachment 404078 [details]
Patch

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

r=me, thanks!

> Source/JavaScriptCore/runtime/IntlLocale.h:56
> -    const String& maximize();
> -    const String& minimize();
> +    const String& maximal();
> +    const String& minimal();

I'm not sure that we need to change the method names here just because we're changing the field names -- toString currently returns m_fullString, say.

(Doesn't matter too much either way though since this is internal.)

> Source/JavaScriptCore/runtime/IntlLocalePrototype.cpp:141
> +    const String& fullString = locale->toString();
> +    RELEASE_AND_RETURN(scope, JSValue::encode(fullString.isEmpty() ? jsUndefined() : jsNontrivialString(vm, fullString)));

Hmm, my reasoning for not empty-checking these three is that we should have failed to construct an Intl.Locale at all if these can't be produced -- i.e. it would be ICU disagreeing *with itself* if these fail.

> JSTests/ChangeLog:9
> +        * stress/intl-locale-maximize-minimize.js: Added.

I think we should just add to the relevant part of intl-locale.js, since this is a mistake in the basic behavior of these two methods.
Comment 6 Yusuke Suzuki 2020-07-11 19:17:41 PDT 
Comment on attachment 404078 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlLocale.h:56
>> +    const String& minimal();
> 
> I'm not sure that we need to change the method names here just because we're changing the field names -- toString currently returns m_fullString, say.
> 
> (Doesn't matter too much either way though since this is internal.)

I think this is better since IntlLocale#maximize() is confusing e.g. it looks like it is implementing Intl.Locale#maximize(). But this is not correct, and it is accessing maximal field of Intl.Locale. And Intl.Locale#maximize is doing the actual thing, which is wrapping Intl.Locale.

>> Source/JavaScriptCore/runtime/IntlLocalePrototype.cpp:141
>> +    RELEASE_AND_RETURN(scope, JSValue::encode(fullString.isEmpty() ? jsUndefined() : jsNontrivialString(vm, fullString)));
> 
> Hmm, my reasoning for not empty-checking these three is that we should have failed to construct an Intl.Locale at all if these can't be produced -- i.e. it would be ICU disagreeing *with itself* if these fail.

Discussed with Ross, I think using `jsString` in these Intl.Locale prototype functions is better since this is a boundary between third-party library (ICU) and JSC, and we are not guaranteeing our invariant in ICU. So being protective would be better.

>> JSTests/ChangeLog:9
>> +        * stress/intl-locale-maximize-minimize.js: Added.
> 
> I think we should just add to the relevant part of intl-locale.js, since this is a mistake in the basic behavior of these two methods.

Moved.
Comment 7 Yusuke Suzuki 2020-07-11 19:23:34 PDT 
Created attachment 404082 [details]
Patch for landing
Comment 8 EWS 2020-07-11 21:49:05 PDT 
Committed r264275: <https://trac.webkit.org/changeset/264275>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404082 [details].