Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix Bug 973396 - Find a better way to serve locale-specific fonts #1706

Merged
merged 1 commit into from Jun 30, 2014

Conversation

Projects
None yet
6 participants
Member

kyoshino commented Feb 17, 2014

The intl.css file should served via CDN like tabzilla.js.

@kyoshino kyoshino referenced this pull request Feb 17, 2014

Closed

Bug 971406 #1688

@jpetto jpetto commented on an outdated diff Mar 3, 2014

bedrock/utils/views.py
@@ -23,8 +26,11 @@ def inner_last_modified(request):
template_time = os.path.getmtime(tmpl_file)
try:
- lang_file = 'tabzilla/tabzilla'
- rel_path = os.path.join('locale', locale, '%s.lang' % lang_file)
+ if template == 'style/intl.css':
+ rel_path = os.path.join(L10N_CSS_PATH, locale, 'intl.css')
+ else:
+ rel_path = os.path.join('locale', locale,
+ 'tabzilla/tabzilla.lang')
@jpetto

jpetto Mar 3, 2014

Member

Is the above if/else too specific for this generic utils app? Would we ever have other templates in utils?

Not a blocker by any means, just curious about approach.

Member

jpetto commented Mar 3, 2014

Would there be any external dependency issues with moving Tabzilla into a different app? I'm not too keen on how other sites implement/source Tabzilla...

@jpetto jpetto commented on an outdated diff Mar 3, 2014

etc/httpd/global.conf
@@ -674,3 +674,6 @@ RewriteRule ^/(\w{2,3}(?:-\w{2})?/)?press/speakerrequest(/?)$ /b/$1press/speaker
# bug 970957
RewriteRule ^/(\w{2,3}(?:-\w{2})?/)?firefox/aurora/up-to-date(/?)$ /b/$1firefox/aurora/up-to-date$2 [PT]
+
+# bug 973396
+RewriteRule ^/(\w{2,3}(?:-\w{2})?/)?style(.*)$ /b/$1style$2 [PT]
@jpetto

jpetto Mar 3, 2014

Member

This looks a little heavy handed, but I am not a regex master. 😔

Couldn't this just target style/intl.css?

@jpetto jpetto commented on an outdated diff Mar 3, 2014

bedrock/base/templates/base-resp.html
@@ -35,6 +35,8 @@
{% include 'includes/canonical-url.html' %}
{% endblock shared_meta %}
+ <link rel="stylesheet" href="{{ settings.CDN_BASE_URL }}{{ url('intl.css') }}?build={{ BUILD_ID_CSS }}">
@jpetto

jpetto Mar 3, 2014

Member

Maybe only include this if a locale specific CSS file exists. For en-US, it's just a link to an empty file.

Member

jpetto commented Mar 3, 2014

Just out of curiosity, how confident are we in the local font names? Where did that information come from?

Member

jpetto commented Mar 3, 2014

Hey @kyoshino -

After discussing this at our PR triage today with @craigcook and @jgmize, we came to the conclusion that his PR has some good ideas, but overall is too heavy-handed. We'd like to try the following:

Keep the existing locale-font-faces.html include, but modify it to use your X-LocaleSpecific font name technique. We should also include any other locale specific font-related CSS in this file - such as letter-spacing or text-transform overrides.

Does that make sense?

Member

kyoshino commented Mar 4, 2014

As I said in the bug, one of the reason behind my approach is I'd like to let other Mozilla sites use the same locale-specific font settings. The existing locale-font-faces.html template is a reasonable solution on Bedrock but it cannot be used by others. I think this will follow the successful Tabzilla utility. Thoughts?

Member

kyoshino commented Mar 10, 2014

So I have made a separate utils app as @pmclanahan suggested on IRC.

Member

kyoshino commented Mar 14, 2014

Added the l10n_css helper function to load a locale-specific CSS file only if exists. Also noted this feature in the l10n doc.

@jpetto jpetto commented on an outdated diff Mar 18, 2014

media/css/sandstone/lib.less
@@ -1,7 +1,7 @@
// Fonts and color
@baseFontSize: 14px;
-@baseFontFamily: 'Open Sans', 'Droid Arabic Naskh', 'Mplus Fulah', sans-serif;
+@baseFontFamily: 'Open Sans', X-LocaleSpecific, sans-serif;
@jpetto

jpetto Mar 18, 2014

Member

I think it would be helpful to devs to add a comment block detailing where X-LocaleSpecific is defined and how it's used. Coming back to this in 6 months or for the first time would likely be confusing.

Member

jpetto commented Mar 18, 2014

I like the more lightweight approach here. The l10n_css helper is nice and unobtrusive for locales with no specific CSS.

One question I'll reiterate is - how confident are we on all the different local font file names? Can we add a comment somewhere linking to material/research supporting these file names? It would be good to have a resource linked to justify decisions, both past and future.

@pmclanahan - Care to give this another review when you have the time?

Member

jpetto commented Mar 24, 2014

@kyoshino - A few things discussed in our triage that we'd like to add:

  1. Reference/source for local font names (for justification and future use)
  2. Comments in lib.less regarding where X-LocaleSpecific is coming from and what it's doing (for future devs)
  3. Add real documentation to read the docs

Thanks!

Member

kyoshino commented Mar 26, 2014

Thanks @jpetto!

  1. Actually some of those locale-specific font names are collected by me. I'm confident with Japanese fonts but anyway I'll file separate bugs to ask localizers to double check. Taiwanese fonts are requested in Bug 795396 so I mentioned the bug number in the CSS file.
  2. Added comments in lib.less and variables.less.
  3. Documented on locale-specific fonts in l10n.rst.
Member

pmclanahan commented Mar 26, 2014

I'm sure I'm missing something, but why are you doing this with a view for serving a CSS file when you could just have a helper to put the CSS file url in a tag in the template like l10n_img?

Member

kyoshino commented Mar 27, 2014

l10n_css outputs nothing if a locale-specific CSS is not provided especially in en-US, so it will help preventing a wasted request. I just fixed the wrong description in misc.py. Note that a wasted request will happen anyway when the CSS is loaded from other Mozilla sites using a CDN URL, because the l10n_css helper function only works on Bedrock.

Member

kyoshino commented Apr 3, 2014

Next step: make it a Jinja helper, as discussed in the meeting.

Member

kyoshino commented Apr 7, 2014

I still don't understand what should I do next. l10n_css is already a context function. As I mentioned earlier, non-Bedrock sites cannot use the helper function anyway; they have to include the intl.css URLs directly. In this case, there will be wasted requests (the file size can be zero) because most locales don't have a locale-specific CSS. This issue doesn't happen on Bedrock because l10n_css outputs nothing when the CSS doesn't exist.

One possible solution here is making this utility a submodule. Pro: other sites can also avoid wasted requests. Con: they have to update the submodule every time a CSS is added or modified.

A purpose of this utility is offering a unified user experience among Mozilla properties with the same typefaces, so I believe serving all CSS files is better than offering a submodule. Thoughts?

Member

kyoshino commented May 5, 2014

Rebased.

Member

kyoshino commented May 8, 2014

So @pmclanahan where the context function should belong to?

Member

pmclanahan commented May 8, 2014

What we're saying is that the intl.css URLs don't need to exist. The
helper context function should work just like our other l10n ones in
that it should either output a link tag to a css file if it exists or
nothing. But it should be directly to the file on the CDN and not to a
special view that outputs CSS if it exists.

Member

kyoshino commented May 8, 2014

Oh, got it! I'll update the PR.

Member

kyoshino commented May 8, 2014

One of the reason why I was using a view was that it could avoid 404 errors when non-existent CSS files are loaded (directly from non-Bedrock sites using the URL). This time I have modified the static file serving function to do the same thing. @pmclanahan r?

Member

pmclanahan commented May 22, 2014

Pushed to demo4 for testing.

@pmclanahan pmclanahan and 2 others commented on an outdated diff May 22, 2014

lib/bedrock_util.py
@@ -23,3 +27,17 @@ def _wrapped_view_func(request, *args, **kwargs):
def server_error_view(request, template_name='500.html'):
"""500 error handler that runs context processors."""
return l10n_utils.render(request, template_name, status=500)
+
+
+def static_serve(request, path, document_root):
@pmclanahan

pmclanahan May 22, 2014

Member

This is only used when in debug mode locally. So in prod these requests will indeed be 404s. Is there a reason we need this? I'd think it might hide issues in development.

@kyoshino

kyoshino May 22, 2014

Member

As I commented earlier, the purpose of this modification was intended to avoid a 404 error when non-existent CSS files are loaded, since the most locales including en-US don't have a locale specific CSS.

Does this work on stage and production?

RewriteCond %{REQUEST_FILENAME} !-f
RewriteRule ^/media/css/l10n/(\w{2,3}(?:-\w{2})?/)/intl\.css$ /media/css/l10n/blank.css [L]
@pmclanahan

pmclanahan May 22, 2014

Member

But that request shouldn't be sent since the l10n_css helper won't produce the tag. And at this point we're not planning to export this feature to any other site.

@kyoshino

kyoshino May 25, 2014

Member

Hmm, but I still want to solve the challenge I wrote before: offering a unified user experience among Mozilla properties with the same typefaces.

@kyoshino

kyoshino May 25, 2014

Member

One possible, simpler solution for the "unified experience" issue while avoiding 404 errors might be... injecting <link rel="stylesheet"> dynamically via Tabzilla. Because tabzilla.js is a Django template, we can have {{ l10n_css() }} and detect if a locale-specific stylesheet is exists in the Python code. It doesn't work if the visitor has disabled JavaScript, but it works on all Tabzilla-ready sites, though they have to update their CSS to use the X-LocaleSpecific font-family and there would be a time lag before fonts are downloaded.

@alexgibson

alexgibson May 25, 2014

Member

It's an interesting suggestion, but I don't know if stuffing more and more features into Tabzilla is necessarily the right thing to do. Seems like two very separate pieces of functionality to me?

And you're right, the download lag could be considerable, given that Tabzilla JS is executed before the closing</body> tag.

@pmclanahan

pmclanahan May 28, 2014

Member

Also the reason we decided to only do this in bedrock for now is that we
didn't want to risk affecting other sites in ways we couldn't foresee. I
agree that possibly making this available to other sites is possibly
good, but let's prove it just on bedrock for now and expand to others if
this works out and they want it to be provided from mozorg.

Member

pmclanahan commented May 22, 2014

As far as the python bits are concerned it looks good to me. I had the one question about the purpose of the modifications to the development static file server, which I think can go away. But apart from that I think we're good if the testing goes well on demo4.

Member

kyoshino commented May 28, 2014

Thanks for your feedback, @pmclanahan and @alexgibson! Okay, let's think about other sites once it goes well on Bedrock. Removed the static_serve modification.

Member

kyoshino commented May 29, 2014

@pmclanahan can you please push this to a demo server again? @craigcook has taken demo4.

Member

pmclanahan commented May 29, 2014

@kyoshino pushed to demo2.

Member

kyoshino commented May 29, 2014

@pmclanahan Thanks!

Member

kyoshino commented Jun 21, 2014

Rebased. Looks like this is still on demo2. This is blocking Azerbaijani locale's work so can we get a quick review? It's simple; ff, ar (and some other locales) should have intl.css in the source, while most locales including en-US and fr should not.

Member

pmclanahan commented Jun 23, 2014

Pushed to demo2 again.

Member

kyoshino commented Jun 25, 2014

LGTM. Can anyone do a spot check? Currently these locales have intl.css: ar, bn-IN, fa, ff, hi-IN, ja, ko, zh-CN, zh-TW

Member

retornam commented Jun 26, 2014

https://www-demo2.allizom.org/ff/ is loading OpenSans first. @kyoshino how is this supposed to work

Member

craigcook commented Jun 26, 2014

I was confused at first, assuming locales would declare their own font stack instead of Open Sans, but once I dug in a little more and tested some other locales I understand what's going on here.

tl;dr: I think this is working as it should, it's just hard to actually see the results on the page without knowing what to look for.

Browsers favor fonts in the order in which they're declared, so they'll try the first font first. If that font isn't available or doesn't have the right characters, it uses the next-specified font, and so on, until it finally reaches the fallback generic font (or, if no generic family is declared, it'll use the browser's own default font). It does this one character at a time, which is why you can sometimes get ugly mixtures of fonts in the same word if your declared font doesn't have all the right characters.

Open Sans has a really broad character set (it's part of why we chose it) so for most languages we still want to favor Open Sans first and the second option is rarely, if ever, used. Rather than redefine the font stack for every locale, this just defines a universal name for "X-Locale-Specific" as the second best choice, and we can define different fonts for that name in intl.css for each locale that needs it. Sometimes it's another webfont, sometimes it's a system font, but in all cases it's the second best option after Open Sans.

For Fula (ff), it looks like Open Sans has all the characters it needs but the second fallback is "mplus-2c-regular-fulah-subset". We're not seeing it on the page because it's not actually needed, Open Sans is doing just fine. We would only see that fallback if Open Sans isn't available (like a 404), or if there's some character Fula needs that Open Sans lacks. So for that locale everything seems to be in order.

Some languages require a very different character set (ar, ja, ko, hi-IN, etc) and Open Sans doesn't have any of the right characters, so the browser goes to the second choice for every character on the page. While we could define a totally different font stack for those locales and leave out Open Sans entirely, there are still occasional instances of mixed languages (proper names, for example) so we still want to favor Open Sans for those cases.

Member

pmclanahan commented Jun 26, 2014

Interesting! Thanks for the explaination @craigcook.

Member

craigcook commented Jun 30, 2014

Finally, at long last, r+. Nice work @kyoshino! 👏 ⭐️ 🌟 💥 🚀

craigcook added a commit that referenced this pull request Jun 30, 2014

Merge pull request #1706 from kyoshino/bug-973396-locale-fonts
Fix Bug 973396 - Find a better way to serve locale-specific fonts

@craigcook craigcook merged commit 3957301 into mozilla:master Jun 30, 2014

1 check passed

default Jenkins build 'bedrock_github' #4468 has succeeded
Details
Member

kyoshino commented Jun 30, 2014

Thank you!!

@kyoshino kyoshino deleted the kyoshino:bug-973396-locale-fonts branch Jun 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment