Closed Bug 321132 Opened 19 years ago Closed 19 years ago

Japanese font grouping is not correct on font pref dialog

Categories

(Core Graveyard :: GFX: Win32, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8.1

People

(Reporter: emk, Assigned: emk)

Details

(4 keywords, Whiteboard: [tjp-dl])

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Patch rv1.0 (obsolete) (deleted) — Splinter Review
Assignee: win32 → VYV03354
Status: NEW → ASSIGNED
Attachment #206526 - Flags: superreview?(rbs)
Attachment #206526 - Flags: review?(rbs)
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.
Attachment #206526 - Attachment is obsolete: true
Attachment #206526 - Flags: superreview?(rbs)
Attachment #206526 - Flags: superreview-
Attachment #206526 - Flags: review?(rbs)
Attachment #206526 - Flags: review-
> 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 :-(
Attached patch Patch rv1.1: more readable (obsolete) (deleted) — Splinter Review
Attachment #206586 - Flags: superreview?(rbs)
Attachment #206586 - Flags: review?(rbs)
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+
Attached patch Patch rv1.2 (deleted) — Splinter Review
Attachment #206586 - Attachment is obsolete: true
Attachment #206633 - Flags: superreview+
Attachment #206633 - Flags: review+
checked-in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Flags: blocking1.8.1?
Flags: blocking1.8.0.2?
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
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
Whiteboard: [tjp-dl]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: