Closed Bug 1376231 Opened 3 years ago Closed 2 years ago

OpenType "vhal" and "vpal" features unexpectedly expand character spaces

Categories

(Core :: Layout: Text and Fonts, defect, P3)

55 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: baba, Assigned: jfkthame)

Details

Attachments

(3 files)

Attached image vhal-vpal.png
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

1. Install "Noto Serif CJK JP" from https://www.google.com/get/noto/
2. Open https://jsbin.com/duhayezuka/3/edit?html,css,output


Actual results:

Spaces around punctuations and parenthesis should be shrunk with "halt", "palt", "vhal" and "vpal" features.


Expected results:

"halt" and "palt" works correctly in horizontal writing mode. However, "vhal" and "vpal" seems to work reversely.
Component: Untriaged → Layout: Text
Product: Firefox → Core
Priority: -- → P3
OK, I see what's happening here -- it's a disagreement between gecko and harfbuzz about the direction of the vertical coordinates when shaping vertical (upright) text. We're giving harfbuzz glyph advances that are measured downwards (so a normal glyph has a positive advance), but it assumes a positive-upward coordinate system.

Most of the time this doesn't actually matter; after shaping we read the glyph positioning info (provided as advances and offsets) from the shaped text to create the gecko textrun, we end up with glyph advances that work the way gecko expects, and all's good.

However, things break down when there is opentype GPOS positioning that wants to adjust the advances during shaping, because this adjustment will be applied assuming harfbuzz's axis direction, so it's the reverse of what we'd need for our opposite-direction coordinates.

And so the simple fix is to negate the y-coordinate returned by the vertical glyph advance (and origin) accessors we provide, so that they're correct when harfbuzz internally applies any positioning adjustments; then flip them back to gecko's direction when retrieving the positioned glyphs to put into the textrun.
With this, the Noto CJK 'hpal'/'vpal' testcase renders as expected, with the glyph widths being adjusted in the proper direction. (FWIW, a similar bug can be observed with fonts like Hiragino Kaku Gothic ProN on macOS; it's not that this is a peculiarity of the Noto fonts implementation.)
Attachment #8909674 - Flags: review?(jmuizelaar)
Assignee: nobody → jfkthame
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8909674 [details] [diff] [review]
Invert the sign of glyph advance and origin y-coordinates in vertical mode, to match harfbuzz expectations, and then convert the resulting glyph positioning back to gecko's orientation

Review of attachment 8909674 [details] [diff] [review]:
-----------------------------------------------------------------

Can we get a test case for this as well?
Attachment #8909674 - Flags: review?(jmuizelaar) → review+
Yes, I'm intending to create a reftest that covers this.
Here's a reftest based on the reported issue, using a subsetted copy of the NotoSansCJK font. The vertical examples fail with current trunk code, because the incorrect glyph-advance deltas will make the red spans larger than expected and they project beyond the green overlay spans. With the above patch, it now passes as expected.
Attachment #8910246 - Flags: review?(jmuizelaar)
Attachment #8910246 - Flags: review?(jmuizelaar) → review+
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b36ff08b10ee
Invert the sign of glyph advance and origin y-coordinates in vertical mode, to match harfbuzz expectations, and then convert the resulting glyph positioning back to gecko's orientation. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4edd97bf01a
Reftest for GPOS-feature adjustments to vertical glyph advances. r=jrmuizel
Sigh... looks like the test font (subsetted OpenType/CFF font derived from NotoSansCJK) fails to load on Win7 (and naturally, the fallback font we end up using doesn't support the half-width/proportional-width features).

Maybe something about the subsetting process has left the font in a slightly unhappy state. I'll investigate a bit further, though I'm inclined to simply mark the test as failing on Win7 and move on. It works as expected (verifying that the font features are applied properly) on all the platforms where the resource loads at all.
Flags: needinfo?(jfkthame)
Summary: OpenType "vhal" and "vpal" faetrues unintendedly expand character spaces → OpenType "vhal" and "vpal" features unexpectedly expand character spaces
(In reply to Jonathan Kew (:jfkthame) from comment #8)
> Sigh... looks like the test font (subsetted OpenType/CFF font derived from
> NotoSansCJK) fails to load on Win7

It appears that the original (non-subsetted) version of Noto Sans CJKjp also fails to load on Win7. Presumably the Win7 font subsystem is a little too old for something this font is doing.
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea0a7d5c2c2e
Invert the sign of glyph advance and origin y-coordinates in vertical mode, to match harfbuzz expectations, and then convert the resulting glyph positioning back to gecko's orientation. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c886aceae7f
Reftest for GPOS-feature adjustments to vertical glyph advances. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/ea0a7d5c2c2e
https://hg.mozilla.org/mozilla-central/rev/0c886aceae7f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.