Bug 131839 (USE_TYPO_METRICS) - The OS/2 USE_TYPO_METRICS fsSelection flag is not taken into account
Summary: The OS/2 USE_TYPO_METRICS fsSelection flag is not taken into account
Status: RESOLVED FIXED
Alias: USE_TYPO_METRICS
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL: http://tests.mathml-association.org/m...
Keywords:
Depends on: 150340 150394 150451
Blocks: 151592 130322
  Show dependency treegraph
 
Reported: 2014-04-17 22:00 PDT by Frédéric Wang (:fredw)
Modified: 2015-11-24 13:27 PST (History)
12 users (show)

See Also:


Attachments
Screenshot of the testcase on epiphany (6.88 KB, image/png)
2015-10-18 04:57 PDT, Frédéric Wang (:fredw)
no flags Details
Patch Freetype (1.49 KB, patch)
2015-10-19 11:39 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch Windows API (2.73 KB, patch)
2015-10-20 11:41 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch Apple API (3.90 KB, patch)
2015-10-20 14:30 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch iOS (4.14 KB, patch)
2015-10-22 22:49 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch iOS 2 (4.57 KB, patch)
2015-10-22 23:25 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch iOS/Mac refactoring (16.40 KB, patch)
2015-11-03 01:40 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch iOS/Mac refactoring (16.40 KB, patch)
2015-11-03 01:46 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch iOS/Mac refactoring (16.39 KB, patch)
2015-11-03 02:17 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch iOS/Mac refactoring (16.42 KB, patch)
2015-11-03 02:42 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch iOS/Mac refactoring (16.48 KB, patch)
2015-11-03 03:07 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-mavericks (689.16 KB, application/zip)
2015-11-03 03:51 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (760.08 KB, application/zip)
2015-11-03 03:54 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (968.01 KB, application/zip)
2015-11-03 03:58 PST, Build Bot
no flags Details
Patch (18.94 KB, patch)
2015-11-03 04:18 PST, Frédéric Wang (:fredw)
darin: review+
Details | Formatted Diff | Diff
Patch (23.15 KB, patch)
2015-11-03 12:29 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (23.36 KB, patch)
2015-11-03 14:17 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2014-04-17 22:00:50 PDT
This is blocking the support for OpenType MATH fonts (bug 130322).

The bug happens to me on Linux, although it does not show up in the MathML code of bug 130322. I'm wondering if the -webkit-display-inline-flex rule in the UA mathml.css stylesheet prevents the bug. However, Gurpreet said it happens on Windows (see attachment 229117 [details]).

Gecko has a similar issue (https://bugzilla.mozilla.org/show_bug.cgi?id=947650) and the suggestion was to "prefer OS/2 sTypo* metrics to hhea ascent/descent if USE_TYPO_METRICS flag is set, and for OpenType Math fonts".

It's not clear to me if that's a browser bug (USE_TYPO_METRICS is ignored) or a font bug (hhea ascent/descent should match the typo metrics). See https://bugzilla.mozilla.org/show_bug.cgi?id=947650#c2

Could someone try

http://www.maths-informatique-jeux.com/ulule/mathml_torture_test/ascent-descent.html

on Mac?
Comment 1 Frédéric Wang (:fredw) 2014-04-18 03:26:55 PDT
I've reported that to STIX:

https://sourceforge.net/p/stixfonts/tracking/53/

and sent a mail to the GUST e-foundry group.
Comment 2 Frédéric Wang (:fredw) 2015-10-18 04:57:22 PDT
Created attachment 263416 [details]
Screenshot of the testcase on epiphany
Comment 3 Frédéric Wang (:fredw) 2015-10-18 05:10:56 PDT
Some update on this, now that the situation is clearer.

1) Sfnt fonts have several vertical metrics:

  a) The "typo" metrics from the OS/2 table
  b) The "Windows" metrics from the OS/2 table
  c) The metrics from the hhea table

Since math fonts contain tall glyphs (e.g. integrals) the metrics from b) are often much larger than those from a). Depending on the fonts, the metrics from c) may also be larger than those from a).

2) The OS/2 table contains a USE_TYPO_METRICS flag to explicitly ask to use the metrics from a). At the time of writing,

  i) This flag is now taken into account on all platforms in Gecko.
  ii) STIX 1.1 does not set this flag. We'll have to wait version 2 which is currently in development.
  iii) All the other fonts with a MATH table sets the flag.

We should probably do the same for WebKit for all the platforms supported. I can at least verify that the flag is not taken into account on Linux (using epiphany) with the following testcase:

http://tests.mathml-association.org/mathml/relations/text-and-math/use-typo-metrics-1.html
Comment 4 Frédéric Wang (:fredw) 2015-10-19 11:39:52 PDT
Created attachment 263497 [details]
Patch Freetype

Here is a quick patch for Linux that seems to work for me with the testcase. We need something similar for WebCore/platform/graphics/cocoa/ and WebCore/platform/graphics/win/...
Comment 5 Frédéric Wang (:fredw) 2015-10-19 14:36:35 PDT
Comment on attachment 263497 [details]
Patch Freetype

Moved to bug 150340.
Comment 6 Frédéric Wang (:fredw) 2015-10-19 14:38:56 PDT
I opened bug 150340 for the FreeType backend. Other places to consider are:

graphics/cocoa/FontCocoa.mm
graphics/win/SimpleFontDataCairoWin.cpp
graphics/win/SimpleFontDataCGWin.cpp
graphics/win/SimpleFontDataWin.cpp
Comment 7 Frédéric Wang (:fredw) 2015-10-20 11:41:22 PDT
Created attachment 263596 [details]
Patch Windows API

Here is a patch for the Windows API.

@Brent, Gurpreet: could you please test it on a Windows machine?

The remaining files use the CGFont API and there does not seem to be any other ways than getting the raw data of the OS/2 table and extracting the fsSelection flags and typo metrics... :-(
Comment 8 Frédéric Wang (:fredw) 2015-10-20 14:30:24 PDT
Created attachment 263616 [details]
Patch Apple API

Untested patch for the Apple API.
Comment 9 Frédéric Wang (:fredw) 2015-10-22 22:49:45 PDT
Created attachment 263903 [details]
Patch iOS
Comment 10 Frédéric Wang (:fredw) 2015-10-22 23:25:33 PDT
Created attachment 263907 [details]
Patch iOS 2
Comment 11 Darin Adler 2015-10-31 15:34:07 PDT
Comment on attachment 263907 [details]
Patch iOS 2

View in context: https://bugs.webkit.org/attachment.cgi?id=263907&action=review

I’m saying review+, but please do *not* land this patch exactly as is.

> Source/WebCore/platform/graphics/ios/FontServicesIOS.mm:38
> +static bool fontHasMathTable(CTFontRef ctFont)

This function already exists in FontCocoa.mm. Please don’t write a second copy of it; instead lets find a way to share the existing function.

> Source/WebCore/platform/graphics/ios/FontServicesIOS.mm:125
> +                if (*(reinterpret_cast<const OpenType::UInt16*>(os2Data + fsSelectionOffset)) & useTypoMetricsMask) {

I suggest writing a helper function to do this messy reinterpret_cast thing that is repeated four times.

> Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:32
> +#if ENABLE(OPENTYPE_MATH)
> +#include "Glyph.h"
> +#endif

I don’t understand why this include needs to be added to a header, when we aren’t touching the header otherwise. If this is needed in FontServicesIOS.mm, then please put it in there.

Also, the header isn’t mentioned in the change log.

Please don’t land this header change!
Comment 12 Frédéric Wang (:fredw) 2015-10-31 23:55:26 PDT
(In reply to comment #11)
> > Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:32
> > +#if ENABLE(OPENTYPE_MATH)
> > +#include "Glyph.h"
> > +#endif
> 
> I don’t understand why this include needs to be added to a header, when we
> aren’t touching the header otherwise. If this is needed in
> FontServicesIOS.mm, then please put it in there.

This is to fix the build failure from attachment 263903 [details]: OpenTypeTypes.h needs to know about the Glyph type since it is used in TableWithCoverage::getCoverageIndex . That change is unrelated to the present bug, but so far all the files including OpenTypeTypes.h also included Glyph.h so the problem did not appear.
Comment 13 Frédéric Wang (:fredw) 2015-11-03 01:40:19 PST
Created attachment 264672 [details]
Patch iOS/Mac refactoring
Comment 14 Frédéric Wang (:fredw) 2015-11-03 01:46:19 PST
Created attachment 264674 [details]
Patch iOS/Mac refactoring
Comment 15 Frédéric Wang (:fredw) 2015-11-03 02:17:00 PST
Created attachment 264678 [details]
Patch iOS/Mac refactoring
Comment 16 Frédéric Wang (:fredw) 2015-11-03 02:42:46 PST
Created attachment 264680 [details]
Patch iOS/Mac refactoring
Comment 17 Frédéric Wang (:fredw) 2015-11-03 03:07:22 PST
Created attachment 264682 [details]
Patch iOS/Mac refactoring
Comment 18 Build Bot 2015-11-03 03:51:31 PST
Comment on attachment 264682 [details]
Patch iOS/Mac refactoring

Attachment 264682 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/376504

New failing tests:
fonts/use-typo-metrics-1.html
mathml/opentype/opentype-stretchy-horizontal.html
mathml/opentype/opentype-stretchy.html
Comment 19 Build Bot 2015-11-03 03:51:35 PST
Created attachment 264686 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 20 Build Bot 2015-11-03 03:54:22 PST
Comment on attachment 264682 [details]
Patch iOS/Mac refactoring

Attachment 264682 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/376505

New failing tests:
fonts/use-typo-metrics-1.html
mathml/opentype/opentype-stretchy-horizontal.html
mathml/opentype/opentype-stretchy.html
Comment 21 Build Bot 2015-11-03 03:54:26 PST
Created attachment 264687 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 22 Build Bot 2015-11-03 03:58:28 PST
Comment on attachment 264682 [details]
Patch iOS/Mac refactoring

Attachment 264682 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/376502

New failing tests:
fonts/use-typo-metrics-1.html
mathml/opentype/opentype-stretchy-horizontal.html
mathml/opentype/opentype-stretchy.html
Comment 23 Build Bot 2015-11-03 03:58:32 PST
Created attachment 264688 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 24 Frédéric Wang (:fredw) 2015-11-03 04:18:48 PST
Created attachment 264689 [details]
Patch
Comment 25 Frédéric Wang (:fredw) 2015-11-03 06:40:33 PST
@Darin: In order to address your review comments I had to refactor the code, so can you please review this new version again?
Comment 26 Darin Adler 2015-11-03 09:31:52 PST
Comment on attachment 264689 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264689&action=review

OK to land like this.

I don’t fully understand why these are in a directory named OpenType, but it’s OK.

> Source/WebCore/platform/graphics/opentype/OpenTypeCoreFoundation.h:26
> +#ifndef OpenTypeCoreFoundation_h

Not sure this file has the right name. These are CoreText and CoreGraphics helper functions; the only role CoreFoundation has is that these other frameworks happen to be based on that framework. I would probably have put these functions into FontCG.h/cpp/mm rather than creating new files. Generally speaking the right suffix would probably be CG.

> Source/WebCore/platform/graphics/opentype/OpenTypeCoreFoundation.h:29
> +#include <CoreFoundation/CoreFoundation.h>

Not sure this include is needed.

> Source/WebCore/platform/graphics/opentype/OpenTypeCoreFoundation.h:34
> +namespace OpenType {

I don’t see a strong reason to put these in an OpenType namespace.

> Source/WebCore/platform/graphics/opentype/OpenTypeCoreFoundation.h:41
> +} // namespace WebCore
> +#endif // OpenTypeCoreFoundation_h

Seems like a missing blank line here.

> Source/WebCore/platform/graphics/opentype/OpenTypeCoreFoundation.mm:27
> +#import "config.h"
> +#import "OpenTypeCoreFoundation.h"

I’m not sure why this file is .mm rather than .cpp.
Comment 27 Frédéric Wang (:fredw) 2015-11-03 10:01:07 PST
@Darin: As I understand the opentype directory / namespace are used for functions to parse the TrueType & OpenType tables which is the case for OS/2 here. Indeed, it should probably be named with "CG" rather than CoreFoundation, sorry about that.

Note that I'm developing on Linux so it's hard to me to know which headers are actually needed, whether I can use the .cpp suffix (and thus use these functions for the WinCG interface too) etc

I'm happy to move these new functions into another shared namespace / directory / file. However, I'm not sure which file "FontCG.h/cpp/mm" you're talking about? I guess I can move this into graphics/Font.h/cpp under a #define.
Comment 28 Darin Adler 2015-11-03 10:34:26 PST
Probably OK like this. Can always move these later if we have a better idea. It’s the sharing that matters.
Comment 29 Frédéric Wang (:fredw) 2015-11-03 10:52:26 PST
OK. However, I'll first give a try to the mm => cpp change and see if that allows to use these functions for WinCG too.
Comment 30 Frédéric Wang (:fredw) 2015-11-03 12:29:06 PST
Created attachment 264712 [details]
Patch
Comment 31 Frédéric Wang (:fredw) 2015-11-03 14:17:50 PST
Created attachment 264723 [details]
Patch
Comment 32 Frédéric Wang (:fredw) 2015-11-04 00:51:30 PST
Committed r192017: <http://trac.webkit.org/changeset/192017>