Bug 214223 - Intl.Locale maximize, minimize should return Intl.Locale instead of String
Summary: Intl.Locale maximize, minimize should return Intl.Locale instead of String
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari Technology Preview
Hardware: iPhone / iPad Other
: P2 Blocker
Assignee: Yusuke Suzuki
URL: https://6f6bm.csb.app/
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-11 10:40 PDT by wopian
Modified: 2020-07-11 21:49 PDT (History)
12 users (show)

See Also:


Attachments
Spec compliant output of Intl.Locale implemented properties/methods. On Edge, Chrome and Firefox (22.11 KB, image/png)
2020-07-11 10:40 PDT, wopian
no flags Details
Output in iOS 14 Dev 2 (57.11 KB, image/png)
2020-07-11 10:40 PDT, wopian
no flags Details
Patch (9.60 KB, patch)
2020-07-11 18:12 PDT, Yusuke Suzuki
ross.kirsling: review+
Details | Formatted Diff | Diff
Patch for landing (16.28 KB, patch)
2020-07-11 19:23 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].