1119062 - (enable-unicode-range-release) enable unicode-range in release builds

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect, P2)

Description

[John Daggett (:jtd)]

The unicode-range implementation is currently only enabled for nightly/aurora builds, waiting for the Linux platform fontlist implementation (bug 1056479). Once that lands and appears to be stable we can enable unicode-range in release builds.

Comment 1

[John Daggett (:jtd)]

Attachment: patch, enable unicode-range in release builds

Comment 2

[dhendo]

What's the plan for releasing this fix?

Comment 3

[John Daggett (:jtd)]

Currently gated by bugs blocking the fontconfig platform fontlist use under Linux. See bug 1180560. That bug gets fixed ==> this bug gets resolved, unicode-range is enabled by default for release builds.

Comment 4

[John Daggett (:jtd)]

Attachment: patch, enable unicode-range in release builds

Once bug 1180560 lands we can enable unicode-range in release builds.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you send an intent to ship notice to dev-platform?  See https://wiki.mozilla.org/WebAPI/ExposureGuidelines

Comment 6

[John Daggett (:jtd)]

Done.
https://groups.google.com/forum/#!topic/mozilla.dev.platform/QFJO6aqQwIs

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8689290 [details] [diff] [review]
patch, enable unicode-range in release builds

r=dbaron


One other thought, though (since you made me look at what the test coverage is):  given that the reason for unicode-range to exist is that it allows a way of not bothering to download a font at all based on character ranges, it seems like it's probably worth adding a mochitest to actually test the "lack of download" aspect of it (with a base case that should be downloaded).  It shouldn't be too hard to do in the manner of test_bug517224.html and bug517224.sjs in layout/style/test/ (although you probably want multiple tests, like the somewhat more complicated visited_image_loading test in the same directory).

Comment 8

[John Daggett (:jtd)]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #7)
> Comment on attachment 8689290 [details] [diff] [review]
> patch, enable unicode-range in release builds
> 
> r=dbaron
> 
> 
> One other thought, though (since you made me look at what the test coverage
> is):  given that the reason for unicode-range to exist is that it allows a
> way of not bothering to download a font at all based on character ranges, it
> seems like it's probably worth adding a mochitest to actually test the "lack
> of download" aspect of it (with a base case that should be downloaded).  It
> shouldn't be too hard to do in the manner of test_bug517224.html and
> bug517224.sjs in layout/style/test/ (although you probably want multiple
> tests, like the somewhat more complicated visited_image_loading test in the
> same directory).

Hmmm, that's basically what the slew of tests in layout/style/test/test_unicode_range_loading.html are doing? Did you look through those? Would you consider those not sufficient? These tests use the font loading API rather than .sjs magic. But if needed I'm sure I could come up with the kind of test you're thinking about too.

Comment 9

[John Daggett (:jtd)]

Attachment: patch, enable unicode-range load test under Linux

Forgot to remove the "skip on Linux" of the unicode-range load test.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab494cc7de0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/55df0ae1a9f5

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ab494cc7de0d
https://hg.mozilla.org/mozilla-central/rev/55df0ae1a9f5

Comment 12

[John Daggett (:jtd)]

Comment on attachment 8689290 [details] [diff] [review]
patch, enable unicode-range in release builds

Approval Request Comment
[Feature/regressing bug #]: With the new work on the Linux font backend, we can now enable unicode-range for beta/release across all platforms.
[User impact if declined]: unicode-range support can't be enabled until FF45
[Describe test coverage new/current, TreeHerder]: landed on trunk last week, no issues reported
[Risks and why]: new fontconfig font backend has been enabled for nightly/aurora for several release cycles now and all known regressions have been resolved
[String/UUID change made/needed]: none

Comment 13

[John Daggett (:jtd)]

Comment on attachment 8689912 [details] [diff] [review]
patch, enable unicode-range load test under Linux

Approval Request Comment

-- same as above --

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

John, while reviewing the patch I noticed that the pref "layout.css.unicode-range.enabled" is being removed completely. Is that intentional? I got confused because the bug talks about enabling unicode-range. Does it mean it is on and not something that can be turned off by a pref setting? Please let me know.

Comment 15

[John Daggett (:jtd)]

(In reply to Ritu Kothari (:ritu) from comment #14)
> John, while reviewing the patch I noticed that the pref
> "layout.css.unicode-range.enabled" is being removed completely. Is that
> intentional? I got confused because the bug talks about enabling
> unicode-range. Does it mean it is on and not something that can be turned
> off by a pref setting? Please let me know.

Right, "enabling" in this case means making it always on by default and removing the pref completely.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8689290 [details] [diff] [review]
patch, enable unicode-range in release builds

This has been on Nightly for 10 days or so. Let's uplift to Aurora44.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8689912 [details] [diff] [review]
patch, enable unicode-range load test under Linux

Aurora44+

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d05845d8e77
https://hg.mozilla.org/releases/mozilla-aurora/rev/6c630e37e53f

Comment 19

[yisibl]

Why MSN display Firefox 36 support?

Comment 20

[yisibl]

https://developer.mozilla.org/zh-CN/docs/Web/CSS/@font-face/unicode-range

Comment 21

[John Daggett (:jtd)]

(In reply to percyley from comment #19)
> Why MSN display Firefox 36 support?

It was supported initially behind a flag, then only in nightly/developer builds. This bug enables it by default for beta/release builds.

I'll work with the dev-docs folks to clarify this on MDN.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to John Daggett (:jtd) from comment #8)
> (In reply to David Baron [:dbaron] ⌚UTC-8 from comment #7)
> > One other thought, though (since you made me look at what the test coverage
> > is):  given that the reason for unicode-range to exist is that it allows a
> > way of not bothering to download a font at all based on character ranges, it
> > seems like it's probably worth adding a mochitest to actually test the "lack
> > of download" aspect of it (with a base case that should be downloaded).  It
> > shouldn't be too hard to do in the manner of test_bug517224.html and
> > bug517224.sjs in layout/style/test/ (although you probably want multiple
> > tests, like the somewhat more complicated visited_image_loading test in the
> > same directory).
> 
> Hmmm, that's basically what the slew of tests in
> layout/style/test/test_unicode_range_loading.html are doing? Did you look
> through those? Would you consider those not sufficient? These tests use the
> font loading API rather than .sjs magic. But if needed I'm sure I could come
> up with the kind of test you're thinking about too.

I guess I think it would still be a good idea to write such a test.

Comment 23

[yisibl]

(In reply to John Daggett (:jtd) from comment #21)
> (In reply to percyley from comment #19)
> > Why MSN display Firefox 36 support?
> 
> It was supported initially behind a flag, then only in nightly/developer
> builds. This bug enables it by default for beta/release builds.
> 
> I'll work with the dev-docs folks to clarify this on MDN.

Thanks!

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3d05845d8e77
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/6c630e37e53f

Comment 25

[Rod]

In preparation for enabling unicode-range for Firefox in Google Fonts I tried out unicode-range on Firefox dev edition 44.0a2 (2015-12-02) and ran into unexpected behavior around src descriptors using local(). 

I have a test that is intended to download two font files (urls ending in ?cyrillic and ?latin-ext). 

If there are no local()'s in src descriptor Firefox downloads three font files: the ones for cyrillic and latin-ext plus one for latin. http://output.jsbin.com/vuragugoza/quiet shows this.

If there are local()'s in the src descriptor Firefox downloads the same three font files and then shortly after that downloads the same three font files again. The Dev Tools Network tab seems to show it downloads the three files, waits a bit, then downloads them again. http://output.jsbin.com/gihumugeya/quiet shows this.

If I make an example where only some src descriptors have local() then only those show as double-downloading.

I had Dev Tools open and Disable Cache (when toolbox is open) enabled when testing.

Comment 26

[Jonathan Kew [:jfkthame]]

(In reply to Rod from comment #25)
> In preparation for enabling unicode-range for Firefox in Google Fonts I
> tried out unicode-range on Firefox dev edition 44.0a2 (2015-12-02) and ran
> into unexpected behavior around src descriptors using local(). 
> 
> I have a test that is intended to download two font files (urls ending in
> ?cyrillic and ?latin-ext). 

Looking at your example  http://output.jsbin.com/gihumugeya  I think it's expected that the ?latin file will also be downloaded, because neither ?cyrillic nor ?latin-ext lists the U+0020 (space) codepoint in its unicode-range.

> 
> If there are no local()'s in src descriptor Firefox downloads three font
> files: the ones for cyrillic and latin-ext plus one for latin.
> http://output.jsbin.com/vuragugoza/quiet shows this.
> 
> If there are local()'s in the src descriptor Firefox downloads the same
> three font files and then shortly after that downloads the same three font
> files again. The Dev Tools Network tab seems to show it downloads the three
> files, waits a bit, then downloads them again.
> http://output.jsbin.com/gihumugeya/quiet shows this.

I can reproduce this with Developer Edition (44.0a2), but don't see it happening in a Nightly build (45.0a1).

jdaggett, does this sound familiar? A known-and-already-fixed issue?

Comment 27

[John Daggett (:jtd)]

(In reply to percyley from comment #23)

> > It was supported initially behind a flag, then only in nightly/developer
> > builds. This bug enables it by default for beta/release builds.
> > 
> > I'll work with the dev-docs folks to clarify this on MDN.
> 
> Thanks!

Just updated the unicode-range MDN page to hopefully document current behavior more clearly.

Comment 28

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #26)

> > If there are local()'s in the src descriptor Firefox downloads the same
> > three font files and then shortly after that downloads the same three font
> > files again. The Dev Tools Network tab seems to show it downloads the three
> > files, waits a bit, then downloads them again.
> > http://output.jsbin.com/gihumugeya/quiet shows this.
> 
> I can reproduce this with Developer Edition (44.0a2), but don't see it
> happening in a Nightly build (45.0a1).
> 
> jdaggett, does this sound familiar? A known-and-already-fixed issue?

Let me take a look today. If it looks like there's a problem, I'll file a new bug. The repeated downloads may be something related to bug 1188802.

Comment 29

[Jean-Yves Perrier [:teoli]]

Thanks all for the help in documenting this!
https://developer.mozilla.org/en-US/docs/Web/CSS/%40font-face/unicode-range
and
https://developer.mozilla.org/en-US/Firefox/Releases/44#CSS

Comment 30

[yisibl]

Not Firefox 45?

Comment 31

[John Daggett (:jtd)]

(In reply to percyley from comment #30)
> Not Firefox 45?

See comment 18, patches have been uplifted to aurora which is what will become Firefox 44.

Comment 32

[michelesr]

Seems that after release 44 some font families are not matched correctly in Linux platforms.

For example "Liberation Mono" seems to be ignored even if is installed correctly in the system:

$ fc-match 'Liberation Mono'
LiberationMono-Regular.ttf: "Liberation Mono" "Regular"

This didn't happen in previous releases. Is it related to unicode-range?

Comment 33

[John Daggett (:jtd)]

(In reply to michelesr from comment #32)
> Seems that after release 44 some font families are not matched correctly in
> Linux platforms.
> 
> For example "Liberation Mono" seems to be ignored even if is installed
> correctly in the system:
> 
> $ fc-match 'Liberation Mono'
> LiberationMono-Regular.ttf: "Liberation Mono" "Regular"
> 
> This didn't happen in previous releases. Is it related to unicode-range?

Hmmm, I'm not seeing this with Liberation Mono installed under Ubuntu. What do you see for URL below?

data:text/html,<body style="font-family: Liberation Mono,serif">burp

Serif or monospace font?

If you don't see monospace could you log a new bug?

Comment 34

[michelesr]

> Hmmm, I'm not seeing this with Liberation Mono installed under Ubuntu. What do you see for URL below?

DejaVu Serif system

Used as: "DejaVu Serif"

Comment 35

[michelesr]

> Hmmm, I'm not seeing this with Liberation Mono installed under Ubuntu. What do you see for URL below?

Arch Linux here, Linux x86_64

fontconfig version: 2.11.1-2
fontconfig configuration: system provided/untouched 

I noticed "Liberation Mono" stopping working after upgrade from 43 to 44

Comment 36

[John Daggett (:jtd)]

(In reply to michelesr from comment #34)
> > Hmmm, I'm not seeing this with Liberation Mono installed under Ubuntu. What do you see for URL below?
> 
> DejaVu Serif system
> 
> Used as: "DejaVu Serif"

Logged as bug 1245811