Closed Bug 816614 Opened 7 years ago Closed 7 years ago

Typography rendering problem: inconsistent character spacing

Categories

(Firefox OS Graveyard :: General, defect, P4)

All
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:-, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g1819+ fixed)

RESOLVED FIXED
B2G C4 (2jan on)
blocking-basecamp -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 19+ fixed

People

(Reporter: pabloUX, Assigned: jfkthame)

References

Details

(Whiteboard: visual design, UX-P1, BerlinWW)

Attachments

(6 files)

[FTE] In Firefox Private choices, text has different spaces between letters.
See image attached for visual explanation
Keywords: polish
Whiteboard: visual design
This looks more to me like a Font issue, not specific FTE, so moving it to system.

Unsure of who should take a look
Component: Gaia::First Time Experience → Gaia
CCing design people
Component: Gaia → Gaia::First Time Experience
Flags: needinfo?(padamczyk)
Maybe is a rendering issue or something related to the browser?
(In reply to Sergi from comment #3)
> Maybe is a rendering issue or something related to the browser?

Actually I dont know why it could be happening, but it just happen in this screen, so it's likely something about the code in this screen.

Captain obvious to the rescue!! :)
Definitely a font rendering issue. Joe / Jeff do you have any ideas of what could be causing this?
Flags: needinfo?(padamczyk) → needinfo?(joe)
Priority: -- → P1
It looks like the glyph metrics are being rounded up. Is it possible to get a reduced test case of the problem?
An odd thing about that screenshot is that not all the text seems to suffer from the same problem - the "Learn how…" sentence looks like the same font, but it doesn't have the erratic spacing. So what's the difference in styling between the different sections of the text?
Well... on second look, maybe "Learn how..." does have problems too (compare the endings of "privacy" vs "policy"), but it doesn't seem nearly as bad as the main text block.
That's why i first thought it may be some kind of weird rendering issue. If you check exact words that are used across the text, like "you", it's possible to see that not always the distance between letters is the same. Sometimes it's correct and sometimes not. If there's an issue with the font should not be the problem reproduced in all the text the same way? ....
Patryk, we have various ideas about what could possibly be causing this, but a minimalish test case would be very helpful. Since I have no idea who the right person to provide such a thing, back to you for dispensation.
Flags: needinfo?(joe) → needinfo?(padamczyk)
I am sure a qualified developer can provide a test case with more ease than a UX/visual designer. Please assign the bug to an engineer.
I agree! But I have no idea who that person would be. That's why I asked Patryk for that info.

Alternately, that might have been directed towards Patryk - if so, please ignore with my apologies!
Whiteboard: visual design → visual design UX-P1
(In reply to Andreas Gal :gal from comment #11)
> I am sure a qualified developer can provide a test case with more ease than
> a UX/visual designer. Please assign the bug to an engineer.

Dietrich and/or David, can you suggest a dev for this?

This is a strong UX must-fix for v1, because of degree of negative impact (causes the type to look incredibly shoddy and cheap) and scope (system wide).
Summary: [FTE] In Firefox Private choices, text has different spaces between letters → Typography rendering problem: inconsistent character spacing
Whiteboard: visual design UX-P1 → visual design, UX-P1
blocking-basecamp: --- → ?
blocking-basecamp: ? → -
Flags: needinfo?(padamczyk)
jfkthame, are solid STRs sufficient, or do you need a reduced test case?
A reduced test case would surely be appreciated. (Though I don't yet have a b2g device, so until that arrives I'm a bit limited as far as digging in to this personally...)
I've found another example of the same problem in other app (settings). As you can see, space between characters isn't too big, but is noticeable specially when you can compare with the same word one line under.
I believe you can see the same bug in any password field; the asterisks that replace the letters you type occasionally have extra space between them.
Renominating. We have text rendering problems throughout the OS, and anything we can do to improve is a high priority quality improvement for v1.
blocking-basecamp: - → ?
Component: Gaia::First Time Experience → Gaia
Keywords: polish
I'd agree that the rendering shown here would be unacceptable for a released product.

I'm pretty sure this has something to do with incorrectly rounding glyph metrics/positions, but I'm not sure why we're seeing problems in b2g but not on android. Once I get hold of a device for testing, I'm hoping to look into this (if it hasn't been solved by then), but a recent attempt to bring it up in an emulator got bogged down...
Gaia triage: moving from Gaia to Gecko since it's a platform issue.
Component: Gaia → General
Flags: needinfo?(overholt)
I spoke with Patryk about this.  While we'd really, really like to get this fixed, we decided that we can't block on it.  But that doesn't mean we don't want to see a fix!  Hence the "soft blocker" P4/bb-, the next best thing to a blocker.

Jonathan, is there a device on its way to you?  Does the B2G desktop build exhibit the same behaviour?
Assignee: nobody → jfkthame
blocking-basecamp: ? → -
Flags: needinfo?(overholt) → needinfo?(jfkthame)
Priority: P1 → P4
I've requested a device through service-now, so hope it'll be here fairly soon.

(I just tried downloading a b2g-desktop nightly, but after displaying "mozilla" at the bottom of the window for a little while, it just remains stubbornly black...)
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #22)
> I've requested a device through service-now, so hope it'll be here fairly
> soon.

Okay, please email me if you don't get it very soon and I'll do something to get you one ASAP.
 
> (I just tried downloading a b2g-desktop nightly, but after displaying
> "mozilla" at the bottom of the window for a little while, it just remains
> stubbornly black...)

You may be interested in bug 813230 :)
(In reply to Jonathan Kew (:jfkthame) from comment #19)
> I'd agree that the rendering shown here would be unacceptable for a released
> product.
> 
> I'm pretty sure this has something to do with incorrectly rounding glyph
> metrics/positions, but I'm not sure why we're seeing problems in b2g but not
> on android. Once I get hold of a device for testing, I'm hoping to look into
> this (if it hasn't been solved by then), but a recent attempt to bring it up
> in an emulator got bogged down...

Hi Jonathon, did someone bring you a Unagi present for the holidays? ;) If so, have you had a chance to take a look at this? We're really, really like to get it resolved for v1, as you know!
Nothing as yet. (The open service-now request is RITM0012580, if you happen to be in a position to chase such things...)
I'll ping IT directly to expedite the request.
They're blocked on shipping any devices to non-US addresses until after the new year, because <insert the usual customs problems here>.
Whiteboard: visual design, UX-P1 → visual design, UX-P1, BerlinWW
Duplicate of this bug: 828712
Status update: I've just received a b2g device and am beginning to investigate this. I think I know what the basic issue is, so hope to have a patch up shortly.
AFAICT, with the freetype backend, glyph positions always snap to whole pixels, even if hinting is disabled. Hence we should be returning aRoundX=true from GetRoundOffsetsToPixels regardless of the hint_metrics setting; even unhinted glyphs will still be pixel-snapped. This results in noticeably more even glyph spacing in b2g. Pushed a tryserver job to check for any unit-test surprises... https://tbpl.mozilla.org/?tree=Try&rev=ed247057cfdd.
Attachment #700325 - Flags: review?(karlt)
Left: current b2g build without this patch
Right: patched build, showing more consistent character spacing
E.g., look at words "administration", "expresses", "impact", all in the first paragraph; compare "administration" in the first and second paragraphs (in the left-hand screenshot, rendering of the word is inconsistent, although the two instances should be identical).
\o/

Confirmed fixed in b2g builds.
Hmm, as I suspected, this does lead to a number of reftest failures on android... will be looking into them.
Comment on attachment 700325 [details] [diff] [review]
with freetype fonts, glyph positions should always snap to pixels regardless of hinting

(In reply to Jonathan Kew (:jfkthame) from comment #30)
> with freetype fonts, glyph positions should always snap to pixels regardless
> of hinting
> 
> AFAICT, with the freetype backend, glyph positions always snap to whole
> pixels, even if hinting is disabled.

Glyph positions, even for FreeType fonts, are not and should not be snapped to pixels with print/vector backends.

I expect CAIRO_HINT_METRICS_OFF (which is fairly independent from hinting) should not be set for these fonts.

That was added for Android because it shaped/measured at one size and drew at a different size.  There, making offsets pixel-aligned at the measuring size did not help at the drawing size, only making things worse.

I would have thought this change would be correctly avoiding CAIRO_HINT_METRICS_OFF in the right circumstances:
http://hg.mozilla.org/mozilla-central/rev/d13825a03073

Do you know why CAIRO_HINT_METRICS_OFF is still set here?
Attachment #700325 - Flags: review?(karlt) → review-
(In reply to Karl Tomlinson (:karlt) from comment #35)
> Glyph positions, even for FreeType fonts, are not and should not be snapped
> to pixels with print/vector backends.
> 
> I expect CAIRO_HINT_METRICS_OFF (which is fairly independent from hinting)
> should not be set for these fonts.
> 
> That was added for Android because it shaped/measured at one size and drew
> at a different size.  There, making offsets pixel-aligned at the measuring
> size did not help at the drawing size, only making things worse.

Presumably the same consideration applies to zoomable web-browser content in b2g, but not to the main Gaia interface.

> I would have thought this change would be correctly avoiding
> CAIRO_HINT_METRICS_OFF in the right circumstances:
> http://hg.mozilla.org/mozilla-central/rev/d13825a03073
> 
> Do you know why CAIRO_HINT_METRICS_OFF is still set here?

gfxAndroidPlatform::FontHintingEnabled() was modified again in bug 802366:

https://hg.mozilla.org/mozilla-central/diff/f69c06692358/gfx/thebes/gfxAndroidPlatform.cpp

and AFAICT it currently seems to be broken. It's returning FALSE for almost everything being rendered in Gaia, because ContentChild::GetSingleton()->HasOwnApp() returns FALSE. cjones, is that expected? Should this be distinguishing between the main Gaia interface (homescreen, first-run panels, etc) and the web browser? Between the web browser and other apps?
jlebar, bug 802366 made ContentChild a TabContext but never hooked it up properly.  I'm not sure it really makes sense for ContentChild to be a tab context since it can contain multiple contexts.  So the patch above restores the previous behavior.

It would, though, be cleaner to check the ScrollingBehavior helper we introduced later instead of !IsForBrowser().  If you want to fight for ContentChild : TabContext, please do, otherwise I prefer the "backout" ;).
I just caught up with Patryk, and he said font rendering *improved* by his reckoning about 2 months ago ... which is exactly when hinting regressed.

We all of course want correct glyph spacing.

So with attachment 700325 [details] [diff] [review], we can choose between hinting + good-spacing and no-hinting + good-spacing.  I'll grab a bag of popcorn and let visual design and font folks fight that one out ;).
Flags: needinfo?(padamczyk)
I think this was a (really bad) mistake; ContentChild should not implement TabContext.

If we really wanted to use TabContext, we could iterate over all our TabChild's and look at their HasOwnApp()'s, but I don't think that's necessary, since we have mIsForApp already.
Comment on attachment 700958 [details] [diff] [review]
Fix IsForBrowser() regression

This is right, issues in comment 39 aside.  I'm really sorry about this.
Attachment #700958 - Flags: review+
I had a bug for the hinting a few months back (https://bugzilla.mozilla.org/show_bug.cgi?id=804147). See examples of pre and post hinting rendering within. The main issue is that our current hinting snaps to the nearest pixel which causes the fonts to look too sharp and even distorts certain glyphs like the "e" at different sizes.

My ideal solution is keep the hinting off (atleast at our current shipping resolution) and keep the correct glyph spacing on.
Flags: needinfo?(padamczyk)
See Also: → 816614
(In reply to Justin Lebar [:jlebar] from comment #41)
> Comment on attachment 700958 [details] [diff] [review]
> Fix IsForBrowser() regression
> 
> This is right, issues in comment 39 aside.  I'm really sorry about this.

No worries; no one can be blamed for regressing something that's not tested (to a first approximation).
Comment on attachment 700958 [details] [diff] [review]
Fix IsForBrowser() regression

Very safe patch that fixes a regression and allows font hinting to be re-enabled.
Attachment #700958 - Flags: approval-mozilla-b2g18?
(In reply to Patryk Adamczyk [:patryk] UX from comment #42)
> I had a bug for the hinting a few months back
> (https://bugzilla.mozilla.org/show_bug.cgi?id=804147). See examples of pre
> and post hinting rendering within. The main issue is that our current
> hinting snaps to the nearest pixel which causes the fonts to look too sharp
> and even distorts certain glyphs like the "e" at different sizes.
> 
> My ideal solution is keep the hinting off (atleast at our current shipping
> resolution) and keep the correct glyph spacing on.

It sounds like as soon as we land the IsForBrowser() regression-fix here, you're going to want a followup bug explicitly about turning off hinting globally for b2g. And then in that bug, we'll still need to address the fact that we don't handle glyph spacing properly when hinting is turned off.
https://hg.mozilla.org/mozilla-central/rev/4c0d5853f72b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
This change is not blocking-basecamp:+ or blocking-b2g:tef+, so we'll default to resolving in v1.0.1 first. You can either land on shira already (will be merged in) or wait for mozilla-b2g18 to be opened up to v1.0.1 changes.
Blocks: 829523
"a" in "storage" has inconsistent spacing. Is more noticeable in the phone's screen size than zoomed
(In reply to Alex Keybl [:akeybl] from comment #48)
> This change is not blocking-basecamp:+ or blocking-b2g:tef+, so we'll
> default to resolving in v1.0.1 first. You can either land on shira already
> (will be merged in) or wait for mozilla-b2g18 to be opened up to v1.0.1
> changes.

I still see inconsistent character spacing in many places. For example see image attached ("a" in "storage" has inconsistent spacing). In large paragraphs is more noticeble.
The patch here has landed on mozilla-central, but not on mozilla-b2g18 yet, so it's expected that you'd still see problems in b2g builds based on that tree. (If you make a build using gecko from mozilla-central instead, it should be fixed.)
Comment on attachment 700958 [details] [diff] [review]
Fix IsForBrowser() regression

Blocks bug 829523.
Attachment #700958 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
You need to log in before you can comment on or make changes to this bug.