-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LocaleRegion API #3027
base: main
Are you sure you want to change the base?
LocaleRegion API #3027
Conversation
adding draft for locale api review
specs/LocaleRegion.md
Outdated
|
||
# Description | ||
* The intended value for `LocaleRegion` is in the format of `language[-country]` where `language` is the | ||
2-letter code from [ISO 639](https://www.iso.org/iso-639-language-codes.html) and `country` is the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very specific, mentioning 2-letter codes from two standards. But BCP-47 supports a much richer set of codes (including 3-letter codes introduced in 2001). Is the precise definition here correct, or should it be relaxed to match BCP-47? (Even if you don't support all the features of BCP 47)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Peter said. This is really messed up. ECMA should be following BCP-47 tags. They should be BCP-47 tags.
"language" can have a 3 digit language, eg: tlh is a valid language. Region can be a 2 letter code (US), or a 3 number UN M49 code (029).
Additionally, that completely ignores the fact that many regions need to have a script specified. Eg: Latn or Cyrl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, on Windows (and other systems) there are locales available that are more than just "language-script-region" codes. You can have "neutrals" (no region, eg: "ja" is sufficient for ja-JP and sometimes used in place of that). You can have variants as well.
Note that Windows will endeavor to "construct" the best data possible from ANY locale tag, so don't over-validate. Any well-formed locale name is permissible. Overvalidation prohibits use of app-defined tags, forward looking tags, or tags renamed for geopolitical reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For WebView2 we are often (and in this case) creating APIs on top of the existing Edge/chromium code base. In this case these docs are attempting to describe the behavior of that existing code and this feature is not itself implementing any new validation. It may be that we're not accurately describing the existing code and we'll look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the existing Edge/chromium/ code base, we see the value gets converted to BCP-47, so I will change this to match that.
Does it make sense to add this link for reference: https://www.techonthenet.com/js/language_tags.php
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what techonthenet.com is - unless someone knows otherwise we shouldn't use that. Links to MDN, learn.microsoft.com and official standards documents are good. MDN doesn't have a document explicitly on this topic, but it does link to https://datatracker.ietf.org/doc/html/rfc5646 which appears to be good source for BCP47
specs/LocaleRegion.md
Outdated
CoreWebView2ControllerOptions ControllerOptions = null; | ||
if (LocaleRegion != null) | ||
{ | ||
ControllerOptions = environment.CreateCoreWebView2ControllerOptions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is a method needed to create the options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ControllerOptions is an existing WebView2 class not defined in this spec. An app can have multiple instances of a webview2 environment running at once and with just a constructor we wouldn't know which instance should be creating an object and so generally creating objects is done via a CoreWebView2Environment
specs/LocaleRegion.md
Outdated
|
||
```c# | ||
CoreWebView2Environment environment; | ||
CoreWebView2ControllerOptions CreateCoreWebView2ControllerOptions(CoreWebView2Environment environment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method name is a little misleading, since it doesn't just create the options, but sets the LocaleRegion
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could name it CreateAndInitializeCoreWebView2ControllerOptions
?
specs/LocaleRegion.md
Outdated
/// The CoreWebView2Environment and all associated webview2 objects will need to closed. | ||
/// | ||
/// The default value for LocaleRegion will be depend on the WebView2 language | ||
/// and OS region. If the language portions of the WebView2 language and OS Region |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the language portion... of the OS region" doesn't make sense. Region != language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The locale region format consists of the language code and country code. i.e. for "en-US", the language code is "en".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A region is a geographic area. A language is what you speak. I can speak "American English" but live in France, for example. So my language would be "en-us" but my region would be France. Windows separates these two concepts. That way, for example, I can see content in English but things like the price of goods would be in Euros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that ECMA is a bit confused on the difference between "language" and "locale", not having a great way of saying "Canadian English speaker in the US". The beginning of this spec says it's intended for date/time formats. Well, do they expect en-CA formats or en-US formats?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this differs from modern app defaults? Modern apps select the language from the language profile, and the locale from that. We already have a ridiculous number of mixed-up language/locale/region paradigms. Webview shouldn't be reinventing the wheel. Grab what we're already using.
(It's also horrible that we confuse "UI language" (meaning system apps) with "language profile", but that's out of scope here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, depending on what they want to pass in, but en-CA and en-US are both valid inputs that will update and be reflected in the Intl.DateTimeFormat().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShawnSteele Setting the LocaleRegion always to the OS region in the Control Panel would cause breaking changes. Especially in the cases where the display language is completely different than their OS region.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ptorr-msft @ShawnSteele
Is there any naming convention you had in mind to describe the value in this control panel setting that I used to describe as "OS Region"? This is what we use to compare with the WebView2 language in order to decide what the default value is.
specs/LocaleRegion.md
Outdated
/// Win32 C++: | ||
/// ```cpp | ||
/// int LanguageCodeBufferSize = | ||
/// ::GetLocaleInfoEx(LOCALE_NAME_USER_DEFAULT, LOCALE_SNAME, nullptr, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally you would use the UPLL (User Preferred Language List) APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was looking through UPLL documentation and didn't see anything to grab the OS region. This example of GetLocaleInfoEx() is what the Edge browser does when matching on OS region as part of their setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPLL lets you get the user's preferred set of languages, for example "I prefer English, but I also speak German and know a little bit of Japanese." An app that is localized only for (say) Korean or Japanese content would not want to pick English (it's meaningless) but would want to pick Japanese.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A majority of the customers' asks are regarding using the regional format found under Settings->Region in the OS, which could be completely different from the display language. For example, the language could be in English ("en-US") but they want their date and time formatting to be "13/06/2022 18:01:00" ("es-MX" format). This only affects the formatting of certain strings defined by the Intl namespace in the JS object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "right" answer here is either to use GetUserDefaultLocaleName() (the win32 answer), or the LPLL. Windows.Globalization.ApplicationLanguages.Languages is probably the right answer.
Windows.System.UserProfile.GlobalizationPreferences.Languages also has the user's preferred list. And Windows.Globalization.ApplicationLanguages.PrimaryLanguageOverride has what's been selected for that app.
I really don't like the latter as it's inconsistently used. In Win8 we meant to have a system setting for app properties that would allow users to pick their preferred language for an app, but the UX was cut. Apps really shouldn't be using that unless we get the system thing hooked up. We also totally mess up resolution for "system" apps so our LPLL behavior isn't always predictable. You have to know if it's a system or normal app.
Anyhoo, pick one and use that. Don't try to do anything further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tried using GetUserDefaultLocaleName and this also works as intended. I will update this document to reference that API instead of GetLocaleInfoEx().
specs/LocaleRegion.md
Outdated
|
||
```c# | ||
CoreWebView2Environment environment; | ||
CoreWebView2ControllerOptions CreateCoreWebView2ControllerOptions(CoreWebView2Environment environment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could name it CreateAndInitializeCoreWebView2ControllerOptions
?
specs/LocaleRegion.md
Outdated
|
||
# Description | ||
* The intended value for `LocaleRegion` is in the format of `language[-country]` where `language` is the | ||
2-letter code from [ISO 639](https://www.iso.org/iso-639-language-codes.html) and `country` is the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For WebView2 we are often (and in this case) creating APIs on top of the existing Edge/chromium code base. In this case these docs are attempting to describe the behavior of that existing code and this feature is not itself implementing any new validation. It may be that we're not accurately describing the existing code and we'll look into this.
specs/LocaleRegion.md
Outdated
# Examples | ||
```cpp | ||
auto webViewEnvironment10 = m_webViewEnvironment.try_query<ICoreWebView2Environment10>(); | ||
wil::com_ptr<ICoreWebView2ControllerOptions> options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ICoreWebView2StagingControllerOptions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API is not in stable yet, but I can update this example to reflect the code once it makes it there. @david-risney does sound this okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sorry I missed this. In our API docs we want to use the expected public names of the APIs in our sample code and in the IDL sections below - and not use the internal or experimental names.
specs/LocaleRegion.md
Outdated
CoreWebView2Environment environment; | ||
CoreWebView2ControllerOptions CreateCoreWebView2ControllerOptions(CoreWebView2Environment environment) | ||
{ | ||
CoreWebView2ControllerOptions ControllerOptions = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CoreWebView2ControllerOptions ControllerOptions = null; | |
CoreWebView2ControllerOptions controllerOptions = null; | |
specs/LocaleRegion.md
Outdated
auto webViewEnvironment10 = m_webViewEnvironment.try_query<ICoreWebView2Environment10>(); | ||
wil::com_ptr<ICoreWebView2ControllerOptions> options; | ||
HRESULT hr = webViewEnvironment10->CreateCoreWebView2ControllerOptions(&options); | ||
options->put_LocaleRegion(m_webviewOption.localeRegion.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you show the type for m_webviewOption
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a struct that saves the necessary variables and functions when creating a webview in our sample app. It might be confusing to include this all in the example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the sample code should be more self-contained so someone can tell what the type of m_webviewOption.localeRegion is. Perhaps make this and the C# sample into functions that take a string parameter and return the controlleroptions?
specs/LocaleRegion.md
Outdated
# Examples | ||
```cpp | ||
auto webViewEnvironment10 = m_webViewEnvironment.try_query<ICoreWebView2Environment10>(); | ||
wil::com_ptr<ICoreWebView2ControllerOptions> options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sorry I missed this. In our API docs we want to use the expected public names of the APIs in our sample code and in the IDL sections below - and not use the internal or experimental names.
specs/LocaleRegion.md
Outdated
|
||
# Description | ||
* The intended value for `LocaleRegion` is in the format of `language[-country]` where `language` is the | ||
2-letter code from [ISO 639](https://www.iso.org/iso-639-language-codes.html) and `country` is the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what techonthenet.com is - unless someone knows otherwise we shouldn't use that. Links to MDN, learn.microsoft.com and official standards documents are good. MDN doesn't have a document explicitly on this topic, but it does link to https://datatracker.ietf.org/doc/html/rfc5646 which appears to be good source for BCP47
specs/LocaleRegion.md
Outdated
auto webViewEnvironment10 = m_webViewEnvironment.try_query<ICoreWebView2Environment10>(); | ||
wil::com_ptr<ICoreWebView2ControllerOptions> options; | ||
HRESULT hr = webViewEnvironment10->CreateCoreWebView2ControllerOptions(&options); | ||
options->put_LocaleRegion(m_webviewOption.localeRegion.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the sample code should be more self-contained so someone can tell what the type of m_webviewOption.localeRegion is. Perhaps make this and the C# sample into functions that take a string parameter and return the controlleroptions?
specs/LocaleRegion.md
Outdated
# Description | ||
* The intended value for `LocaleRegion` is in the format of `language[-country]` where `language` is the | ||
2-letter code from [ISO 639](https://www.iso.org/iso-639-language-codes.html) and `country` is the | ||
2-letter code from [ISO 3166](https://www.iso.org/standard/72482.html). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just say "in the same format as the CoreWebView2EnvironmentOptions.Language property" (assuming that's true). That way, there's only one place you have to document language tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Assuming these are the same (I believe so but please verify) please have this reference the Language property like Raymond says. Please also validate the Language property currently has a good description of its input in its reference documentation and if not open a bug for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From our reference docs, it seems the allowed input for WebView2 language is more restrictive than what we are getting here: https://learn.microsoft.com/en-us/dotnet/api/microsoft.web.webview2.core.corewebview2environmentoptions.language?view=webview2-dotnet-1.0.1418.22
It's mentioned that it's specifically 2 letter code for language and region.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but is that correct or was it also a mistake?
specs/LocaleRegion.md
Outdated
variations in time and date formats. Currently, the locale is by default set to the same value as the | ||
display language. | ||
|
||
You can use the `LocaleRegion` API to update the locale value individually from the display |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the `LocaleRegion` API to update the locale value individually from the display | |
You can use the `LocaleRegion` property to update the locale value individually from the display | |
specs/LocaleRegion.md
Outdated
```cpp | ||
auto webViewEnvironment10 = m_webViewEnvironment.try_query<ICoreWebView2Environment10>(); | ||
wil::com_ptr<ICoreWebView2ControllerOptions> options; | ||
HRESULT hr = webViewEnvironment10->CreateCoreWebView2ControllerOptions(&options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should test result of try_query
before using webViewEnvironment10
, and also need to CHECK_RESULT
the return value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix
specs/LocaleRegion.md
Outdated
auto webViewEnvironment10 = m_webViewEnvironment.try_query<ICoreWebView2Environment10>(); | ||
wil::com_ptr<ICoreWebView2ControllerOptions> options; | ||
HRESULT hr = webViewEnvironment10->CreateCoreWebView2ControllerOptions(&options); | ||
options->put_LocaleRegion(m_webviewOption.localeRegion.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example doesn't say what it does, so I just have to guess what this code is trying to accomplish.
Is this really how you set the locale region for all WebViews connected to the environment? Create an options object, change a property, and then throw the options object away?
I think what you have to do is use this options to create a new controller (and indirectly a new WebView).
If true, then sample should show that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please fix.
specs/LocaleRegion.md
Outdated
/// std::unique_ptr<char[]> buffer(new char[LanguageCodeBufferSize]); | ||
/// WCHAR* w_language_code = new WCHAR[LanguageCodeBufferSize]; | ||
/// ::GetLocaleInfoEx(LOCALE_NAME_USER_DEFAULT, LOCALE_SNAME, w_language_code, | ||
/// LanguageCodeBufferSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This comment is mooted by change to GetUserDefaultLocaleName.
TOCTTOU bug here if the user changes locales between the two calls.
Also, LOCAL_SNAME
is documented as returning a maximum of LOCALE_NAME_MAX_LENGTH
characters, so we could have avoided the double-call (and the concomitant race condition).
specs/LocaleRegion.md
Outdated
/// 639](https://www.iso.org/iso-639-language-codes.html) and `country` is the | ||
/// 2-letter code from [ISO 3166](https://www.iso.org/standard/72482.html). | ||
/// | ||
/// This property sets the region for a CoreWebView2Environment during its creation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this sentence. The CoreWebview2Environment already exists; indeed, it's the object that creates the CoreWebView2ControllerOptions! So this property cannot affect the CoreWebview2Environment's creation, because the creation has already happened.
specs/LocaleRegion.md
Outdated
/// This property sets the region for a CoreWebView2Environment during its creation. | ||
/// Creating a new CoreWebView2Environment object that connects to an already running | ||
/// browser process cannot change the region previously set by an earlier CoreWebView2Environment. | ||
/// The CoreWebView2Environment and all associated webview2 objects will need to closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this is saying.
- If a browser process has more than one CoreWebView2Environment associated with it, then the first CoreWebView2Environment can change the region at any time to any supported value. If the second CoreWebView2Environment tries to change the region, then the call...
a. is ignored,
b. fails with an exception. - If a browser process has more than one CoreWebView2Environment associated with it, then neither can change the region.
- If a browser process has more than one CoreWebView2Environment associated with it, and then the first CoreWebView2Environment is destroyed, the second CoreWebView2Environment still cannot change the region (because only the first one can change the region).
- If a browser process has more than one CoreWebView2Environment associated with it, and then either CoreWebView2Environment is destroyed, the remaining CoreWebView2Environment can change the region (because there is now only one environment connected).
- If a browser process has more than one CoreWebView2Environment associated with it, and the second one tries to change the region and fails, then you must close the second one (and all the webview2 objects associated with the second environment) immediately.
a. Failure to do so immediately will cause unspecified harm.
b. If you do not destroy the second environment immediately, the second environment will be non-functional (all methods throw exceptions or something).
specs/LocaleRegion.md
Outdated
/// Creating a new CoreWebView2Environment object that connects to an already running | ||
/// browser process cannot change the region previously set by an earlier CoreWebView2Environment. | ||
/// The CoreWebView2Environment and all associated webview2 objects will need to closed. | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still confused about what the region is associated with. This text suggests that it's associated with the browser process. But if that's the case, why is it a property on the controller options?
specs/LocaleRegion.md
Outdated
/// | en-GB | en-US | en-GB | | ||
/// | es-MX | en-US | en-US | | ||
/// | en-US | en-GB | en-US | | ||
/// The default value can be reset using the empty string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this is saying. Is this saying, "If you set LocaleRegion to the empty string, then all the values in the "Default WebView2 LocaleRegion" column become the empty string"?
Or is this saying, "If you set LocaleRegion to the empty string, then the "Default WebView2 LocaleRegion column" column controls the region of the WebView."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is this saying, "If you set LocaleRegion to the empty string, then the "Default WebView2 LocaleRegion column" column controls the region of the WebView."
Yes, the second thing is what its saying. We could rephrase to something like "You can set the LocaleRegion to the empty string to get the default LocaleString value" or "If you set set the LocaleRegion to the empty string the default LocaleRegion value will be used"
specs/LocaleRegion.md
Outdated
auto webViewEnvironment10 = m_webViewEnvironment.try_query<ICoreWebView2Environment10>(); | ||
wil::com_ptr<ICoreWebView2ControllerOptions> options; | ||
HRESULT hr = webViewEnvironment10->CreateCoreWebView2ControllerOptions(&options); | ||
options->put_LocaleRegion(m_webviewOption.localeRegion.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please fix.
specs/LocaleRegion.md
Outdated
```cpp | ||
auto webViewEnvironment10 = m_webViewEnvironment.try_query<ICoreWebView2Environment10>(); | ||
wil::com_ptr<ICoreWebView2ControllerOptions> options; | ||
HRESULT hr = webViewEnvironment10->CreateCoreWebView2ControllerOptions(&options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix
specs/LocaleRegion.md
Outdated
[uuid(0c9a374f-20c3-4e3c-a640-67b78a7e0a48), object, pointer_default(unique)] | ||
interface ICoreWebView2StagingControllerOptions : IUnknown { | ||
/// The default region for the WebView2. It applies to JavaScript API | ||
/// Intl.DateTimeFormat() which affects string formatting like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking more at the implementation, this property is setting the DefaultLocale (https://402.ecma-international.org/1.0/#sec-6.2.4) of the JavaScript engine. Which applies to everything under the Intl namespace in JavaScript and indirectly some other things in JavaScript that were changed later to use Intl like Date.toLocaleDateString. Accordingly I don't think LocaleRegion is quite descriptive enough. Instead let's do:
- ScriptLocale
Other possibilities:
- ScriptDefaultLocale
- ScriptDefaultLanguage
I use Script because it matches other places in the API where JavaScript has been shortened to just 'Script'. I also considered 'Content' or 'Document', but that might sound like it would cover the HTTP accept-language header used to request the language of the document from the HTTP server which this does not cover (but is covered by the existing Language property IIRC)
specs/LocaleRegion.md
Outdated
# Description | ||
* The intended value for `LocaleRegion` is in the format of `language[-country]` where `language` is the | ||
2-letter code from [ISO 639](https://www.iso.org/iso-639-language-codes.html) and `country` is the | ||
2-letter code from [ISO 3166](https://www.iso.org/standard/72482.html). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Assuming these are the same (I believe so but please verify) please have this reference the Language property like Raymond says. Please also validate the Language property currently has a good description of its input in its reference documentation and if not open a bug for that.
specs/LocaleRegion.md
Outdated
[uuid(0c9a374f-20c3-4e3c-a640-67b78a7e0a48), object, pointer_default(unique)] | ||
interface ICoreWebView2StagingControllerOptions : IUnknown { | ||
/// The default region for the WebView2. It applies to JavaScript API | ||
/// Intl.DateTimeFormat() which affects string formatting like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It applies to everything under Intl correct? It would be nice to adjust this to say just that it sets the default locale for all Intl JavaScript APIs and other JavaScript APIs that depend on that like Date.toLocaleDateString. And provide a short one line example of JavaScript that would be affected like Intl.DateTimeFormat().format(new Date())
(or appropriate)
specs/LocaleRegion.md
Outdated
/// | en-GB | en-US | en-GB | | ||
/// | es-MX | en-US | en-US | | ||
/// | en-US | en-GB | en-US | | ||
/// The default value can be reset using the empty string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is this saying, "If you set LocaleRegion to the empty string, then the "Default WebView2 LocaleRegion column" column controls the region of the WebView."
Yes, the second thing is what its saying. We could rephrase to something like "You can set the LocaleRegion to the empty string to get the default LocaleString value" or "If you set set the LocaleRegion to the empty string the default LocaleRegion value will be used"
No description provided.