The default bug view has changed. See this FAQ.

Japanese font grouping is not correct on font pref dialog

RESOLVED FIXED in mozilla1.8.1

Status

Core Graveyard
GFX: Win32
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

(4 keywords)

Trunk
mozilla1.8.1
x86
Windows XP
fixed1.8.1, intl, jp-critical, verified1.8.0.2
Bug Flags:
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tjp-dl])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Steps to reproduce:
1. Tools-Options-Content-Fonts & Colors-Advanced.
2. Select Japanese from "Fonts for".
3. Open "Serif"/"Sans-serif"/"Monospace" drop-down lists.
Actual result:
"MS Mincho" is listed in the first group of "Serif" drop-down list.
"MS PGothic" and "MS UI Gothic" are listed in the first group of "Monospace" drop-down list.
Expected result:
"MS Mincho" should be listed in the first group of "Monospace" drop-down list.
"MS PGothic" and "MS UI Gothic" should be listed in the first group of "Sans-serif" drop-down list.
(Assignee)

Comment 1

11 years ago
Created attachment 206526 [details] [diff] [review]
Patch rv1.0
Assignee: win32 → VYV03354
Status: NEW → ASSIGNED
Attachment #206526 - Flags: superreview?(rbs)
Attachment #206526 - Flags: review?(rbs)
Keywords: intl

Comment 2

11 years ago
This patch is hard to read.

For ease of reference here is what the MSDN doc says:
=========
FF_DONTCARE:
Use default font.

FF_ROMAN:
Fonts with variable stroke width (proportional) and with serifs. MS Serif is an example.

FF_MODERN:
Fonts with constant stroke width (monospace), with or without serifs. Monospace fonts are usually modern. Pica, Elite, and CourierNew are examples.

FF_SWISS:
Fonts with variable stroke width (proportional) and without serifs. MS Sans Serif is an example.

FF_SCRIPT:
Fonts designed to look like handwriting. Script and Cursive are examples.

FF_DECORATIVE:
Novelty fonts. Old English is an example.
=========

So, we can very well have monospace-serif fonts and monospace-sans-serif fonts, but for them to be variable-pitch is odd... It seems that it is the Japanese fonts that are peculiar. (We have seen so far that they seem to have unusual settings/metrics.)

Anyway, my reading of your patch is that what you want is to treat any font as a monospace font if it has the fixed-pitch flag, and treat any monospace-sans-serif font as a sans-serif font.

Can you code exactly what you want outside the |switch|, leaving the switch as a fall through path if you criteria are not met. It will make the code easier to read and check for review.
(Assignee)

Updated

11 years ago
Attachment #206526 - Attachment is obsolete: true
Attachment #206526 - Flags: superreview?(rbs)
Attachment #206526 - Flags: superreview-
Attachment #206526 - Flags: review?(rbs)
Attachment #206526 - Flags: review-
(Assignee)

Comment 3

11 years ago
> For ease of reference here is what the MSDN doc says:
> (snip)
Please notice the word "stroke width". Stroke width doesn't always agree with pitch. We may assume that fonts having constant stroke width are also fixed pitch in Western typography world, but Japanese typography follows the different manner.

"MS Mincho" and "MS PGothic" are only a couple of examples among many.
AFAIK all Japanese Mincho fonts belong to FF_ROMAN even if they are fixed pitch because they have variable stroke width (unlike Courier New).
And all Japanese Gothic fonts belong to FF_MODERN even if they are variable pitch because they have constant stroke width (unlike Arial).

Some documents say that Japanese fonts only use FF_ROMAN(Mincho) or FF_MODERN(Gothic), but I don't quote because unfortunately all of them were written in Japanese :-(
(Assignee)

Comment 4

11 years ago
Created attachment 206586 [details] [diff] [review]
Patch rv1.1: more readable
Attachment #206586 - Flags: superreview?(rbs)
Attachment #206586 - Flags: review?(rbs)

Comment 5

11 years ago
Comment on attachment 206586 [details] [diff] [review]
Patch rv1.1: more readable

r+sr=rbs, much better.

+  PRUint8 family = aFont->logFont.lfPitchAndFamily & 0xF0;
+  PRUint8 pitch = aFont->logFont.lfPitchAndFamily & 0xF;

nit: 0x0F gives a nicer symmetry:
Attachment #206586 - Flags: superreview?(rbs)
Attachment #206586 - Flags: superreview+
Attachment #206586 - Flags: review?(rbs)
Attachment #206586 - Flags: review+
(Assignee)

Comment 6

11 years ago
Created attachment 206633 [details] [diff] [review]
Patch rv1.2
Attachment #206586 - Attachment is obsolete: true
Attachment #206633 - Flags: superreview+
Attachment #206633 - Flags: review+
checked-in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Attachment #206633 - Flags: branch-1.8.1?(rbs)
Flags: blocking1.8.1?
Flags: blocking1.8.0.2?
Attachment #206633 - Flags: approval1.8.0.2?

Updated

11 years ago
Attachment #206633 - Flags: branch-1.8.1?(rbs) → branch-1.8.1+
checked-in to 1.8 branch.
Flags: blocking1.8.1?
Keywords: fixed1.8.1
Target Milestone: --- → mozilla1.8.1

Updated

11 years ago
Keywords: jp-critical
Flags: blocking1.8.0.2? → blocking1.8.0.2+
Comment on attachment 206633 [details] [diff] [review]
Patch rv1.2

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #206633 - Flags: approval1.8.0.2? → approval1.8.0.2+
checked-in to 1.8.0 branch too. Thanks.
Keywords: fixed1.8.0.2

Updated

11 years ago
Whiteboard: [tjp-dl]
Keywords: fixed1.8.0.2 → verified1.8.0.2
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.