Bug 1119062 (enable-unicode-range-release)

enable unicode-range in release builds

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Core
Graphics: Text
P2
major
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: jtd, Assigned: jtd)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla45
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8545607 [details] [diff] [review]
patch, enable unicode-range in release builds
(Assignee)

Updated

2 years ago
Depends on: 1180560
Alias: enable-unicode-range-release
Depends on: 475891
Blocks: 651693
Keywords: dev-doc-needed

Comment 2

2 years ago
What's the plan for releasing this fix?
Flags: needinfo?(jdaggett)
(Assignee)

Comment 3

2 years ago
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.
Flags: needinfo?(jdaggett)

Updated

2 years ago
Depends on: 1218123
(Assignee)

Comment 4

2 years ago
Created attachment 8689290 [details] [diff] [review]
patch, enable unicode-range in release builds

Once bug 1180560 lands we can enable unicode-range in release builds.
Attachment #8545607 - Attachment is obsolete: true
Attachment #8689290 - Flags: review?(dbaron)
Did you send an intent to ship notice to dev-platform?  See https://wiki.mozilla.org/WebAPI/ExposureGuidelines
Flags: needinfo?(jdaggett)
(Assignee)

Comment 6

2 years ago
Done.
https://groups.google.com/forum/#!topic/mozilla.dev.platform/QFJO6aqQwIs
Flags: needinfo?(jdaggett)
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).
Attachment #8689290 - Flags: review?(dbaron) → review+
(Assignee)

Comment 8

2 years ago
(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.
(Assignee)

Comment 9

2 years ago
Created attachment 8689912 [details] [diff] [review]
patch, enable unicode-range load test under Linux

Forgot to remove the "skip on Linux" of the unicode-range load test.
Attachment #8689912 - Flags: review?(dbaron)
Attachment #8689912 - Flags: review?(dbaron) → review+

Comment 10

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

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ab494cc7de0d
https://hg.mozilla.org/mozilla-central/rev/55df0ae1a9f5
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Comment 12

2 years ago
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
Attachment #8689290 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 13

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

Approval Request Comment

-- same as above --
Attachment #8689912 - Flags: approval-mozilla-aurora?

Updated

2 years ago
status-firefox44: --- → affected
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.
Flags: needinfo?(jdaggett)
(Assignee)

Comment 15

2 years ago
(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.
Flags: needinfo?(jdaggett)
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.
Attachment #8689290 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8689912 [details] [diff] [review]
patch, enable unicode-range load test under Linux

Aurora44+
Attachment #8689912 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 18

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d05845d8e77
https://hg.mozilla.org/releases/mozilla-aurora/rev/6c630e37e53f
status-firefox44: affected → fixed

Comment 19

2 years ago
Why MSN display Firefox 36 support?

Comment 20

2 years ago
https://developer.mozilla.org/zh-CN/docs/Web/CSS/@font-face/unicode-range
(Assignee)

Comment 21

2 years ago
(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.
(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.
Flags: needinfo?(jdaggett)

Comment 23

2 years ago
(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

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3d05845d8e77
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/6c630e37e53f
status-b2g-v2.5: --- → fixed

Comment 25

2 years ago
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.
(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?
(Assignee)

Comment 27

2 years ago
(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.
Flags: needinfo?(jdaggett)
(Assignee)

Comment 28

2 years ago
(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.
See Also: → bug 1188802
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
Keywords: dev-doc-needed → dev-doc-complete

Comment 30

2 years ago
Not Firefox 45?
(Assignee)

Comment 31

2 years ago
(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.
(Assignee)

Updated

2 years ago
Depends on: 1230437

Comment 32

2 years ago
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?
(Assignee)

Comment 33

2 years ago
(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

2 years ago
> 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

2 years ago
> 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
(Assignee)

Comment 36

2 years ago
(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
(Assignee)

Updated

2 years ago
Depends on: 1245811
Depends on: 1253542
Depends on: 1286011
You need to log in before you can comment on or make changes to this bug.