Skip to content
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

Set user language from HTTP Accept-Language #8751

Merged
merged 4 commits into from Jun 25, 2019

Conversation

Projects
None yet
3 participants
@Beuc
Copy link
Contributor

commented Jun 6, 2019

This allows users to start the app/game with LANG set to their preferred language, avoiding the need to explicitly change it in the app's GUI, POSIX-style.
e.g.

Accept-Language: fr,fr-FR;q=0.8,en-US;q=0.5,en;q=0.3
> navigator.languages = Array(4) [ "fr", "fr-FR", "en-US", "en" ]
> LANG=fr.UTF-8
@@ -877,6 +877,9 @@ LibraryManager.library = {
ENV['PWD'] = '/';
ENV['HOME'] = '/home/web_user';
ENV['LANG'] = 'C.UTF-8';
if (navigator.languages != 'undefined' && navigator.languages.length > 0) {
ENV['LANG'] = navigator.languages[0].replace('-', '_') + '.UTF-8';
}

This comment has been minimized.

Copy link
@kripken

kripken Jun 7, 2019

Member

This change seems a little risky to me - maybe some language has - in other places? Also, the app may behave differently for different users, which is the point I guess :) but it may be surprising to start doing so now.

This comment has been minimized.

Copy link
@juj

juj Jun 16, 2019

Collaborator

To improve Closure minifiability, one could do:

ENV['LANG'] = (typeof navigator === 'object' && navigator.languages && navigator.languages[0] || 'C').replace('-', '_') + '.UTF-8';

(I would assume that if navigator.languages exists, it is an array)

@Beuc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

the app may behave differently for different users, which is the point I guess :) but it may be surprising to start doing so now.

Here, I wish to pass the language information from the browser to the application, which is currently lacking, so that i18n can be initiated.
I added a note in ChangeLog.md to describe the new behavior.
I understand this may cause the application's behavior to differ (note: it needs to call setlocale() to experience any effect), though I believe this new behavior is better for portability.
I also appreciate that this is a low-level change, and I'm open to other implementation ideas. Do you want me to start a thread on the mailing list?

This change seems a little risky to me - maybe some language has '-' in other places?

According to the list of languages in Firefox > Preferences > Languages, no (only xxx or xxx-XXX codes).
According to RFC7231 pseudo-languages like x-pig-latin could theoretically happen, in which case this will result in proposing invalid locale x_pig-latin, which a traditional C program would just ignore.
MDN also references spelling variants like de-DE-1996 which are "usually not used in the context of this header" and would result in de_DE-1996 which I believe is correct.

@kripken

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Interesting, thanks.

Overall I feel I don't know enough about this to say what's right. I especially worry since the MDN page on this feature says it is experimental. So, yeah, maybe asking on the mailing list is a good idea. If there are no strong feelings against it then this seems ok to me, the risk is probably low enough.

Btw, why would this change only be noticeable by calling setlocale? It seems like anything that looks at that environment value could be affected? (which I agree is probably very few things though)

@@ -877,6 +877,12 @@ LibraryManager.library = {
ENV['PWD'] = '/';
ENV['HOME'] = '/home/web_user';
ENV['LANG'] = 'C.UTF-8';
if (typeof navigator === 'object' && typeof navigator.languages === 'object' && navigator.languages.length > 0) {
ENV['LANG'] = navigator.languages[0].replace('-', '_') + '.UTF-8';

This comment has been minimized.

Copy link
@kripken

kripken Jun 11, 2019

Member

(should be 2-space indentation)

@Beuc Beuc force-pushed the Beuc:patch-5 branch from 42a440b to d21786b Jun 11, 2019

@Beuc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

the MDN page on this feature says it is experimental

navigator.language (without 's') is old/"standard" but may still return the browser's internal language pack, rather than the requested webpage languages from the user's preferences.
navigator.languageS, which this patch relies on, correctly returns HTTP Accept-Language, is more recent and is supported by all browsers that support WebAssembly AFAICS. For older browsers, no changes.

Btw, why would this change only be noticeable by calling setlocale?

I meant that standard C library functions like scanf(3) are affected only when calling setlocale(3).
If the program directly checks the environment variable then indeed there's a change :)

@Beuc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@Beuc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

Following the discussion on the mailing list:

  • nobody complained so I believe people are generally fine with this PR
  • I gave 2 direct use cases with gettext and python.locale

Surprisingly, nobody suggested we did something for nodejs, where I believe it'd make more sense to inherit LANG from the system environment - but since all environment variables are reset that sorta makes sense too.

What do we do with this PR ? :)

@juj

This comment has been minimized.

Copy link
Collaborator

commented Jun 16, 2019

This PR looks good to me - it does bridge POSIX language feature with browser API in a straightforward expected way, and developers do have a way to enforce no locale from both C (setlocale(...)) and JS (ENV['LANG'] = ....

@kripken

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

I agree, after the discussion I think this is a good approach.

Please just add a test before we land.

@Beuc Beuc force-pushed the Beuc:patch-5 branch from d21786b to 4820689 Jun 23, 2019

@Beuc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

I got some time at last and implemented the test case!
(I mocked navigator.languages, as I didn't see any way to set custom browser headers and/or reconfigure the browser settings in the current test harness.)

@@ -883,6 +883,8 @@ LibraryManager.library = {
ENV['PWD'] = '/';
ENV['HOME'] = '/home/web_user';
ENV['LANG'] = 'C.UTF-8';
// Browser language detection #8751
ENV['LANG'] = (typeof navigator === 'object' && navigator.languages && navigator.languages[0] || 'C').replace('-', '_') + '.UTF-8';

This comment has been minimized.

Copy link
@kripken

kripken Jun 24, 2019

Member

How does the order of evaluation work here - is && before || or the opposite? Might be clearer to add parens, I think.

// Expected results:
// LANG=C: 1 / 3.4
// LANG=fr_FR.UTF-8: 1,2 / 3
// Note: locale needs to be installed (cf. `locale -a`)

This comment has been minimized.

Copy link
@kripken

kripken Jun 24, 2019

Member

I'm not sure what this means - it needs to be installed locally? What error happens if it isn't?

@kripken

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Thanks, looks good aside from those minor comments! CI errors look unrelated too.

@Beuc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

I added clarifications.

@kripken

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Great, thanks @Beuc!

@kripken kripken merged commit 0ffcb05 into emscripten-core:incoming Jun 25, 2019

25 of 27 checks passed

ci/circleci: test-upstream-browser-chrome Your tests failed on CircleCI
Details
ci/circleci: test-upstream-browser-firefox Your tests failed on CircleCI
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-upstream Your tests passed on CircleCI!
Details
ci/circleci: flake8 Your tests passed on CircleCI!
Details
ci/circleci: test-ab Your tests passed on CircleCI!
Details
ci/circleci: test-browser-chrome Your tests passed on CircleCI!
Details
ci/circleci: test-browser-firefox Your tests passed on CircleCI!
Details
ci/circleci: test-c Your tests passed on CircleCI!
Details
ci/circleci: test-d Your tests passed on CircleCI!
Details
ci/circleci: test-e Your tests passed on CircleCI!
Details
ci/circleci: test-f Your tests passed on CircleCI!
Details
ci/circleci: test-ghi Your tests passed on CircleCI!
Details
ci/circleci: test-jklmno Your tests passed on CircleCI!
Details
ci/circleci: test-other Your tests passed on CircleCI!
Details
ci/circleci: test-p Your tests passed on CircleCI!
Details
ci/circleci: test-qrst Your tests passed on CircleCI!
Details
ci/circleci: test-sanity Your tests passed on CircleCI!
Details
ci/circleci: test-upstream-other Your tests passed on CircleCI!
Details
ci/circleci: test-upstream-wasm0 Your tests passed on CircleCI!
Details
ci/circleci: test-upstream-wasm2 Your tests passed on CircleCI!
Details
ci/circleci: test-uvwxyz Your tests passed on CircleCI!
Details
ci/circleci: test-wasm0 Your tests passed on CircleCI!
Details
ci/circleci: test-wasm1 Your tests passed on CircleCI!
Details
ci/circleci: test-wasm2 Your tests passed on CircleCI!
Details
ci/circleci: test-wasm3 Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.