Closed Bug 1449605 Opened 6 years ago Closed 6 years ago

Some letters thickness is not consistent on https://developer.microsoft.com page

Categories

(Core :: Layout: Text and Fonts, defect)

61 Branch
x86_64
Windows
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox61 --- verified
firefox62 --- verified

People

(Reporter: roxana.leitan, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Attached image fonts.png
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180328100134

[Affected versions]:
Nightly 61.0a1

[Affected platforms]:
Windows 10 X64

[Steps to reproduce]:
1.Launch Nightly 61.0a1 with a new profile
2.Open https://developer.microsoft.com/en-us/microsoft-edge/testdrive/demos/variable-fonts/

[Expected results]:
The letters thickness should be consistent 

[Actual results]:
"R" and "P" letters thickness is not consistent 

[Note]:
The issue is reproducible also with "layout.css.font-variations.enabled" set to False
Wow, that's ugly. It appears to be an issue with the bold simulation being applied to the Bahnschrift font (when a large font-weight is applied).
Now that bug 1454597 is done, we should re-examine this.

I'm still seeing bad rendering of the headings using the "Bahnschrift Variable" font on the MS demo page. The reason is that the page loads Bahnschrift using a simple @font-face rule that doesn't include a font-weight descriptor, although in fact the resource supports a range of weights (300..700, IIRC). But because there's no explicit weight descriptor, it's considered to have font-weight:400 for font selection purposes.

Then, because the face (nominally) has weight:400, when a bold font-weight is applied by the page's CSS, we end up applying a synthetic-bold effect at rendering time (in addition to setting the "wght" variation axis). This results in overly-bold rendering on macOS, and a poor/inconsistent result on Windows (because this font doesn't work well with DirectWrite's "bold simulation").

What I think we should do is to disable the application of synthetic-bold effects when using a variable font that has a "wght" axis, even if the font entry (based on the @font-face descriptor, or implied) suggests the face is only a light/regular weight.
Blocks: 1302685
Depends on: 1454597
I've created a couple of testcases to check the behavior involved here:

* https://jfkthame.github.io/variation-fonts/bahnschrift.html
Testcase based on the Microsoft demo page. This loads the Bahnschrift font from the MS site, so it may break if that resource becomes unavailable.

* https://jfkthame.github.io/variation-fonts/mutator.html
Similar example using the MutatorSans font, hosted locally so not liable to disappear without notice. :)

In each case, the expected result is described in the test page. In current Nightly, they work _almost_ correctly; the exception is that in the first line of the final group of 4 examples, we apply synthetic-bold to the last part of the text, but we shouldn't do this because the boldness is provided by the variation font.

(Note that none of the currently-shipping implementations I've tried (Safari, Chrome, Edge) render these examples correctly, nor do any two of them agree with each other. I believe the "expected" behavior as described in the testcases corresponds to what the CSSWG has recently decided, but nobody yet implements it; nor is there interop agreement on any other result.)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8971818 [details] [diff] [review]
Rename gfxFontEntry::IsBold() to SupportsBold() and make it smarter about variable fonts, to avoid inappropriate use of synthetic-bold effect

Cancelling r? for now, as this fails a couple of reftests on android. :( Will update the patch once I figure out what's going on there...
Attachment #8971818 - Flags: review?(jwatt)
Comment on attachment 8971818 [details] [diff] [review]
Rename gfxFontEntry::IsBold() to SupportsBold() and make it smarter about variable fonts, to avoid inappropriate use of synthetic-bold effect

Review of attachment 8971818 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFontEntry.cpp
@@ +1523,5 @@
>      uint32_t count = mAvailableFonts.Length();
>      if (count == 1) {
>          fe = mAvailableFonts[0];
>          aNeedsSyntheticBold =
> +            wantBold && aFontStyle.allowSyntheticWeight && !fe->SupportsBold();

Nit: I think the previous ordering reads more naturally and doesn't require a small mental hop back.

@@ +1566,5 @@
>              fe = mAvailableFonts[order[trial]];
>              if (fe) {
>                  aNeedsSyntheticBold =
> +                    wantBold && aFontStyle.allowSyntheticWeight &&
> +                    !fe->SupportsBold();

Same here.

@@ +1614,5 @@
>  
>      if (matched) {
>          aFontEntryList.AppendElement(matched);
> +        if (aFontStyle.weight >= FontWeight(600) &&
> +            aFontStyle.allowSyntheticWeight && !matched->SupportsBold()) {

I guess the ordering was already different here, but it would be good if all three matched.

::: gfx/thebes/gfxTextRun.cpp
@@ +2821,5 @@
>      }
>  
> +    bool needsBold = mStyle.weight >= FontWeight(600) &&
> +                     mStyle.allowSyntheticWeight &&
> +                     fe->SupportsBold();

Oh, a fourth place. ;)

@@ +3447,5 @@
>          gfxPlatformFontList::PlatformFontList()->
>              SystemFindFontForChar(aCh, aNextCh, aRunScript, &mStyle);
>      if (fe) {
>          bool wantBold = mStyle.weight >= FontWeight(600);
> +        return fe->FindOrMakeFont(&mStyle, wantBold && !fe->SupportsBold());

Hmm, why aren't we checking mStyle.allowSyntheticWeight here as we do elsewhere?
(In reply to Jonathan Kew (:jfkthame) from comment #5)
> Will update the patch once I figure out what's going on there...

Ah, I'll await your new review then. :)
(In reply to Jonathan Watt [:jwatt] from comment #6)
> Comment on attachment 8971818 [details] [diff] [review]
> Rename gfxFontEntry::IsBold() to SupportsBold() and make it smarter about
> variable fonts, to avoid inappropriate use of synthetic-bold effect
> 
> Review of attachment 8971818 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxFontEntry.cpp
> @@ +1523,5 @@
> >      uint32_t count = mAvailableFonts.Length();
> >      if (count == 1) {
> >          fe = mAvailableFonts[0];
> >          aNeedsSyntheticBold =
> > +            wantBold && aFontStyle.allowSyntheticWeight && !fe->SupportsBold();
> 
> Nit: I think the previous ordering reads more naturally and doesn't require
> a small mental hop back.

I reordered these because allowSyntheticWeight is a simple flag test, while SupportsBold() may now be a bit more expensive; so at least for authors who care enough to disable synthetic styling, this should be more optimal.
I have a similar test here: http://test.chrislewis.codes/faux-bold-var/

Assuming that a font-variation-settings: "wght" rule should override a font-weight rule, then the second two lines of each trio should display the same weight.
(In reply to Jonathan Kew (:jfkthame) from comment #8)
> I reordered these because allowSyntheticWeight is a simple flag test, while
> SupportsBold() may now be a bit more expensive; so at least for authors who
> care enough to disable synthetic styling, this should be more optimal.

I would hope compilers would be smart enough to reorder a series of "&&" expressions to make dereferences after simple truthiness checks, but I admittedly haven't checked.
I doubt they can do that, in general. IIRC, && (and ||) promise to evaluate terms in left-to-right order, and short-circuit as soon as the result is known. So (X && Y) is guaranteed to evaluate X in all cases (which might be important if X has side-effects), and is guaranteed not to evaluate Y at all if X is true.

https://en.cppreference.com/w/cpp/language/eval_order
https://en.cppreference.com/w/cpp/language/operator_logical
Just writing to weigh in from the type community - 

General preference seems to be that if `font-variation-settings' is used to specify` wght` then we would want the the browser to _not_ apply a synthetic bold weight. (either from a specified `font-weight` or an implied one via user agent styles, such as for headings)

Our logic is that if you go to the extreme of using `font-variation-settings` then that should have the last word.
So the reason my first patch didn't entirely work is due to the structure of the font selection & rendering code. We currently decide during the font-selection algorithm whether we need to apply synthetic-bold to the chosen face, and then pass that decision through the fontgroup (storing it in the FamilyFace entries of the mFonts array there) down to the actual rendering code that instantiates fonts from the faces (font entries) we've selected.

That's a problem because in the case of a user font, we may not have downloaded the resource yet, so we just have a "user font container" entry, which carries the descriptors from the @font-face rule and will fetch the actual resource when needed. But in the case of a @font-face rule without a weight descriptor, that means we don't actually know at font-selection time whether the face will support "true" bold (via a variation axis) or not, so we can't reliably make the right decision about applying synthetic bold.

To fix this, I propose to refactor the font code so that we no longer try to make that determination during font matching at all, but defer it until we actually instantiate a platform font object to shape/measure/draw text. At that point, we have the requested style and we also have the real font resource, so we can easily determine whether fake-bold is required.

In addition to letting us fix the variation-font case here, this also provides some nice simplification, because we're no longer passing a "needsBold" flag around in so many places because we made the decision so early, compared to where we need to use it.
Attachment #8971818 - Attachment is obsolete: true
Note that there's an analagous problem with synthesized italics and the "ital" variation axis. I've added a test for that here:
http://test.chrislewis.codes/faux-bold-var/
(In reply to chris from comment #16)
> Note that there's an analagous problem with synthesized italics and the
> "ital" variation axis. I've added a test for that here:
> http://test.chrislewis.codes/faux-bold-var/

Right, I'm aware of this. We haven't got all this hooked up yet. The font-style case is a bit complex because of the relationship between normal, oblique and italic; and italic normally maps to a separate face rather than a variation axis. (Whereas oblique maps directly to 'slnt'.)

(FWIW, I think the Kairos Sans font is a poor example; it should be using 'slnt' rather than 'ital' for its variation. 'ital' is not expected to be a continuously-variable feature, it's a binary toggle.)
Depends on: 1458004
Blocks: 1458004
No longer depends on: 1458004
Comment on attachment 8971972 [details] [diff] [review]
part 1 - Refactor so that the decision whether to apply synthetic bold is deferred until actually instantiating a font, not made during the font-matching process

Review of attachment 8971972 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message documents "what" you're changing, but it should really have a comment about the "why?" too (in a separate paragraph).
Attachment #8971972 - Flags: review?(jwatt) → review+
Attachment #8971973 - Flags: review?(jwatt) → review+
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4096171bfdb
part 1 - Rearrange thebes font code so that the decision whether to apply synthetic-bold is deferred until actually instantiating a font, not made during the font-matching process. r=jwatt
Landed part 1 (only) for now, to give a chance to check that it sticks without visible regressions before applying the behavior change in part 2. Marking leave-open until the second patch is also landed.
Keywords: leave-open
It turns out this whole issue could be avoided by specifying a multi-value font-weight in the @font-face descriptor:

https://drafts.csswg.org/css-fonts-4/#font-prop-desc

So, specifying "font-weight: 400 900" in the @font-face would solve the problem, in theory. Of course this requires the browser to recognize the multi-value syntax.

I was totally unaware of this syntax until yesterday!
Yes, that would avoid the issue (and is what the CSS Working Group recommends). I believe all the browsers that support variation fonts also support this syntax (although with some bugs, last time I checked; e.g. not all browsers accept the keyword version "font-weight: normal bold", although it should be exactly the same as "font-weight: 400 700").

Anyhow, we'll be fixing the issue here shortly.
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24583a2f0c84
part 2 - Rename gfxFontEntry::IsBold() to SupportsBold() and make it smarter about variable fonts, to avoid inappropriate use of synthetic-bold effect. r=jwatt
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/24583a2f0c84
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
QA Whiteboard: [good first verify]
Verified fixed on Windows 10 x64 and Ubuntu 16.04 x64 on Nightly 62.0a1(2018-05-08) and Firefox Beta 61.0b3.

For Mac OS X I filed a new bug 1459858.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.