1544432 - Font Rendering for "Josefin Sans" Broken

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Martin]

Attachment: Screenshots.jpg

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

1. Visit https://glassjore.com/
2. Josefin Sans font no longer renders properly. The outline lettering and font is broken up on drawing and also hover.

Actual results:

WOFF2 "Josefin Sans" font no longer renders properly. The outline lettering and font is broken up on drawing and also hover.

Expected results:

Page should render like attachment of prior versions and also in other Firefox derivative browsers such as Pale Moon or Waterfox.

Comment 1

[Martin]

Screenshot of the glitching font on Firefox 66.0.3: https://ibb.co/s68bfTT

Comment 2

[Jonathan Kew [:jfkthame]]

This seems to be a regression from bug 1519144. Inspecting the SVG text elements on the page makes it clear the bounds of those elements are significantly wrong.

(FWIW, the rendering is slightly glitchy for me prior to bug 1519144; on some of the text lines, the bottom of the letters doesn't paint quite right. It looks like perhaps a rounding error that is getting magnified by scaling, or something like that. But the problem becomes dramatically worse after bug 1519144, with the <text> element rects going wildly out of place.)

Comment 3

[Alex Henrie]

Thank you for the bug report. I am working on it, but right now all I can say is that there's something peculiar about the Josefin Sans font. If you use a different font, there is no problem.

Comment 4

[Martin]

(In reply to Alex Henrie from comment #3)

... there's something peculiar about the Josefin Sans font. If you use a different font, there is no problem.

Sure, if you use a different font, no problem. But a problem with the Josefin Sans font itself? I'm not so sure. Chrome handles it just fine, and I say, so did Firefox in earlier versions, and Firefox forks such as Palemoon and Waterfox.

But if there really is an issue with the font, could we work out what it is so I can contact Google or whomever?

Failing that, it seems like a Firefox issue to me?

Comment 5

[Alex Henrie]

Attachment: [PATCH] Scale around (0,0) when transforming SVG text from run user space to user space

The attached patch seems to fix the problems with the Josefin Sans font, but I don't know if it's the right solution.

Comment 6

[Martin]

Thanks Alex. Can I help you by testing it somehow? Although I have no idea how to actually do that...

Comment 7

[Alex Henrie]

Martin, could you find another font that has the same problem? I am having a hard time figuring out what the right solution should be because I still haven't been able to figure out what makes Josefin Sans special.

Comment 8

[Martin]

(In reply to Alex Henrie from comment #7)

I don't know. All I can say is that the font is a Google font and has been working fine until something in Firefox has changed. Nothing else on the site has changed. Again, font seems fine on other browsers and prior FF versions/derivatives, so I don't know what makes the font different somehow all the sudden.

Comment 9

[Martin]

The font is also hosted locally (and has always been) so it hasn't changed.

Comment 10

[Martin]

*as a file I mean.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Here's a reduced testcase.

In Chrome, I see text in a black box. In Firefox, the text is missing. (It's actually missing in builds from before bug 1519144, too -- so this isn't a perfect testcase for the regression, but it might be a more-general testcase that demonstrates a bug that preexisted and has simply become more revealed / triggerable in more cases.)

Comment 12

[Daniel Holbert [:dholbert]]

(Safari 12 and Edge 18 both show the text, too [matching Chrome], BTW.)

Comment 13

[Daniel Holbert [:dholbert]]

Firefox's DevTools and calls to text-element.getBoundingClientRect seem to suggest that the text is positioned ~700px above the SVG element, which is offscreen in the testcase.

If I push the SVG element down by a good bit (e.g. adding "margin-top:1000px") and scroll down to the SVG element and force it to paint by e.g. clicking or selecting it, then the text shows up -- though devtools and getBoundingClientRect still think it's positioned about 700px above the top of the SVG element.

Comment 14

[Daniel Holbert [:dholbert]]

[dropping "WOFF2" from bug summary because this is actually a TTF font for the characters in question -- specifically https://jore.cc/inc/Qw3FZQNVED7rKGKxtqIqX5Ec0lhtdV0k.ttf I think, or one of several other .ttf files referenced in the stylesheet.]

So the "interesting" thing about this font, from this bug's perspective, is that when we hit the code in Alex's proposed patch, this font gives us a negative value for fill.y and for baseline. If I use another font (e.g. my system default "serif" or "sans-serif", or "Ahem), then those variables both have a value "0". So, that's part of the explanation for why we end up with a mispositioned rect with this font in particular vs. not with other fonts -- it's a question of whether we're scaling up a zero vs. nonzero value.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (no special font, but some diacritics shooting up)

Comment 16

[Daniel Holbert [:dholbert]]

The zero-vs-nonzero thing from comment 14 seems to come from the difference between the top of the text vs. how tall the ascent is.

This font has an unusually large (but probably technically-fine) difference there, probably due to some tall characters. jfkthame was able to trigger the same issue with Helvetica on Mac, as well, with an accented uppercase letter. And "testcase 2" shows that we can create the same scenario with generic fonts with charcters that have been made sufficiently tall via stacked diacritics.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: patch, part 2: automated test for this bug

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (requires Ahem font to be installed, and about:config pref nglayout.debug.paint_flashing = true)

This is a testcase which I think demonstrates that we're still not getting the transform quite correct here, even with the attached patch (and the remaining incorrectness is web-observable issue, via :hover-area at least).

I'll attach a screencast shortly. The testcase has two green blocks with diacritics (requires Ahem font to display correctly), the left one being HTML and the right one being SVG, and it changes color when hovered. This lets its overflow area be tested easily.

EXPECTED RESULTS:

• both green-blocks-with-diacritics should show up.
• they should have the same-size hoverable area.
• the area that gets repainted when you hover them should be the same.

ACTUAL RESULTS, with current Nightly:

• the second green block (SVG) doesn't show up at all (due to the main issue in this bug)

ACTUAL RESULTS, with the tentative patch (attachment 9058842 [details] [diff] [review]) applied:

• the second green block (SVG) has a much taller hoverable area and repainted area (encompassing a bunch of blank space), as compared to the first green block (HTML) which has a nice tightly-contained hoverable/repainted area.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: screencast of testcase 3, in a build with tentative patch

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: screencast of testcase 3, in a build with tentative fix (showing extra repaint/hover area)

Comment 21

[Daniel Holbert [:dholbert]]

Actually, "good news": testcase 3 is unrelated to this bug -- it's a problem even without a viewBox and without any of the scaling that's involved in this bug.

I'll file a separate bug for it.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 9073390 [details]
testcase 3 (requires Ahem font to be installed, and about:config pref nglayout.debug.paint_flashing = true)

(I spun off testcase 3 as bug 1561075.)

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Alex Henrie from comment #5)

The attached patch seems to fix the problems with the Josefin Sans font, but I don't know if it's the right solution.

I suspect this is the right solution.

I think the current mozilla-central code was written with faulty assumption that the text origin ("baseline, 0" or "0, baseline" depending on writing-mode) has coordinates that are at the correct position already in the final scale. (However, it does not.)

In one example I was looking at, we were getting a ScaleAround call for the rect {x=0,y=-11,width=71,height=128}, being scaled around aPoint==(0,-5.5), with a scale factor of 0.01. This produces a very tiny resulting rect, which is anchored around (0,-5.5) (since that's the point we're scaling around), and in particular it ends up producing the rect {x=0,y=-5.555,width=0.71,height=1.28} which lives entirely in negative territory. I'm guessing that this 0,-5.5 point may very well be the correct the "origin" of this text (i.e. the top of its ascent), when we're dealing with the scale that we started out in -- but after we've scaled, then the 0,-5.5 position isn't really useful anymore, and it's not useful for us to have our tiny rect positioned way off entirely in negative space there.

So: I think Alex's patch (removing this ScaleAround usage, so that we just scale the whole rect around the 0,0 origin) makes sense. I'll trigger a try run with that patch and with my automated test "part 2"-patch, just to be sure we do indeed pass the automated test everywhere after the fix -- and if that goes well, I'll land both patches.

Thanks for the report, Martin, and thanks for the patch, Alex!

Comment 24

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf1634ba788ec0d6c2821ed86824cbace05a6bbd

Comment 25

[Martin]

Thanks for your attentiveness and hard work.

Comment 26

[Alex Henrie]

Yes, thanks Daniel for taking this on!

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb92957787d
part 1: Scale around (0,0) when transforming SVG text from run user space to user space. r=longsonr
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d57aaf5c429
part 2: Add WPT for text being scaled up via SVG viewBox. r=longsonr

Comment 29

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17490 for changes under testing/web-platform/tests

Comment 30

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 31

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/7bb92957787d
https://hg.mozilla.org/mozilla-central/rev/4d57aaf5c429

Comment 32

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Does the user impact of this bug justify backport consideration for 68 or can this fix ride the trains?

Comment 34

[Alex Henrie]

In my opinion this fix can wait until Firefox 69.0, and I'd feel more comfortable with the extra testing anyway.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Thanks for the quick reply. 69 it is then!

Comment 36

[Daniel Holbert [:dholbert]]

(In reply to Alex Henrie from comment #34)

In my opinion this fix can wait until Firefox 69.0, and I'd feel more comfortable with the extra testing anyway.

Yup, I agree with this. Thanks!