Closed
Bug 1115916
Opened 6 years ago
Closed 6 years ago
improve synthetic vertical metrics for fonts that don't have actual vhea/vmtx tables
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: jfkthame, Assigned: jfkthame)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
2.23 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
2.35 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
The code in gfxFont::CreateVerticalMetrics is supposed to synthesize appropriate default metrics when the font does not have actual vertical-metrics tables. However, when an 'OS/2' table is present it does a poor job, which particularly affects non-Mac platforms (many of the standard OS X fonts do not have an OS/2 table, so we use the 'head' table as a fallback, and avoid the issues here). To see the poor results on OS X, compare (a) data:text/html,<div style="writing-mode:vertical-lr;text-orientation:upright;font:32px courier">ABC upright (b) data:text/html,<div style="writing-mode:vertical-lr;text-orientation:upright;font:32px courier new">ABC upright Result: (a) looks OK, as Courier has no OS/2 table; (b) is badly spaced (especially noticeable if you drag-select across some of the text) because it uses the OS/2 table and synthesizes bad metrics from it. Non-Mac platforms will tend to hit cases like (b) with most fonts.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #8541877 -
Flags: review?(smontagu)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•6 years ago
|
||
Here's a reftest to go with this. Using the Ahem font, the background of the <span>s should not show at all as the glyphs completely fill the frame. But prior to the patch here, the middle case (vertical/upright) fails badly.
Attachment #8541901 -
Flags: review?(smontagu)
Assignee | ||
Comment 3•6 years ago
|
||
Reduced the reftest to include only the critical case using upright vertical glyphs; the horizontal-metrics cases (ironically) add too much 'fuzz' due to small subpixel discrepancies. (I think even this will need a small amount of fuzz, once tryserver returns its verdict.)
Attachment #8541955 -
Flags: review?(smontagu)
Assignee | ||
Updated•6 years ago
|
Attachment #8541901 -
Attachment is obsolete: true
Attachment #8541901 -
Flags: review?(smontagu)
Comment 4•6 years ago
|
||
Comment on attachment 8541955 [details] [diff] [review] Reftest for synthetic vertical metrics Review of attachment 8541955 [details] [diff] [review]: ----------------------------------------------------------------- No need to rerequest review if you need to add a little fuzz
Attachment #8541955 -
Flags: review?(smontagu) → review+
Comment 5•6 years ago
|
||
Comment on attachment 8541877 [details] [diff] [review] improve synthetic vertical metrics for fonts that don't have actual vhea/vmtx tables Review of attachment 8541877 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFont.cpp @@ +3395,5 @@ > + (int16_t(os2->sTypoAscender) - int16_t(os2->sTypoDescender)); > + metrics->aveCharWidth = > + std::max(metrics->emHeight, ascentDescent); > + gfxFloat halfCharWidth = > + int16_t(os2->xAvgCharWidth) * gfxFloat(mFUnitsConvFactor) / 2; Is there a particular reason for using half the char-width rather than some other fraction, or is it just black magic? Should we experiment with a range of fonts on different platforms?
Assignee | ||
Comment 6•6 years ago
|
||
Comment on attachment 8541877 [details] [diff] [review] improve synthetic vertical metrics for fonts that don't have actual vhea/vmtx tables Review of attachment 8541877 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFont.cpp @@ +3395,5 @@ > + (int16_t(os2->sTypoAscender) - int16_t(os2->sTypoDescender)); > + metrics->aveCharWidth = > + std::max(metrics->emHeight, ascentDescent); > + gfxFloat halfCharWidth = > + int16_t(os2->xAvgCharWidth) * gfxFloat(mFUnitsConvFactor) / 2; The idea is simply to give the font a (minimum) horizontal extent of xAvgCharWidth, with half of this as the ascent, and half as the descent -- as the vertical metrics are based on a default centered baseline. (In many cases, this won't actually alter things from the default of 0.5em that was set at the start of this function. But it seems worth including for the benefit of fonts with unusual proportions.)
Comment 7•6 years ago
|
||
Comment on attachment 8541877 [details] [diff] [review] improve synthetic vertical metrics for fonts that don't have actual vhea/vmtx tables Review of attachment 8541877 [details] [diff] [review]: ----------------------------------------------------------------- Fair enough, but this code really needs more comments -- it's very hard to parse because it's not clear which of the terms like "width" "height" "ascent" "descent" etc. retain their physical meaning and which have it reversed.
Attachment #8541877 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 8•6 years ago
|
||
Fair point! I added some comments, hoping that'll help. https://hg.mozilla.org/integration/mozilla-inbound/rev/d260f281dfe6 https://hg.mozilla.org/integration/mozilla-inbound/rev/6536d9885d2e
Target Milestone: --- → mozilla37
https://hg.mozilla.org/mozilla-central/rev/d260f281dfe6 https://hg.mozilla.org/mozilla-central/rev/6536d9885d2e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment 10•6 years ago
|
||
Hello, I just visited this testpage http://lxr.mozilla.org/mozilla-central/source/layout/reftests/writing-mode/1115916-1-vertical-metrics.html and http://lxr.mozilla.org/mozilla-central/source/layout/reftests/writing-mode/1115916-1-vertical-metrics-ref.html and the Ahem font is not fetched and I believe it is because it should be "Ahem" and not "ahem" (see lines 8 and 12): <style> @font-face { font-family: ahem; src: url(../fonts/Ahem.ttf); } font: 16px/24px ahem; The error console, CSS tab also reports: " downloadable font: rejected by sanitizer (font-family: "ahem" style:normal weight:normal stretch:normal src index:0) source: http://lxr.mozilla.org/mozilla-central/source/layout/reftests/fonts/Ahem.ttf " The Ahem.ttf font file is there and properly linked. Should I create a separate bug report for this? Gérard
Comment 11•6 years ago
|
||
(In reply to Gérard Talbot from comment #10) > The error console, CSS tab also reports: > " > downloadable font: rejected by sanitizer (font-family: "ahem" style:normal > weight:normal stretch:normal src index:0) source: > http://lxr.mozilla.org/mozilla-central/source/layout/reftests/fonts/Ahem.ttf > " > > The Ahem.ttf font file is there and properly linked. > > Should I create a separate bug report for this? > > Gérard If the sanitizer is rejecting a font it's generally a bug in your font. If you look closely at the error message above, you'll see the problem. The testcase your running on lxr is referencing the lxr-ified version of Ahem.ttf: http://lxr.mozilla.org/mozilla-central/source/layout/reftests/fonts/Ahem.ttf So running the test that way will pull down a *HTML* page, not binary font data, and the sanitizer will quickly reject it.
You need to log in
before you can comment on or make changes to this bug.
Description
•