605043 - partial wrong Letter Spacing in Textfields and Tooltips

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[(mostly gone) XtC4UaLL [:xtc4uall]]

Attachment: Screenshot showing the Issue

STR on WinXP with Royal Theme (Tahoma Font 8pt used for Textfields/Dialog Text):

* new profile + set gfx.font_rendering.harfbuzz.level;1 (default since Bug 569531)
* paste "zezezezezezezezezezezezezezezezezezezezezezezezezezezezezezezeze" in a Textfields of your Choice in Fx UI, e.g. Location Bar, Search Bar, Addon-Manager Search Bar

expected:
equal Letter Spacing

actual:
partial bound Letters

I noticed this for Textfields and Tooltips when i went to http://www.customize.org/ where you can see the wrong Spacing with the "ze" Letters.

Setting gfx.font_rendering.harfbuzz.level to 0 hides the Issue.

This regressed within http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=beab39d49de9&tochange=b26489e73818
=> Candidate: Bug 575695

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: round inter-glyph spacing to pixels when glyphs will be pixel snapped

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: test pixel-snapping of glyphs and assumptions re various backend behavior

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 488817 [details] [diff] [review]
round inter-glyph spacing to pixels when glyphs will be pixel snapped

Yep, this seems to make good sense, thanks.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 488817 [details] [diff] [review]
round inter-glyph spacing to pixels when glyphs will be pixel snapped

This is needed for bug 569770 (as well as for restoring the appearance with GDI).

Comment 7

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/38480a41a8e8
http://hg.mozilla.org/mozilla-central/rev/8b1208b3ddef

Comment 8

[Octoploid]

This breaks compilation with --enable-system-cairo on Linux:

c++ -o gfxHarfBuzzShaper.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /var/tmp/aur-firefox-hg/src/mozilla-central/config/gcc_hidden.h -DIMPL_THEBES -DWOFF_MOZILLA_CLIENT -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -DEXCLUDE_SKIA_DEPENDENCIES -DCHROMIUM_MOZILLA_BUILD -DOS_LINUX=1 -DOS_POSIX=1 -I/var/tmp/aur-firefox-hg/src/mozilla-central/ipc/chromium/src -I/var/tmp/aur-firefox-hg/src/mozilla-central/ipc/glue -I../../ipc/ipdl/_ipdlheaders  -I/var/tmp/aur-firefox-hg/src/mozilla-central/gfx/thebes -I. -I../../dist/include -I../../dist/include/nsprpub  -I/var/tmp/aur-firefox-hg/src/mozilla-central/ff-opt-obj/dist/include/nspr -I/var/tmp/aur-firefox-hg/src/mozilla-central/ff-opt-obj/dist/include/nss       -fPIC -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -Wno-long-long -march=native -O2 -pipe -fno-strict-aliasing -fshort-wchar -pthread -pipe -DNDEBUG -DTRIMMED -O3 -fomit-frame-pointer -pthread -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng14   -pthread -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng14   -pthread -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng14 -I/usr/include/gtk-unix-print-2.0   -pthread -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/libpng14     -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/gfxHarfBuzzShaper.pp /var/tmp/aur-firefox-hg/src/mozilla-central/gfx/thebes/gfxHarfBuzzShaper.cpp
GLContext.cpp
/var/tmp/aur-firefox-hg/src/mozilla-central/gfx/thebes/gfxHarfBuzzShaper.cpp: In member function ‘virtual PRBool gfxHarfBuzzShaper::InitTextRun(gfxContext*, gfxTextRun*, const PRUnichar*, PRUint32, PRUint32, PRInt32)’:
/var/tmp/aur-firefox-hg/src/mozilla-central/gfx/thebes/gfxHarfBuzzShaper.cpp:872:14: warning: unused variable ‘rv’
/var/tmp/aur-firefox-hg/src/mozilla-central/gfx/thebes/gfxHarfBuzzShaper.cpp: In function ‘void GetRoundOffsetsToPixels(gfxContext*, PRBool*, PRBool*)’:
/var/tmp/aur-firefox-hg/src/mozilla-central/gfx/thebes/gfxHarfBuzzShaper.cpp:927:14: error: ‘CAIRO_FONT_TYPE_DWRITE’ was not declared in this scope
/var/tmp/aur-firefox-hg/src/mozilla-central/gfx/thebes/gfxHarfBuzzShaper.cpp: In member function ‘nsresult gfxHarfBuzzShaper::SetGlyphsFromRun(gfxContext*, gfxTextRun*, hb_buffer_t*, PRUint32, PRUint32)’:
/var/tmp/aur-firefox-hg/src/mozilla-central/gfx/thebes/gfxHarfBuzzShaper.cpp:1068:26: warning: comparison between signed and unsigned integer expressions
/var/tmp/aur-firefox-hg/src/mozilla-central/gfx/thebes/gfxHarfBuzzShaper.cpp:1075:30: warning: comparison between signed and unsigned integer expressions
/var/tmp/aur-firefox-hg/src/mozilla-central/gfx/thebes/gfxHarfBuzzShaper.cpp:1081:24: warning: comparison between signed and unsigned integer expressions
/var/tmp/aur-firefox-hg/src/mozilla-central/gfx/thebes/gfxHarfBuzzShaper.cpp:1163:67: warning: comparison between signed and unsigned integer expressions
make[5]: *** [gfxHarfBuzzShaper.o] Error 1

CAIRO_FONT_TYPE_DWRITE is not defined in this case (from /usr/cairo/cairo.h):

typedef enum _cairo_font_type {
    CAIRO_FONT_TYPE_TOY,
    CAIRO_FONT_TYPE_FT,
    CAIRO_FONT_TYPE_WIN32,
    CAIRO_FONT_TYPE_QUARTZ,
    CAIRO_FONT_TYPE_USER
} cairo_font_type_t;

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: followup, fix --with-system-cairo build failure

Alternatively, we could use #ifdef XP_WIN around the dwrite case; any preference, Karl?

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 490547 [details] [diff] [review]
followup, fix --with-system-cairo build failure

I'd recommend CAIRO_HAS_DWRITE_FONT.
That way, the code would be correct with system-cairo on XP_WIN (as is this patch), but would still be correct when system-cairo gets DWRITE fonts, and also would not have this unnecessary code on other platforms.

Comment 11

[Jonathan Kew [:jfkthame]]

Yes, that looks like the most sensible choice of symbol.

Pushed with CAIRO_HAS_DWRITE_FONT:
http://hg.mozilla.org/mozilla-central/rev/db24505d74e9