Skip to content

Use FormatJS Intl.Collator polyfill to fix missing locales in Chromium (e.g. "se") - #2025

Merged
osma merged 8 commits into
mainfrom
issue2016-chromium-intl-polyfill
Jun 3, 2026
Merged

Use FormatJS Intl.Collator polyfill to fix missing locales in Chromium (e.g. "se")#2025
osma merged 8 commits into
mainfrom
issue2016-chromium-intl-polyfill

Conversation

@osma

@osma osma commented May 28, 2026

Copy link
Copy Markdown
Member

Reasons for creating this PR

The hierarchy sidebar does not sort concepts correctly in Northern Sami locale (se) on Chromium based browsers, see #2016. This PR fixes the issue by using the Intl.Collator polyfill from FormatJS which can be used to retrofit wider locale support to JavaScript environments. The polyfill is quite heavy and it is only loaded when actually needed: in my tests using Chromium, it will be loaded when the content language is set to Northern Sámi and the page includes a hierarchy sidebar (even if it's not the currently active tab). In all other cases (e.g. Firefox browser, a content language other than Northern Sámi, or a page without the hierarchy tab), the polyfill will not be loaded at all!

Link to relevant issue(s), if any

Description of the changes in this PR

  • add formatjs/intl-collator as a dependency managed by npm
  • change the hierarchy sidebar Vue app to use Intl.Collator for sorting concept labels; the collators are initialized only once, after loading the Promise that loads the FormatJS polyfill (but only if needed)

Known problems or uncertainties in this PR

  • There were many difficulties creating this: FormatJS is heavily oriented towards JS modules, while Skosmos so far uses more traditional script tags and no build step. It's also difficult to get the timing right; the polyfill takes a long time to initialize so the hierarchy app has to wait for it to be ready, which is now handled using a Promise.
  • Is it OK to add the polyfill JS code into src/view/scripts.inc.twig or would it be better to move it into its own small .js file?
  • No Cypress test for the polyfill yet - I wonder if this can be easily tested?
  • Cypress tests are currently failing due to LCSH mapping issues, unrelated to this PR.

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@osma osma added this to the 3.3 milestone May 28, 2026
@osma osma self-assigned this May 28, 2026
@qltysh

qltysh Bot commented May 28, 2026

Copy link
Copy Markdown

1 new issue

Tool Category Rule Count
qlty Structure Function with many returns (count = 4): compareConcepts 1

// fall back to non-numeric sort to ensure a consistent order
return strA.localeCompare(strB, lang, { sensitivity: 'variant' })
return this.$fallbackCollator.compare(strA, strB)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function with many returns (count = 4): compareConcepts [qlty:return-statements]

@osma
osma force-pushed the issue2016-chromium-intl-polyfill branch from 33e4f01 to daa8d8a Compare May 28, 2026 12:55
@codecov

codecov Bot commented May 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.42%. Comparing base (10ae9e6) to head (080ce95).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2025   +/-   ##
=========================================
  Coverage     70.42%   70.42%           
  Complexity     1703     1703           
=========================================
  Files            34       34           
  Lines          4436     4436           
=========================================
  Hits           3124     3124           
  Misses         1312     1312           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@osma osma moved this to Under review in Skosmos 3.x Backlog Jun 2, 2026
@osma osma changed the title use FormatJS Intl.Collator polyfill to fix missing locales in Chromium Use FormatJS Intl.Collator polyfill to fix missing locales in Chromium (e.g. "se") Jun 2, 2026
@osma
osma requested a review from Copilot June 2, 2026 12:06
@osma
osma marked this pull request as ready for review June 2, 2026 12:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a client-side Intl.Collator polyfill path to fix incorrect concept sorting for locales missing in Chromium (e.g. se), and updates the hierarchy sidebar sorting to use Intl.Collator instances.

Changes:

  • Add @formatjs/intl-collator npm dependency.
  • Load the Collator polyfill via dynamic import() and expose a global readiness Promise.
  • Replace localeCompare() sorting in the hierarchy sidebar with shared Intl.Collator instances and wait for the polyfill before starting the app.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/view/scripts.inc.twig Adds import map + module loader that conditionally imports the FormatJS Collator polyfill and exposes window.intlCollatorReady.
resource/js/tab-hierarchy.js Switches hierarchy sorting to use $collator.compare() and waits for the polyfill Promise before starting.
package.json Adds @formatjs/intl-collator dependency.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/view/scripts.inc.twig
Comment thread resource/js/tab-hierarchy.js Outdated
@osma
osma requested a review from UnniKohonen June 2, 2026 12:35

@UnniKohonen UnniKohonen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collation seems to be working correctly in Chrome. Moving the polyfill code to a separate file would make scripts.inc a little cleaner but it's not necessary IMO. Error handling in tab-hierarchy.js suggested by copilot should probably be implemented but otherwise this implementation seems fine to me.

Maybe the existence of #tab-hierarchy in DOM could be checked before loading the polyfill if you haven't tried that already.

Comment thread src/view/scripts.inc.twig
Comment thread resource/js/tab-hierarchy.js Outdated
Comment thread resource/js/tab-hierarchy.js
@osma

osma commented Jun 2, 2026

Copy link
Copy Markdown
Member Author

Maybe the existence of #tab-hierarchy in DOM could be checked before loading the polyfill if you haven't tried that already.

I've now made the polyfill loading even lazier: it will only be loaded if the hierarchy initialization code needs it. This means that it will not be loaded on e.g. the landing and feedback pages which don't have a hierarchy element.

@UnniKohonen UnniKohonen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@osma
osma merged commit 860c2e6 into main Jun 3, 2026
14 of 17 checks passed
@github-project-automation github-project-automation Bot moved this from Under review to Issue/PR closed in Skosmos 3.x Backlog Jun 3, 2026
@osma
osma deleted the issue2016-chromium-intl-polyfill branch June 3, 2026 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Issue/PR closed

Development

Successfully merging this pull request may close these issues.

Northern Sámi collation not supported in Chromium-based browsers

3 participants