Closed Bug 1544432 Opened 1 year ago Closed 1 year ago

Font Rendering for "Josefin Sans" Broken

Categories

(Core :: SVG, defect)

66 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: community, Assigned: alexhenrie24)

References

(Regression)

Details

(Keywords: regression)

Attachments

(5 files, 3 obsolete files)

Attached image 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.

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

Component: Untriaged → Graphics: Text
Product: Firefox → Core

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.)

Status: UNCONFIRMED → NEW
Component: Graphics: Text → SVG
Ever confirmed: true
Keywords: regression
Regressed by: 1519144
Summary: WOFF2 Font Rendering for "Josefin Sans" Broken → Regression of wrong SVG text element boundaries

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.

Assignee: nobody → alexhenrie24
Summary: Regression of wrong SVG text element boundaries → WOFF2 Font Rendering for "Josefin Sans" Broken

(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?

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

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

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.

(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.

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

*as a file I mean.

Attached file 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.)

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

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.

[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.

Summary: WOFF2 Font Rendering for "Josefin Sans" Broken → Font Rendering for "Josefin Sans" Broken

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.

Flags: needinfo?(dholbert)

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.
Attachment #9073390 - Attachment description: testcase 3 (requires Ahem font to be installed, and about:config pref `nglayout.debug.paint_flashing = true`) → testcase 3 (requires Ahem font to be installed, and about:config pref nglayout.debug.paint_flashing = true)
Flags: needinfo?(dholbert)
Attachment #9073392 - Attachment is obsolete: true

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 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.)
Attachment #9073390 - Attachment is obsolete: true
Attachment #9073393 - Attachment is obsolete: true

(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!

Flags: needinfo?(dholbert)

Thanks for your attentiveness and hard work.

Yes, thanks Daniel for taking this on!

Pushed by dholbert@mozilla.com:
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
Duplicate of this bug: 1545702
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17490 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
See Also: → 1561075

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

Flags: needinfo?(alexhenrie24)
Flags: in-testsuite+

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

Flags: needinfo?(alexhenrie24)

Thanks for the quick reply. 69 it is then!

(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!

Flags: needinfo?(dholbert)
See Also: → 1611450
You need to log in before you can comment on or make changes to this bug.