364678 - Incorrectly styled text after bug 362665 (only on western page??)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[philippe (part-time)]

All text in webpages and chrome of Minefield is displayed using 'Helvetica'.
Styling of text on Webpages using CSS: font-family: XXX (where XXX is the font name) is not applied.

Behaviour changed between the 2006122119 build and the 2006122120
works OK
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a2pre) Gecko/20061221 Minefield/3.0a2pre ID:2006122119 [cairo]
Fails
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a2pre) Gecko/20061221 Minefield/3.0a2pre ID:2006122120 [cairo]

--> bug 362665

Comment 1

[philippe (part-time)]

Attachment: basic test case for font-styling

Each paragraph should be styled as specified.
All of them are styled with 'Helvetica' except one: 'georgia'

Comment 2

[philippe (part-time)]

Attachment: screenshot of testcase

Side by Side
Minefield (build 2006122120) on the left.
Camino trunk build (build 2006122117 (1.2+)) on the right. 

That Camino build predates the Landing of the patch in bug 362665 by a few hours

Comment 3

[philippe (part-time)]

Attachment: Screenshot of Chrome part

Note the incorrect alignment of text on the bookmark bar

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

On my environment, I cannot reproduce this bug.
Please wait.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: logging patch

Would you attach the log by this patch?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: logging patch

Oops, sorry. I attached the wrong file.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Wow, I can reproduce with changing the page charset to western in the testcase.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that the resolver works fine.
The cause may be font switching by ATSUI on gfxAtsuiFont.cpp

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The previous comment is wrong. I find a easy mistake.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

I forgot to cache the postscript name that is using for finding the atsui font id in |FindFromSystem|.
But I don't know why it works fine with non-western encodings...

# I need to leave my desk. After this review, please check-in this patch if anybody can do it.

Comment 11

[philippe (part-time)]

Attachment: log per comment 6

I'm a bit late/slow... :-)
Requested log. As you guessed, can't find those fonts

(In reply to comment #10)
>
Glad you found it. I'll try this as soon as possible

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #11)
> Created an attachment (id=249421) [edit]
> log per comment 6
> 
> I'm a bit late/slow... :-)
> Requested log. As you guessed, can't find those fonts

Wow, your list doesn't contain the 'Times' and 'Lucida Grande'. Did you uninstall the fonts??

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Or, your these fonts don't have the postscript name.
I cannot find the fonts they are not having the postscript name, I may need to care the fonts.(This bug cannot be fixed by the patch.)

Comment 14

[philippe (part-time)]

(In reply to comment #12)
> (In reply to comment #11)
> > Created an attachment (id=249421) [edit]

> 
> Wow, your list doesn't contain the 'Times' and 'Lucida Grande'. Did you
> uninstall the fonts??
> 
No, all fonts are there in their default location.

(10.4.8 PPC)

I loaded an other page from my development server. That page uses most of the roman fonts available on OS X/10.4. The log shows that Minefield can't find any of them.

Comment 15

[philippe (part-time)]

(In reply to comment #13)
> Or, your these fonts don't have the postscript name.
> I cannot find the fonts they are not having the postscript name, I may need to
> care the fonts.(This bug cannot be fixed by the patch.)
> 

According to FontBook.app, the postscript name for Lucida Grande is 'LucidaGrande', the bold face is 'LucidaGrande-Bold'.
Times is 'Times-Roman'

I had a script somewhere to output a list of all fonts. I'll try to find it back.

Comment 16

[philippe (part-time)]

(In reply to comment #15)

> I had a script somewhere to output a list of all fonts. I'll try to find it
> back.
> 

Found it, Font Tools, form Apple
http://developer.apple.com/textfonts/download/

I'll attach the list next.

Comment 17

[philippe (part-time)]

Attachment: list of fonts

Result of ftxinstalledfonts -fpq > ~/Desktop/installedfontsPSQD.txt

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 249419 [details] [diff] [review]
Patch rv1.0

we need to care the non-postscript font(?).

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1

philippe:

Would you test this patch? And would you attach the log?

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

I understand what happens. Because I changed the system locale to English, I can reproduce the bug of failing to list up the all fonts.

The converting by CFString doesn't work fine. I don't know the reason. Now, I use TCE for converting. Because old gfx uses this.

And I found another bug. If the (n)th font has font family name that is same as the name of (n+1)th or later fonts, the resolver returns (n)th font for the name. I add the new member mFamilyNames that is linking to postscript name. See for the detail of the members.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #20)
> the name of (n+1)th or later fonts, the resolver returns (n)th font for the
> name. I add the new member mFamilyNames that is linking to postscript name. See
> for the detail of the members.

"See the header file for the detail of the members."
     ^^^^^^^^^^^^^^^

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0.1

Oops, there is a unused variable.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 249458 [details] [diff] [review]
Patch rv2.0.1

Looks good again, I think; though once again, I didn't test.  Btw, "localized" in your comments, not "localzed" :)

Comment 24

[philippe (part-time)]

Patch rv2.0.1
In a  quick first testing: this works fine. !!
The test case attached here displays correctly. The text in the chrome part of the browser displays correctly (bookmarks bar, Preference window, and more).

Thank you Masayuki-san.

However, some Japanese sites (like http://mozillazine.jp/) still don't display correctly from my end. But I think that is another bug (maybe reopen 357555 for that ?).

Comment 25

[philippe (part-time)]

Attachment: screenshot

from http://mozillazine.jp/
patched Minefield, English OS 10.4.8
I see the same problem in a tab title when it contains Japanese characters

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.1 (for checking-in)

Thank you.
# I added a new condition for an assertion.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in, thank you.

philippe:

Thank you for your testing! I will file a bug for the font switching. If you find the font switching bug, please note the URLs. Thanks.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.1 (for checking-in)

Oops, I forgot to type a diff command.