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

Handle browsers with no navigator.language better (#521) #674

Merged
merged 1 commit into from
Feb 4, 2022

Conversation

zackw
Copy link
Contributor

@zackw zackw commented Nov 4, 2020

The code for setting config["lang"], in isso/app/config.js,
assumes that browsers will always provide a value for
navigator.language and/or navigator.userLanguage. Per bug #521,
this is not a safe assumption.

While I was attempting to fix this, I noticed that regional variants
of a language (zh-TW, pt-BR) were being handled in an ad-hoc,
unreliable manner. I also noticed a new user-agent language property
navigator.languages which more closely matches the semantics of
Accept-Language—it would be good to support that.

This patch addresses all the above problems, as follows:

  1. Add a new configuration property data-isso-default-lang that
    specifies the language to use (instead of English) when the browser
    doesn’t have a preference.

  2. Document that we expect the value of data-isso-lang and
    data-isso-default-lang to be a BCP 47 language tag, because
    this is what navigator.language etc contain. (The practical
    upshot is that tags like zh-TW are officially allowed now.)

  3. In config.js, compile an array of candidate language tags, in
    descending order of preference, from all available sources:
    data-isso-lang, navigator.languages, navigator.language,
    navigator.userLanguage, data-isso-default-lang, and finally
    "en" as a backstop. Handle any or all of the above being
    null/undefined/empty. This array goes into config["langs"].
    config["lang"] is removed.

  4. In i18n.js, select the first entry in config["langs"] for which
    we have both pluralforms and a translation, and make that the value
    of i18n.lang. An English backstop is supplied here too for extra
    defensiveness. Also, if we don’t have a translation for say
    zh-HK, try zh before moving on to the next entry in the array.

  5. New function utils.normalize_bcp47 ensures that we process
    language tags, whereever they came from, case-insensitively and
    treating _ as equivalent to -.

  6. Move utils.ago to i18n.ago to avoid a circular dependency
    between utils and i18n.

@ix5
Copy link
Member

ix5 commented Feb 7, 2021

Good job on the PR!

I've tested with navigator.lang{ueages} set to en, en-US, de, de-de and pt-BR so far.
Apart from the nit in the linline comment I posted, it works great.

Could you rebase on latest master please? The pt_PT translation was added in the meantime.
I've pushed a rebased version with the fix for lang is undefined to ix5/isso:navigator-lang-674 for you to pick.

@zackw
Copy link
Contributor Author

zackw commented Feb 8, 2021

@ix5 Rebased and made both of the requested changes.

@ix5
Copy link
Member

ix5 commented Feb 8, 2021

Great, thanks! @jelmer @blatinier please review when you find the time.

@ix5 ix5 mentioned this pull request Dec 22, 2021
@ix5 ix5 self-assigned this Dec 27, 2021
@ix5
Copy link
Member

ix5 commented Jan 24, 2022

@zackw I've rebased your branch on top of current master, please see https://github.com/ix5/isso/tree/pull-674-rebased
Would you mind adding an entry to the CHANGES.rst file as well?

After that, I'll merge this PR.

The code for setting `config["lang"]`, in `isso/app/config.js`,
assumes that browsers will always provide a value for
`navigator.language` and/or `navigator.userLanguage`.  Per bug isso-comments#521,
this is not a safe assumption.

While I was attempting to fix this, I noticed that regional variants
of a language (`zh-TW`, `pt-BR`) were being handled in an ad-hoc,
unreliable manner.  I also noticed a new user-agent language property
[`navigator.languages`][] which more closely matches the semantics of
[`Accept-Language`][]—it would be good to support that.

This patch addresses all the above problems, as follows:

1. Add a new configuration property `data-isso-default-lang` that
   specifies the language to use (instead of English) when the browser
   *doesn’t* have a preference.

2. Document that we expect the value of `data-isso-lang` and
   `data-isso-default-lang` to be a [BCP 47][] language tag, because
   this is what `navigator.language` etc contain.  (The practical
   upshot is that tags like `zh-TW` are officially allowed now.)

3. In `config.js`, compile an array of candidate language tags, in
   descending order of preference, from all available sources:
   `data-isso-lang`, `navigator.languages`, `navigator.language`,
   `navigator.userLanguage`, `data-isso-default-lang`, and finally
   `"en"` as a backstop.  Handle any or all of the above being
   null/undefined/empty.  This array goes into `config["langs"]`.
   `config["lang"]` is removed.

4. In `i18n.js`, select the first entry in `config["langs"]` for which
   we have both pluralforms and a translation, and make that the value
   of `i18n.lang`.  An English backstop is supplied here too for extra
   defensiveness.  Also, if we don’t have a translation for say
   `zh-HK`, try `zh` before moving on to the next entry in the array.

5. New function `utils.normalize_bcp47` ensures that we process
   language tags, whereever they came from, case-insensitively and
   treating `_` as equivalent to `-`.

6. Move `utils.ago` to `i18n.ago` to avoid a circular dependency
   between utils and i18n.

[`navigator.languages`]: https://developer.mozilla.org/en-US/docs/Web/API/NavigatorLanguage/languages
[`Accept-Language`]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Language
[BCP 47]: https://tools.ietf.org/html/bcp47
@zackw
Copy link
Contributor Author

zackw commented Jan 24, 2022

@ix5 Done.

@stefangehn
Copy link
Contributor

LGTM, I think proper handling of the two-part language identifiers is also worth merging this.

@ix5 ix5 merged commit 48a4736 into isso-comments:master Feb 4, 2022
@ix5
Copy link
Member

ix5 commented Feb 4, 2022

Thank you for reviewing, @stefangehn!
And thank you for your patience @zackw!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants