Last Comment Bug 470440 - text spacing is bad in Fennec at non-1:1 zoom levels
: text spacing is bad in Fennec at non-1:1 zoom levels
Status: VERIFIED FIXED
[needs patch][fennec-only]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- major (vote)
: ---
Assigned To: Stuart Parmenter
:
Mentors:
: 458458 511958 517610 (view as bug list)
Depends on: 537411
Blocks: 473596 504390 540788
  Show dependency treegraph
 
Reported: 2008-12-19 09:07 PST by cmtalbert
Modified: 2010-01-19 22:40 PST (History)
23 users (show)
pavlov: blocking1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final-fixed
1.0+


Attachments
disable hinting for mobile (1.94 KB, patch)
2009-12-29 18:10 PST, Stuart Parmenter
no flags Details | Diff | Splinter Review
address review comments (4.13 KB, patch)
2009-12-29 22:32 PST, Stuart Parmenter
karlt: review+
Details | Diff | Splinter Review

Description cmtalbert 2008-12-19 09:07:19 PST
Load any page into fennec that uses standard variable width fonts and note the kerning (space between the letters).  You'll see that letters are oddly spaced.

For example, on bugzilla.mozilla.org, you can note that the a's have too much space after them and the t's have too little (check out t's at the ends of words).  You can see the same effect on cnn.com.

If you zoom in, the kerning is repaired and looks normal.  But at the default zoom level, this makes pages hard to read.

1. Go to site (bugzilla.mozilla.org, cnn.com, etc)

== Actual ==
Kerning is messed up.

== Expected == 
Kerning is displayed properly.  Note that if you zoom in, the kerning is displayed properly, but returning to the default zoom level, the incorrect spacing returns.

I'll try to get some screen shots.

Marking blocking ? because if you can't easily read sites, fennec isn't useful.
Comment 1 cmtalbert 2008-12-19 09:26:35 PST
Doh: version: Mozilla/5.0 (X11; U; Linux armv6l; en-US;rv:1.9.2a1pre) Gecko/20081219 Fennec/1.0a2pre

sorry for spam
Comment 2 Tanner M. Young [:tmyoung] 2009-06-26 13:32:56 PDT
*** Bug 458458 has been marked as a duplicate of this bug. ***
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2009-08-21 17:29:49 PDT
*** Bug 511958 has been marked as a duplicate of this bug. ***
Comment 4 Benjamin Stover (:stechz) 2009-10-16 17:54:38 PDT
Can someone verify this behavior on the latest nightly?  I tried bugzilla and theglobeandmail with no luck.  Has it been fixed?

Since we set zoom level to <1 on websites that have overflows at 800px width (like theglobeandmail and some urls on bugzilla), I wonder if this only occurs at zoom levels less than 1.
Comment 5 Madhava Enros [:madhava] 2009-10-19 10:45:38 PDT
*** Bug 517610 has been marked as a duplicate of this bug. ***
Comment 6 Madhava Enros [:madhava] 2009-10-22 07:19:47 PDT
I think it's still an issue.  Here's what a globeandmail.com page looks like, zoomed in, with today's nightly:
http://www.flickr.com/photos/madhava_work/4034902494/sizes/o/
Comment 7 Aakash Desai [:aakashd] 2009-10-22 08:53:12 PDT
You can also see it when zooming into article snippets on www.google.com/news
Comment 8 Benjamin Stover (:stechz) 2009-10-29 17:08:20 PDT
Just noticed this isn't a problem on Mac!
Comment 9 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-23 08:32:17 PST
Jeff asked, and I'll answer here: I marked this blocking1.9.2+ because it was marked blocking-fennec:1.0+

It may have been fixed a while back; just don't want to lose track.
Comment 10 John Daggett (:jtd) 2009-11-23 11:26:47 PST
Is this something specific to fennec builds or happens also on Linux?  Windows?  Not sure the platform == all makes sense.  It would really help to have a testcase and some screenshots to investigate.
Comment 11 cmtalbert 2009-11-24 11:44:58 PST
I'd like more detail on what the approach is to fix this.

In the platform meeting [1] we discussed that a possible solution would be forking the file that controls this code.  I really really discourage that approach because once you fork that file, we'll lose the ability to test this change as part of the automated regression testing for the general 1.9.2 platform.  The fennec tests are not up to snuff to catch any regression issues that might occur as a result of the change.

What are the options we're considering here?  Is there a WIP patch we can take a look at?

[1]: https://wiki.mozilla.org/Platform/2009-11-24
Comment 12 John Daggett (:jtd) 2009-11-24 16:32:14 PST
(In reply to comment #11)
Clint, please set up a testcase with a page that uses a defined set of fonts that don't display correctly in a given environment.  Please note whether zooming is required or not to produce the problem.  If the problem is on fennec, does the same problem also occur under in a Linux environment?

A couple example test pages using downloadable fonts, these can easily be reworked to use platform fonts:

http://people.mozilla.org/~jdaggett/tests/otfttfkerning.html
http://people.mozilla.org/~jdaggett/tests/waterfall.html

Kerning is usually a post-process of adjusting the advance widths based on the surrounding characters but the screen shot in comment 6 looks really off, I don't think this is just a problem with handling kerning data, something else is off, even if the end result looks like a kerning problem.
Comment 13 Boris Zbarsky [:bz] 2009-12-01 00:37:49 PST
Any ETA here?  This is at this point one of three remaining non-JS core blockers.... and has had no activity at all for a week.
Comment 14 Mike Beltzner [:beltzner, not reading bugmail] 2009-12-01 12:20:28 PST
Stuart's under the weather, but is working on the bug.

I hate to do it, but I'd like to revisit the blocking decision on it. Specifically, from looking at the problem with Madhava, I wonder if the gain from fixing kerning will be enough, or if the font appearance problem runs deeper than just kerning. There appeared to be differences in subpixel rendering when compared side by side with another device like the iPhone
Comment 15 Madhava Enros [:madhava] 2009-12-01 12:21:48 PST
I was asked for another example of the problem, so here:

http://www.flickr.com/photos/madhava_work/4151195104/

some particularly problematic areas called out with flickr notes
Comment 16 Vladimir Vukicevic [:vlad] [:vladv] 2009-12-01 12:35:22 PST
That honestly doesn't look bad to me -- especially since that's zoomed in.  Why would you ever zoom in that much to read text?
Comment 17 Mike Beltzner [:beltzner, not reading bugmail] 2009-12-01 12:39:28 PST
BTW, is the title of this bug inaccurate? Should it be "kerning incorrect when zoomed in" or is it referring to 100% zoom?
Comment 18 Mark Finkle (:mfinkle) (use needinfo?) 2009-12-01 13:14:29 PST
Here is CNN on the Maemo:

Default page load:
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-kerning-defaultzoom.png

Double tap on a paragraph so I can read it:
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-kerning-zoomin.png
Comment 19 Benjamin Stover (:stechz) 2009-12-01 13:17:15 PST
(In reply to comment #17)
> BTW, is the title of this bug inaccurate? Should it be "kerning incorrect when
> zoomed in" or is it referring to 100% zoom?

Kerning us observed when zoomed out as well, as Mark points out.  By default we zoom to fit the page on the screen.
Comment 20 Mike Beltzner [:beltzner, not reading bugmail] 2009-12-01 13:22:21 PST
The CNN shots are certainly more illustrative, and zoomed out it affects small text readability :/

Are we sure kerning alone will fix this?
Comment 21 Stuart Parmenter 2009-12-01 13:42:03 PST
this bug has nothing really to do with kerning
Comment 22 Karl Tomlinson (:karlt) 2009-12-01 14:48:13 PST
The screenshots in comment 18 are both taken with hinting enabled.
How do things look with hinting off?
(This will need to be done for subpixel positioning anyway I assume, so would at least be a cheap partial solution?)
At what size is the text reflowed?
Comment 23 Joe Drew (not getting mail) 2009-12-07 09:10:03 PST
Stuart doesn't consider this a blocker, and I tend to agree, because we can ifdef this out on non-mobile platforms.
Comment 24 Stuart Parmenter 2009-12-07 14:52:21 PST
this is a 1.9.2 blocker, not a firefox 3.6 one.
Comment 25 Mike Beltzner [:beltzner, not reading bugmail] 2009-12-14 21:57:20 PST
Stuart's right, though if this becomes the last blocker for Firefox desktop, then we'll declare 1.9.2 complete and Stuart will put this in a 1.9.2.1 on which Firefox for Maemo will be based. Ah, the fun of branch mechanics!
Comment 26 Stuart Parmenter 2009-12-29 18:10:33 PST
Created attachment 419506 [details] [diff] [review]
disable hinting for mobile
Comment 27 Karl Tomlinson (:karlt) 2009-12-29 18:45:03 PST
Comment on attachment 419506 [details] [diff] [review]
disable hinting for mobile

In PrepareSortPattern():

>@@ -1746,6 +1746,14 @@
>     }
> #endif
> 
>+#ifdef MOZ_GFX_OPTIMIZE_MOBILE
>+    cairo_font_options_t *options = cairo_font_options_create();
>+    cairo_font_options_set_hint_style (options, CAIRO_HINT_STYLE_NONE); // not sure if this takes effect here or not

>+    cairo_font_options_set_hint_metrics (options, CAIRO_HINT_METRICS_OFF); // this doesn't take effect here

Yes, this does nothing here.  Don't bother with this line.

>+    cairo_ft_font_options_substitute(options, aPattern);

Docs for cairo_ft_font_options_substitute say "Options that are already in the
pattern, are not overridden".  In fact, in some options will get overridden
sometimes but that is a bug.
Really you want this before ApplyGdkScreenFontOptions().
Fortunately, CAIRO_HINT_STYLE_NONE will not be affected (overridden) by the
bug.

What might be most clear, though, is to only change
ApplyGdkScreenFontOptions() (instead of PrepareSortPattern()) to add
cairo_font_options_copy() and cairo_font_options_set_hint_style (options,
CAIRO_HINT_STYLE_NONE) after gdk_screen_get_font_options() (with a later
destroy).

In CreateScaledFont():

>@@ -2520,7 +2528,11 @@
>     // font will be used, but currently we don't have different gfxFonts for
>     // different surface font_options, so we'll create a font suitable for the
>     // Screen. Image and xlib surfaces default to CAIRO_HINT_METRICS_ON.
>+#ifdef MOZ_GFX_OPTIMIZE_MOBILE
>+    cairo_font_options_set_hint_metrics (fontOptions, CAIRO_HINT_METRICS_OFF);
>+#else
>     cairo_font_options_set_hint_metrics(fontOptions, CAIRO_HINT_METRICS_ON);
>+#endif

Looks good.

>+#ifndef MOZ_GFX_OPTIMIZE_MOBILE
>     FcBool hinting;
>     if (FcPatternGetBool(aPattern, FC_HINTING, 0, &hinting) != FcResultMatch) {
>         hinting = FcTrue;
>@@ -2578,6 +2591,9 @@
> #endif
>     }
>     cairo_font_options_set_hint_style(fontOptions, hint_style);
>+#else
>+    cairo_font_options_set_hint_style(fontOptions, CAIRO_HINT_STYLE_NONE);
>+#endif

This shouldn't usually be necessary as this is already set in
PrepareSortPattern()/ApplyGdkScreenFontOptions().  It will override user
fontconfig settings if users want to force something different (after
PrepareSortPattern has set the property).  I'll let you choose whether you
want to keep this or not.
Comment 28 Stuart Parmenter 2009-12-29 22:32:56 PST
Created attachment 419530 [details] [diff] [review]
address review comments
Comment 29 Stuart Parmenter 2009-12-30 00:17:14 PST
pushed.  this fixes this bug well enough to close it out, will do rest of subpixel work in another bug.

http://hg.mozilla.org/mozilla-central/rev/1759d1ad0ce1
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/369720551307
Comment 30 Joel Maher ( :jmaher ) 2009-12-31 03:33:20 PST
this looks a lot better with the 20091230 builds on my n810 and n900.

Note You need to log in before you can comment on or make changes to this bug.