Closed
Bug 816614
Opened 12 years ago
Closed 11 years ago
Typography rendering problem: inconsistent character spacing
Categories
(Firefox OS Graveyard :: General, defect, P4)
Tracking
(blocking-basecamp:-, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g1819+ fixed)
People
(Reporter: pabloUX, Assigned: jfkthame)
References
Details
(Whiteboard: visual design, UX-P1, BerlinWW)
Attachments
(6 files)
52.63 KB,
image/jpeg
|
Details | |
56.44 KB,
image/png
|
Details | |
2.46 KB,
patch
|
karlt
:
review-
|
Details | Diff | Splinter Review |
418.05 KB,
image/png
|
Details | |
2.58 KB,
patch
|
justin.lebar+bug
:
review+
cjones
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
56.36 KB,
image/jpeg
|
Details |
[FTE] In Firefox Private choices, text has different spaces between letters. See image attached for visual explanation
Comment 1•12 years ago
|
||
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
Comment 2•12 years ago
|
||
CCing design people
Updated•12 years ago
|
Component: Gaia → Gaia::First Time Experience
Updated•12 years ago
|
Flags: needinfo?(padamczyk)
Reporter | ||
Comment 4•12 years ago
|
||
(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!! :)
Comment 5•12 years ago
|
||
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
Comment 6•12 years ago
|
||
It looks like the glyph metrics are being rounded up. Is it possible to get a reduced test case of the problem?
Assignee | ||
Comment 7•12 years ago
|
||
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?
Assignee | ||
Comment 8•12 years ago
|
||
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? ....
Comment 10•12 years ago
|
||
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)
Comment 11•12 years ago
|
||
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.
Comment 12•12 years ago
|
||
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!
Updated•12 years ago
|
Whiteboard: visual design → visual design UX-P1
Comment 13•12 years ago
|
||
(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
Updated•12 years ago
|
blocking-basecamp: --- → ?
Updated•12 years ago
|
blocking-basecamp: ? → -
Updated•12 years ago
|
Flags: needinfo?(padamczyk)
Comment 14•12 years ago
|
||
jfkthame, are solid STRs sufficient, or do you need a reduced test case?
Assignee | ||
Comment 15•12 years ago
|
||
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...)
Reporter | ||
Comment 16•12 years ago
|
||
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.
Comment 17•12 years ago
|
||
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.
Comment 18•12 years ago
|
||
Renominating. We have text rendering problems throughout the OS, and anything we can do to improve is a high priority quality improvement for v1.
Assignee | ||
Comment 19•12 years ago
|
||
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
Updated•12 years ago
|
Flags: needinfo?(overholt)
Comment 21•12 years ago
|
||
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
Assignee | ||
Comment 22•12 years ago
|
||
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)
Comment 23•12 years ago
|
||
(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 :)
Comment 24•12 years ago
|
||
(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!
Assignee | ||
Comment 25•12 years ago
|
||
Nothing as yet. (The open service-now request is RITM0012580, if you happen to be in a position to chase such things...)
Comment 26•12 years ago
|
||
I'll ping IT directly to expedite the request.
Comment 27•12 years ago
|
||
They're blocked on shipping any devices to non-US addresses until after the new year, because <insert the usual customs problems here>.
Updated•12 years ago
|
Whiteboard: visual design, UX-P1 → visual design, UX-P1, BerlinWW
Assignee | ||
Comment 29•11 years ago
|
||
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.
Assignee | ||
Comment 30•11 years ago
|
||
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)
Assignee | ||
Comment 31•11 years ago
|
||
Left: current b2g build without this patch Right: patched build, showing more consistent character spacing
Assignee | ||
Comment 32•11 years ago
|
||
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.
Assignee | ||
Comment 34•11 years ago
|
||
Hmm, as I suspected, this does lead to a number of reftest failures on android... will be looking into them.
Comment 35•11 years ago
|
||
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-
Assignee | ||
Comment 36•11 years ago
|
||
(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)
Comment 40•11 years ago
|
||
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 41•11 years ago
|
||
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+
Comment 42•11 years ago
|
||
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)
(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?
Assignee | ||
Comment 45•11 years ago
|
||
(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/integration/mozilla-inbound/rev/4c0d5853f72b Filed bug 829523 on final decision for hinting.
Comment 47•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4c0d5853f72b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 48•11 years ago
|
||
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.
tracking-b2g18:
--- → 19+
Reporter | ||
Comment 49•11 years ago
|
||
"a" in "storage" has inconsistent spacing. Is more noticeable in the phone's screen size than zoomed
Reporter | ||
Comment 50•11 years ago
|
||
(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.
Assignee | ||
Comment 51•11 years ago
|
||
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+
Comment 53•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/eabcd2e6919b
status-b2g18:
--- → fixed
status-firefox19:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
Target Milestone: --- → B2G C4 (2jan on)
You need to log in
before you can comment on or make changes to this bug.
Description
•