Bug 131839 – The OS/2 USE_TYPO_METRICS fsSelection flag is not taken into account

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Frédéric Wang (:fredw)]

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?

[Frédéric Wang (:fredw)]

I've reported that to STIX:

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

and sent a mail to the GUST e-foundry group.

[Frédéric Wang (:fredw)]

Created attachment 263416 [details]
Screenshot of the testcase on epiphany

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

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/...

[Frédéric Wang (:fredw)]

Comment on attachment 263497 [details]
Patch Freetype

Moved to bug 150340.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

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... :-(

[Frédéric Wang (:fredw)]

Created attachment 263616 [details]
Patch Apple API

Untested patch for the Apple API.

[Frédéric Wang (:fredw)]

Created attachment 263903 [details]
Patch iOS

[Frédéric Wang (:fredw)]

Created attachment 263907 [details]
Patch iOS 2

[Darin Adler]

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!

[Frédéric Wang (:fredw)]

(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.

[Frédéric Wang (:fredw)]

Created attachment 264672 [details]
Patch iOS/Mac refactoring

[Frédéric Wang (:fredw)]

Created attachment 264674 [details]
Patch iOS/Mac refactoring

[Frédéric Wang (:fredw)]

Created attachment 264678 [details]
Patch iOS/Mac refactoring

[Frédéric Wang (:fredw)]

Created attachment 264680 [details]
Patch iOS/Mac refactoring

[Frédéric Wang (:fredw)]

Created attachment 264682 [details]
Patch iOS/Mac refactoring

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Frédéric Wang (:fredw)]

Created attachment 264689 [details]
Patch

[Frédéric Wang (:fredw)]

@Darin: In order to address your review comments I had to refactor the code, so can you please review this new version again?

[Darin Adler]

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.

[Frédéric Wang (:fredw)]

@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.

[Darin Adler]

Probably OK like this. Can always move these later if we have a better idea. It’s the sharing that matters.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 264712 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 264723 [details]
Patch

[Frédéric Wang (:fredw)]

Committed r192017: <http://trac.webkit.org/changeset/192017>