Bug 198909 - [WinCairo] incorrect font height for 'Google Sans Display' font
Summary: [WinCairo] incorrect font height for 'Google Sans Display' font
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-16 21:30 PDT by Fujii Hironori
Modified: 2019-06-26 16:46 PDT (History)
5 users (show)

See Also:


Attachments
test case (467 bytes, text/html)
2019-06-16 21:30 PDT, Fujii Hironori
no flags Details
[Screenshot] WinCairo port (11.84 KB, image/png)
2019-06-16 21:31 PDT, Fujii Hironori
no flags Details
WIP patch (775 bytes, patch)
2019-06-16 23:22 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (2.36 KB, patch)
2019-06-21 03:02 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (6.68 KB, patch)
2019-06-25 00:59 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (4.64 KB, patch)
2019-06-25 01:01 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (5.25 KB, patch)
2019-06-25 01:02 PDT, Fujii Hironori
ews: commit-queue-
Details | Formatted Diff | Diff
UseTypo.ttx (10.15 KB, text/plain)
2019-06-25 01:14 PDT, Fujii Hironori
no flags Details
Archive of layout-test-results from ews101 for mac-highsierra (3.17 MB, application/zip)
2019-06-25 02:14 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.76 MB, application/zip)
2019-06-25 02:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-highsierra (3.00 MB, application/zip)
2019-06-25 02:51 PDT, Build Bot
no flags Details
Patch (4.91 KB, patch)
2019-06-25 03:46 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2019-06-16 21:30:46 PDT
Created attachment 372231 [details]
test case

[WinCairo] incorrect font height for 'Google Sans Display' font

Google News: https://news.google.com/?hl=en-US&gl=US&ceid=US:en
Comment 1 Fujii Hironori 2019-06-16 21:31:55 PDT
Created attachment 372232 [details]
[Screenshot] WinCairo port
Comment 2 Fujii Hironori 2019-06-16 23:22:29 PDT
Created attachment 372233 [details]
WIP patch
Comment 3 Fujii Hironori 2019-06-17 01:12:11 PDT
It seems that FreeType 2.10 fixed the issue.

[sfnt] Use typo metrics if OS/2 fsSelection USE_TYPO_METRICS bit is set.
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=f72b00746c64e13625cf8371f031411fbd0d6161

Line height of a font is disregarded (#1626) · Issues · GNOME / gtk · GitLab
https://gitlab.gnome.org/GNOME/gtk/issues/1626
Comment 4 Frédéric Wang (:fredw) 2019-06-17 01:32:34 PDT
Can you please write (or find) a WPT test similar to:

https://github.com/web-platform-tests/wpt/blob/master/mathml/tools/use-typo-lineheight.py
https://w3c-test.org/mathml/relations/text-and-math/use-typo-metrics-1.html

(and check whether that one is working on your port)
Comment 5 Fujii Hironori 2019-06-21 03:02:45 PDT
Created attachment 372624 [details]
WIP patch
Comment 6 Fujii Hironori 2019-06-25 00:59:25 PDT
Created attachment 372821 [details]
Patch
Comment 7 Fujii Hironori 2019-06-25 01:01:08 PDT
Created attachment 372822 [details]
Patch
Comment 8 Fujii Hironori 2019-06-25 01:02:27 PDT
Created attachment 372823 [details]
Patch
Comment 9 Frédéric Wang (:fredw) 2019-06-25 01:05:06 PDT
Comment on attachment 372822 [details]
Patch

It would help to have the script to generate UseTypo.ttf or at least explanation on its metrics.
Comment 10 Fujii Hironori 2019-06-25 01:14:49 PDT
Created attachment 372825 [details]
UseTypo.ttx

(In reply to Frédéric Wang (:fredw) from comment #9)
> It would help to have the script to generate UseTypo.ttf or at least
> explanation on its metrics.

Here is the step how I created the UseTypo.ttf.

1. Open Ahem.ttf by FontForge
2. Clear all glyphs but 'O'.
3. Save as UseTypo.ttf
4. ttx UseTypo.ttf
5. Edit UseTypo.ttf manually
   Turn USE_TYPO_METRICS flag on
   Change ascent of hhea to 3000
   Change descent of hhea to 3000
6. ttx UseTypo.ttx

https://github.com/fonttools/fonttools

Should I add a comment in the test case?
Comment 11 Frédéric Wang (:fredw) 2019-06-25 02:07:30 PDT
(In reply to Fujii Hironori from comment #10)
> Created attachment 372825 [details]
> UseTypo.ttx
> 
> (In reply to Frédéric Wang (:fredw) from comment #9)
> > It would help to have the script to generate UseTypo.ttf or at least
> > explanation on its metrics.
> 
> Here is the step how I created the UseTypo.ttf.
> 
> 1. Open Ahem.ttf by FontForge
> 2. Clear all glyphs but 'O'.
> 3. Save as UseTypo.ttf
> 4. ttx UseTypo.ttf
> 5. Edit UseTypo.ttf manually
>    Turn USE_TYPO_METRICS flag on
>    Change ascent of hhea to 3000
>    Change descent of hhea to 3000
> 6. ttx UseTypo.ttx
> 
> https://github.com/fonttools/fonttools
> 
> Should I add a comment in the test case?

I think it would be good to have some a script similar to

https://github.com/web-platform-tests/wpt/blob/master/mathml/tools/use-typo-lineheight.py

so that people know the font metrics and may even modify it in the future. 

Right now it's not obvious to me what are the ascent/descent/linegap and win/hhea/typo values for this font.

Is this test passing before/after your patch:
https://w3c-test.org/mathml/relations/text-and-math/use-typo-metrics-1.html
(I would have expected it fails before and passes after... hence you would actually not need to write a new test) 

Note: Regarding failures on Apple platforms, see bug 150394 comment 12. I'll let Myles comment about this.
Comment 12 Build Bot 2019-06-25 02:14:05 PDT
Comment on attachment 372823 [details]
Patch

Attachment 372823 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12569355

New failing tests:
fonts/use-typo-metrics-2.html
Comment 13 Build Bot 2019-06-25 02:14:07 PDT
Created attachment 372828 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 14 Build Bot 2019-06-25 02:21:36 PDT
Comment on attachment 372823 [details]
Patch

Attachment 372823 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12569396

New failing tests:
fonts/use-typo-metrics-2.html
Comment 15 Build Bot 2019-06-25 02:21:38 PDT
Created attachment 372829 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 16 Fujii Hironori 2019-06-25 02:43:06 PDT
(In reply to Frédéric Wang (:fredw) from comment #11)
> I think it would be good to have some a script similar to
> 
> https://github.com/web-platform-tests/wpt/blob/master/mathml/tools/use-typo-
> lineheight.py
> 
> so that people know the font metrics and may even modify it in the future. 

Got it. Will do.
Should I check the script in the repository? Then, which directory?

> Right now it's not obvious to me what are the ascent/descent/linegap and
> win/hhea/typo values for this font.

You can see them in the ttx file.

> Is this test passing before/after your patch:
> https://w3c-test.org/mathml/relations/text-and-math/use-typo-metrics-1.html
> (I would have expected it fails before and passes after... hence you would
> actually not need to write a new test) 

Unfortunately, this patch doesn't make the test pass. It is filed in Bug 199186.

> Note: Regarding failures on Apple platforms, see bug 150394 comment 12. I'll
> let Myles comment about this.

Good to know. Thanks.
Comment 17 Build Bot 2019-06-25 02:51:30 PDT
Comment on attachment 372823 [details]
Patch

Attachment 372823 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12569446

New failing tests:
fonts/use-typo-metrics-2.html
Comment 18 Build Bot 2019-06-25 02:51:34 PDT
Created attachment 372831 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 19 Frédéric Wang (:fredw) 2019-06-25 03:08:35 PDT
(In reply to Fujii Hironori from comment #16)
> Got it. Will do.
> Should I check the script in the repository? Then, which directory?

I would put it in the same directory as the generated font file, unless there is another place where we have such scripts.

> > Right now it's not obvious to me what are the ascent/descent/linegap and
> > win/hhea/typo values for this font.
> 
> You can see them in the ttx file.

Right but my point is that people shouldn't need to execute programs to review a patch.
Comment 20 Fujii Hironori 2019-06-25 03:46:55 PDT
Created attachment 372833 [details]
Patch
Comment 21 Fujii Hironori 2019-06-25 20:52:32 PDT
Comment on attachment 372833 [details]
Patch

Clearing flags on attachment: 372833

Committed r246831: <https://trac.webkit.org/changeset/246831>
Comment 22 Fujii Hironori 2019-06-25 20:52:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2019-06-26 16:46:25 PDT
<rdar://problem/52218772>