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)
Created attachment 404058 [details] Output in iOS 14 Dev 2
<rdar://problem/65413620>
Relevant sections of the specification: https://tc39.es/ecma402/#sec-Intl.Locale.prototype.maximize https://tc39.es/ecma402/#sec-Intl.Locale.prototype.minimize
Created attachment 404078 [details] Patch
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 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.
Created attachment 404082 [details] Patch for landing
Committed r264275: <https://trac.webkit.org/changeset/264275> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404082 [details].