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)
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)
(deleted),
patch
|
emk
:
review+
emk
:
superreview+
rbs
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.2+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
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.
Assignee | ||
Updated•19 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•19 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•19 years ago
|
||
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+
Assignee | ||
Comment 6•19 years ago
|
||
Attachment #206586 -
Attachment is obsolete: true
Attachment #206633 -
Flags: superreview+
Attachment #206633 -
Flags: review+
Comment 7•19 years ago
|
||
checked-in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Attachment #206633 -
Flags: branch-1.8.1?(rbs)
Updated•19 years ago
|
Flags: blocking1.8.1?
Flags: blocking1.8.0.2?
Updated•19 years ago
|
Attachment #206633 -
Flags: approval1.8.0.2?
Attachment #206633 -
Flags: branch-1.8.1?(rbs) → branch-1.8.1+
Comment 8•19 years ago
|
||
checked-in to 1.8 branch.
Updated•19 years ago
|
Keywords: jp-critical
Updated•19 years ago
|
Flags: blocking1.8.0.2? → blocking1.8.0.2+
Comment 9•19 years ago
|
||
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+
Updated•19 years ago
|
Whiteboard: [tjp-dl]
Updated•19 years ago
|
Keywords: fixed1.8.0.2 → verified1.8.0.2
Updated•16 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•