129666 - [Xlib] Xlib/Xprint do not scale em-dash & co. correctly...

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Roland Mainz]

[follow-up to bug 85417 ("[Xlib] Xlib/Xprint do not display/print em-dash & co.
correctly...")]
Xlib/Xprint can now display and print special chars like em-dash,dagger etc. but
they are not correctly scaled...

Comment 1

[Roland Mainz]

shanjian:
Any idea what may cause this issue ?

Comment 2

[Roland Mainz]

Attachment: Better testcase with larger fonts

Comment 3

[Roland Mainz]

Attachment: Screenshot of Xlib Mozilla (em-dash,smart-quotes etc. too small)

Comment 4

[Roland Mainz]

Attachment: Screenshot of GTK+ Mozilla (em-dash, smart-quotes etc. correctly sized)

Comment 5

[kill this account]

> Any idea what may cause this issue ?

Could you make a simple test case just showing the em-dash
and set NS_FONT_DEBUG to 3D?

Lets try and find out what the layout layer asked for and what the font code
found.

Comment 6

[Roland Mainz]

Attachment: Xlib Mozilla output viewing attachment 73197 with NS_FONT_DEBUG=0x3D

Log file from debugging session, Xlib toolkit:
-- snip .--
% export NS_FONT_DEBUG=0x3D
% ./mozilla 'http://bugzilla.mozilla.org/attachment.cgi?id=73197&action=view' |
tee /tmp/debug_log_xlib.log
-- snip --

Comment 7

[Roland Mainz]

Attachment: GTK+ Mozilla output viewing attachment 73197 with NS_FONT_DEBUG=0x3D

Log file from debugging session, GTK+ toolkit:
-- snip --
% export NS_FONT_DEBUG=0x3D
% ./mozilla 'http://bugzilla.mozilla.org/attachment.cgi?id=73197&action=view' |
tee /tmp/debug_log_gtk.log
-- snip --

Comment 8

[Roland Mainz]

Attachment: Filtered gdiff file (gdiff -u -w debug_log_xlib.log debug_log_gtk.log)

Filtered gdiff output showing the differences between debug_log_xlib.log and
debug_log_gtk.log ...

Comment 9

[Roland Mainz]

Comparing both screenshots (Xlib is attachment 73199 [details], GTK+ is attachment 73200 [details])
... it looks that the width of the special chars(=em-dash, dagger etc.) is
correctly calculated+scaled - they are only _rendered_ too small (I assume the
rendered size is not scaled up) ...

Comment 10

[Roland Mainz]

rbs:
Any ideas what may go wrong here ?

Comment 11

[kill this account]

The glyphs in attachment 73200 [details] look like they are generated by the X server
from an outline (note the ratty shape of the upper left corner and upper right
of the 'X' in the text "This is an em-dash: X-Y ...").

Looking at the diff between the Gtk version and the Xlib version (attachment 
73463 [details]) I see a lot of 
+outline scaled font: 
-linotype-helvetica-medium-r-normal--0-0-0-0-p-0-iso8859-1, 
../../../../../../../../home/mozilla/src/2002-03-06-08-trunk/mozilla/gfx/src/XXX
/nsFontMetricsXXX.cpp

so I would guess that the Gtk version is seeing these outlines and the Xlib
version is not. Hence the scaled size for the Gtk version and the non scaled
size for the Xlib version (this is all assuming I am not confused).

Comment 12

[kill this account]

PS: use 'xmag' to see the details of the glyph.

Comment 13

[rbs]

Pasting the following information from shanjian so that it doesn't get
forgotten:

> Those special characters were transliterated and displayed using substitute
> font (See function nsFontMetricsXlib::FindStyleSheetGenericFont). Substitute
> font
> is in fact a preloaded western font. I would suggest you to do such
> experiment.
> Could you verify that you have no problem in displaying the transliterated
> characters?
> In another words, em dash (u+2014) will be transliterated to "--" (two minus
> sign, u+002d)
> before display.So try to put "--" directly into your html and try it. I
> think
> it should be displayed OK. The next step we might need to worry about the
> way how substitute font is created. Because it is directly referenced to a
> loaded font instance, it is possible that the font instance is dereferenced
> before
> substitute font is used. An experiment to verify this is to change the
> implementation
> of FindSubstituteFont. Instead of assigning a existing font pointer, call
> "mSubstituteFont =  FindLangGroupPrefFont(gWesternLocale, 'a')", this should
> created a
> new font. If it works this way, you have to figure out the reference problem
> and fix it.

Comment 14

[Roland Mainz]

Attachment: Test patch for 2002-04-27-08-trunk per shanjian's suggestion

This patch seems to fix the problem (PostScript job from Xprint follows...) ...

Comment 15

[Roland Mainz]

Attachment: Attachment 73197 printed via Xprint module (DIN-A4, 300 DPI) + test patch (attachment 81429)

Comment 16

[Roland Mainz]

Attachment: "better" test patch for 2002-04-27-08-trunk

Minor changes to use the |nsFontXlibSubstitute|-wrapper class...

Comment 17

[Roland Mainz]

Attachment: Patch which always returns |nsnull| from |nsFontMetricsXlib::FindSubstituteFont| (fixes the problem, too)

Comment 18

[Roland Mainz]

Attachment: Another patch which fixes the problem by commenting-out |AddToLoadedFontsList(sub_font);|

Comment 19

[rbs]

Next step:

>If it works this way, you have to figure out the reference problem
> and fix it

(in other words, why is that picking an already loaded font is not working?)

Comment 20

[Roland Mainz]

rbs wrote:
> > If it works this way, you have to figure out the reference problem
> > and fix it
> 
> (in other words, why is that picking an already loaded font is not working?)

The font itself seems not to be the wrong one - but it has the wrong size (maybe
scaling is wrong ?!).

I still have no clue what is going wrong here... ;-((

I hope bug 137271 ("Purify checking for Xprint codes") may bring some light into
this issue...

Comment 21

[kill this account]

Erik's design always loaded a font with an 'a' _first_

  nsFontMetricsGTK::Init(...)
  ...
    mWesternFont = FindFont('a');

Is this font the wrong size?

Comment 22

[rbs]

> Erik's design always loaded a font with an 'a' _first_

Nope, it is not necessarily loaded "first". The fonts are loaded according to
the ordering of the CSS font-family list.

What this initial FindFont('a') does is to iterate on that list, loading the
fonts in order, until a font with 'a' is found (and returned as 'the'
ASCII/Western font).

Comment 23

[Roland Mainz]

Attachment: Another workaround which fixes the issue by commenting-out |font = FindStyleSheetGenericFont(aChar);| in |nsFontMetricsXlib::FindFont|

Comment 24

[Roland Mainz]

and always setting |gAllowDoubleByteSpecialChars = PR_FALSE;| fixes this issue,
too...

Comment 25

[Roland Mainz]

Attachment: Patch for 2002-04-27-08-trunk to get this problem "under control" for the 1.0 release

Somehow, for an unknown reason I cannot get this issue under control, I am
still running in circles... ;-((
But I need a fix for this issue for the 1.0 release (urgendly!) ...

... bstell:
Is the attached patch acceptable as workaround for the 1.0 release ?

Comment 26

[Roland Mainz]

Comment on attachment 81616 [details] [diff] [review]
Patch for 2002-04-27-08-trunk to get this problem "under control" for the 1.0 release

This patch still does not fix the problem that glyphs which get transliterated
to '?' due missing fonts are far too small (like on http://arabic.cnn.com/ when
you do not have any arabic fonts)

Comment 27

[kill this account]

> Is the attached patch acceptable as workaround for the 1.0 release ?

So far all the patches I have seen are very likely to have serious unwanted
side effects. (but I know you know that). I would argue against anything like 
attachment 81616 [details] [diff] [review] (or any of the other patches) for the GTK code.

Comment 28

[Roland Mainz]

Attachment: Some debug code to monitor loading of fonts, the resulting font and what is passed to |AddToLoadedFontsList|

The patch generates the following output (viewing the example URL):
-- snip --
# Xlib
#---- LoadFont:
query='-adobe-times-medium-r-normal--17-120-100-100-p-84-iso8859-1',
got='-Adobe-Times-Medium-R-Normal--17-120-100-100-P-84-ISO8859-1'
#---- AddToLoadedFontsList:
'-Adobe-Times-Medium-R-Normal--17-120-100-100-P-84-ISO8859-1'
#---- LoadFont:
query='-adobe-times-medium-r-normal--12-120-75-75-p-64-iso8859-1',
got='-Adobe-Times-Medium-R-Normal--12-120-75-75-P-64-ISO8859-1'
#---- AddToLoadedFontsList:
'-Adobe-Times-Medium-R-Normal--12-120-75-75-P-64-ISO8859-1'
#---- LoadFont:
query='-adobe-times-bold-r-normal--12-120-75-75-p-67-iso8859-1',
got='-Adobe-Times-Bold-R-Normal--12-120-75-75-P-67-ISO8859-1'
#---- AddToLoadedFontsList:
'-Adobe-Times-Bold-R-Normal--12-120-75-75-P-67-ISO8859-1'
#---- AddToLoadedFontsList:
'-Adobe-Times-Medium-R-Normal--17-120-100-100-P-84-ISO8859-1'
#---- LoadFont:
query='-adobe-times-bold-r-normal--34-240-100-100-p-177-iso8859-1',
got='-Adobe-Times-Bold-R-Normal--34-240-100-100-P-177-ISO8859-1'
#---- AddToLoadedFontsList:
'-Adobe-Times-Bold-R-Normal--34-240-100-100-P-177-ISO8859-1'
#---- LoadFont: query='-adobe-times-medium-r-normal--48-*-0-0-p-*-iso8859-1',
got='-adobe-times-medium-r-normal--[48.20 0.00 0.00 48.20]-[48.20 0.00 0.00
48.20]-72-72-p-72-iso8859-1'
#---- AddToLoadedFontsList: '-adobe-times-medium-r-normal--[48.20 0.00 0.00
48.20]-[48.20 0.00 0.00 48.20]-72-72-p-72-iso8859-1'
#---- LoadFont:
query='-adobe-symbol-medium-r-normal--48-*-0-0-p-*-adobe-fontspecific',
got='-adobe-symbol-medium-r-normal--[48.20 0.00 0.00 48.20]-[48.20 0.00 0.00
48.20]-72-72-p-72-adobe-fontspecific'
#---- AddToLoadedFontsList: '-adobe-symbol-medium-r-normal--[48.20 0.00 0.00
48.20]-[48.20 0.00 0.00 48.20]-72-72-p-72-adobe-fontspecific'
#---- AddToLoadedFontsList-2: '-adobe-times-medium-r-normal--[48.20 0.00 0.00
48.20]-[48.20 0.00 0.00 48.20]-72-72-p-72-iso8859-1'
#---- AddToLoadedFontsList:
'-Adobe-Times-Bold-R-Normal--34-240-100-100-P-177-ISO8859-1'
#---- AddToLoadedFontsList:
'-Adobe-Times-Medium-R-Normal--17-120-100-100-P-84-ISO8859-1'
#---- AddToLoadedFontsList: '-adobe-times-medium-r-normal--[48.20 0.00 0.00
48.20]-[48.20 0.00 0.00 48.20]-72-72-p-72-iso8859-1'
#---- AddToLoadedFontsList: '-adobe-symbol-medium-r-normal--[48.20 0.00 0.00
48.20]-[48.20 0.00 0.00 48.20]-72-72-p-72-adobe-fontspecific'
#---- AddToLoadedFontsList-2: '-adobe-times-medium-r-normal--[48.20 0.00 0.00
48.20]-[48.20 0.00 0.00 48.20]-72-72-p-72-iso8859-1'
-- snip --

Diff between Xlib and GTK+ code output looks like this:
-- snip --
--- aaa Fri May  3 18:24:04 2002
+++ bbb Fri May  3 18:22:08 2002
@@ -1,10 +1,10 @@
-# Xlib
+# GTK+
 #---- LoadFont:
query='-adobe-times-medium-r-normal--17-120-100-100-p-84-iso8859-1',
got='-Adobe-Times-Medium-R-Normal--17-120-100-100-P-84-ISO8859-1'
 #---- AddToLoadedFontsList:
'-Adobe-Times-Medium-R-Normal--17-120-100-100-P-84-ISO8859-1'
-#---- LoadFont:
query='-adobe-times-medium-r-normal--12-120-75-75-p-64-iso8859-1',
got='-Adobe-Times-Medium-R-Normal--12-120-75-75-P-64-ISO8859-1'
-#---- AddToLoadedFontsList:
'-Adobe-Times-Medium-R-Normal--12-120-75-75-P-64-ISO8859-1'
-#---- LoadFont:
query='-adobe-times-bold-r-normal--12-120-75-75-p-67-iso8859-1',
got='-Adobe-Times-Bold-R-Normal--12-120-75-75-P-67-ISO8859-1'
-#---- AddToLoadedFontsList:
'-Adobe-Times-Bold-R-Normal--12-120-75-75-P-67-ISO8859-1'
+#---- LoadFont:
query='-linotype-helvetica-medium-r-normal-sans-12-120-72-72-p-65-iso8859-1',
got='-Linotype-Helvetica-Medium-R-Normal-Sans-12-120-72-72-P-65-ISO8859-1'
+#---- AddToLoadedFontsList:
'-Linotype-Helvetica-Medium-R-Normal-Sans-12-120-72-72-P-65-ISO8859-1'
+#---- LoadFont:
query='-linotype-helvetica-bold-r-normal-sans-12-120-72-72-p-67-iso8859-1',
got='-Linotype-Helvetica-Bold-R-Normal-Sans-12-120-72-72-P-67-ISO8859-1'
+#---- AddToLoadedFontsList:
'-Linotype-Helvetica-Bold-R-Normal-Sans-12-120-72-72-P-67-ISO8859-1'
 #---- AddToLoadedFontsList:
'-Adobe-Times-Medium-R-Normal--17-120-100-100-P-84-ISO8859-1'
 #---- LoadFont:
query='-adobe-times-bold-r-normal--34-240-100-100-p-177-iso8859-1',
got='-Adobe-Times-Bold-R-Normal--34-240-100-100-P-177-ISO8859-1'
 #---- AddToLoadedFontsList:
'-Adobe-Times-Bold-R-Normal--34-240-100-100-P-177-ISO8859-1'
-- snip --

Comment 29

[Roland Mainz]

Attachment: Debug log of Xlib fontmetrics

(sorry, putting the diff in the comment form did not work, attaching the
files...)

Comment 30

[Roland Mainz]

Attachment: Debug log of GTK+ fontmetrics

Comment 31

[Roland Mainz]

Attachment: Diff between the two previous log files

Comment 32

[Roland Mainz]

Attachment: gdiff -u -w nsFontMetricsGTK.cpp nsFontMetricsXlib.cpp # The Strings "xlib" and "gtk" have been removed before running gdiff

Differences between nsFontMetricsGTK.cpp and nsFontMetricsXlib.cpp (I removed
the strings "gtk" and "xlib" before running GNU diff to avoid that it lists all
the name difference, too).

I really hope that someone sees the difference which is causing this issue...

Comment 33

[Roland Mainz]

Attachment: gdiff -u -w nsRenderingContextGTK.cpp nsRenderingContextXlib.cpp # The Strings "xlib" and "gtk" have been removed before running gdiff

Differences between nsRenderingContextGTK.cpp and nsRenderingContextXlib.cpp (I
removed the strings "gtk" and "xlib" before running GNU diff to avoid that it
lists all the name difference, too).

Comment 34

[Roland Mainz]

Small list of what is not causing this bug:
- Xlib and Xprint override the nsFontCache class with subclasses... disabling
this does not fix this bug
- Shrinking the size of the GC-cache does not help (GTK+ uses 10 entries,
Xlib/Xprint use 32 entries)

Comment 35

[kill this account]

Attachment: minimal test case

Both Xlib and Gtk think they are loading the right size for the western font.
It looks like the en-dash, dagger, etc. are being transliterated.

Comment 36

[Shanjian Li]

I could not make xlib works after several tries. The code built successful, but
mozilla could not start. I check the code but could not spot any obvious
problem. I hope brian will be able to identify the problem. 

Roland, if you are desparate, using "mSubstituteFont = 
FindLangGroupPrefFont(gWesternLocale, 'a')" might be a acceptable workaround
now. I could not think of any unacceptable sideeffect by doing this. 

Comment 37

[kill this account]

For debug I made all calls to XLoadQueryFont load a particular large font. I
noted that the en-dash was *not* effected. I suspect this may mean that no font
is being set in the GC and it is falling back to fixed.

Comment 38

[Roland Mainz]

Shanjian Li wrote:
> I could not make xlib works after several tries. The code built successful, 
> but mozilla could not start. I check the code but could not spot any obvious
> problem.

Could you file a bug for your problem, assign it to me and CC: timeless, please
?

> Roland, if you are desparate, using "mSubstituteFont = 
> FindLangGroupPrefFont(gWesternLocale, 'a')" might be a acceptable workaround
> now. I could not think of any unacceptable sideeffect by doing this. 

Well, it looks that |FindLangGroupPrefFont(gWesternLocale, 'a')| returns
|nsnull| (weired enougth, making |nsFontMetricsXlib::FindSubstituteFont()|
always return |nsnull| fixes the issue partially - after that "dagger" and the
"smart quotes" are rendered at ~~80% of the expected size - only em-dash is
still far too small).

Comment 39

[Roland Mainz]

Attachment: Patch per shanjian's comment #36

Comment 40

[Roland Mainz]

Attachment: Screenshot from Xlib Mozilla (2002-05-17-08-trunk + patch 84982) viewing testcase attachment 73197

Screenshot from Xlib Mozilla (2002-05-17-08-trunk + patch 84982) viewing
testcase attachment 73197 [details].

Console output looks like this:
-- snip --
warning: property cp1256 already exists
Note: frameverifytree is disabled
WARNING: SetUpFontCharSetInfo: charset = 'ISO-8859-1', file
../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/gfx/src/xlib/nsFontMetricsXlib.cpp,
line 2164
WEBSHELL+ = 3
XXX Damage rectangle (4437,663,272,272) does not intersect the widget's view
(0,0,0,0)!
XXX Damage rectangle (4437,663,272,272) does not intersect the widget's view
(0,0,272,272)!
WARNING: couldn't get widget helper service, file
../../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/xpfe/components/xremote/src/XRemoteService.cpp,
line 368
JavaScript error: 
 line 0: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIXRemoteService.addBrowserInstance]" 
nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://navigator/content/navigator.js :: Startup :: line 451"  data: no]

WEBSHELL+ = 4
WARNING: CSSLoaderImpl::LoadSheet: Load of URL
'chrome://communicator/skin/bookmarks/platformBookmarks.css' failed.  Error
code: 16389, file
../../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/content/html/style/src/nsCSSLoader.cpp,
line 1289
WARNING: SetUpFontCharSetInfo: charset = 'Adobe-Symbol-Encoding', file
../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/gfx/src/xlib/nsFontMetricsXlib.cpp,
line 2164
###!!! ASSERTION: failed to get a special chars substitute font: 'sub_font',
file
../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/gfx/src/xlib/nsFontMetricsXlib.cpp,
line 4823
Break: at file
../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/gfx/src/xlib/nsFontMetricsXlib.cpp,
line 4823
WARNING: SetUpFontCharSetInfo: charset = 'jis_0208-1983', file
../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/gfx/src/xlib/nsFontMetricsXlib.cpp,
line 2164
WARNING: SetUpFontCharSetInfo: charset = 'ISO-8859-8', file
../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/gfx/src/xlib/nsFontMetricsXlib.cpp,
line 2164
WARNING: SetUpFontCharSetInfo: charset = 'ks_c_5601-1987', file
../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/gfx/src/xlib/nsFontMetricsXlib.cpp,
line 2164
###!!! ASSERTION: Attempt to decrement focus controller's suppression when no
suppression active!
: 'PR_FALSE', file
../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/dom/src/base/nsFocusController.cpp,
line 438
Break: at file
../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/dom/src/base/nsFocusController.cpp,
line 438
Document http://bugzilla.mozilla.org/attachment.cgi?id=73197&action=view loaded
successfully
Start reading in bookmarks.html
Finished reading in bookmarks.html  (32301 microseconds)
###!!! ASSERTION: Attempt to decrement focus controller's suppression when no
suppression active!
: 'PR_FALSE', file
../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/dom/src/base/nsFocusController.cpp,
line 438
Break: at file
../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/dom/src/base/nsFocusController.cpp,
line 438
###!!! ASSERTION: failed to get a special chars substitute font: 'sub_font',
file
../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/gfx/src/xlib/nsFontMetricsXlib.cpp,
line 4823
Break: at file
../../../../../../../../home/mozilla/src/2002-05-17-08-trunk/mozilla/gfx/src/xlib/nsFontMetricsXlib.cpp,
line 4823
XXX Damage rectangle (17,4114,2720,3655) does not intersect the widget's view
(0,0,2720,3655)!
-- snip --

Comment 41

[Roland Mainz]

Attachment: Patch to fix this nightmare from hell

Finally I figured out that the |XFontStructPtr()| cast operator in |nsFontXlib|
was causing this nightmare from hell.

Removing it and doing the same as the GTK+ code (e.g.
|mCurrentFont->GetXFontStruct()|) fixes the issue... :-)

Comment 42

[Roland Mainz]

To explain the bug here:
The |XFontStructPtr()| cast operator in |nsFontXlib| was never "overriden" in
|nsFontXlibSubstitute| to return a |XFontStruct *| from the source font of the
substitute font, therefore we got a |nsnull| result from the |XFontStructPtr()|
cast operator (since it looks at the |mFont| member of the
|nsFontXlibSubstitute| object) which means that
|nsRenderingContextXlib::UpdateGC()| never set/updates the font to the one in
|mCurrentFont|.

Removing the special (Xlib-gfx only) |XFontStructPtr()| operator and switching
to the GTK+ gfx code behaviour fixes the issue and should be a low risk patch
since the same code already exists in gfx/src/gtk/ ...

Requesting r=/sr=/a= for attachment 85080 [details] [diff] [review] ...

Comment 43

[kill this account]

Comment on attachment 85080 [details] [diff] [review]
Patch to fix this nightmare from hell

r=[e-mail]

Comment 44

[Scott Collins]

Comment on attachment 85080 [details] [diff] [review]
Patch to fix this nightmare from hell

sr=scc

Comment 45

[Roland Mainz]

Fix checked in into "trunk" by smontagu
(http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1022622000&maxdate=1022622120&who=smontagu%25netscape.com).

----

Requesting approval for 1.0-branch since this bug affects many many pages...

Comment 46

[Roland Mainz]

Drivers refused checkin for 1.0 branch (too late... ;-( ) ... requesting
approval for 1.0.1-branch...

Comment 47

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 85080 [details] [diff] [review]
Patch to fix this nightmare from hell

please check into the 1.0.1 branch ASAP. once landed remove the 
mozilla1.0.1+ keyword and add the fixed1.0.1 keyword

Comment 48

[Simon Montagu :smontagu]

I checked this into the branch.

Comment 49

[Roland Mainz]

Simon Montagu wrote:
> I checked this into the branch.

Thanks!

----

Marking bug as FIXED.

Comment 50

[Roland Mainz]

katakai:
Can you verify the bug for the 1.0.1 branch, please ?