Bug 99921 - [MathML] Symbol font uses greek letters for roman ones on linux and Windows
Summary: [MathML] Symbol font uses greek letters for roman ones on linux and Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Barton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-20 20:37 PDT by Dave Barton
Modified: 2014-02-17 12:58 PST (History)
13 users (show)

See Also:


Attachments
Gaps in stretched vertical bars (148 bytes, text/html)
2012-10-20 20:37 PDT, Dave Barton
no flags Details
Patch (591.71 KB, patch)
2012-10-20 21:36 PDT, Dave Barton
no flags Details | Formatted Diff | Diff
Patch (1.05 MB, patch)
2012-10-23 12:50 PDT, Dave Barton
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Barton 2012-10-20 20:37:43 PDT
Created attachment 169784 [details]
Gaps in stretched vertical bars

For parts of stretched parentheses and brackets, mathml.css currently uses the Symbol font, which on linux and Windows has greek letters at roman code points. See e.g.
http://build.webkit.org/results/Chromium%20Win%20Release%20%28Tests%29/r130950%20%2830524%29/mathml/presentation/attributes-actual.png
or bug 38377 comment 2.

To fix this, we switch to the STIXSizeOneSym font when available, including OS X Lion or later. However, this can cause vertical gaps between glyph parts, because of hard-coded constants in RenderMathMLOperator.cpp. This was partially addressed in bug 84649, but not completely because RenderMathMLOperator::createStackableStyle still used a hard-coded font size of 14pt, while e.g. RenderMathMLOperator::glyphHeightForCharacter did not. This can be seen even with the current Symbol font, in the attached example.
Comment 1 Dave Barton 2012-10-20 21:36:28 PDT
Created attachment 169785 [details]
Patch
Comment 2 spartha80 2012-10-23 08:13:09 PDT
Not all platforms currently implement platformBoundsForGlyph. The bound returned will be zero sized on some of them. Currently most of ports using Skia, FreeType for font rendering still work(I still haven't figured out how), maybe because of the values being hard coded in RenderMathMLOperator::glyphHeightForCharacter. That behavior may break with your patch. I will try integrating your patch over one of those ports and check.
Comment 3 Dave Barton 2012-10-23 09:49:55 PDT
Do the Skia/FreeType/etc. ports have ENABLE_MATHML on? Do they run pixel tests? The mac, efl, and gtk ports are the only ones with any LayoutTests/platform/*/mathml files. The apple/win port enables MathML, but doesn't run pixel tests.

Until now, most web pages and libraries that use MathML browser-sniff and turn MathML off for WebKit, i.e. Safari. We've really had just a prototype implementation with many layout problems, and possible security issues. I've been fixing these, notably in r124512 (bug 43819) where I converted MathML to use  { -webkit-line-box-contain: glyphs replaced; line-height: 0; }. This broke the efl and gtk ports because of no platformBoundsForGlyph as you (spartha80) say. Christophe Dumez was able to fix this on linux fairly quickly in bug 93073 - see its comments 8, 11 and 12, and its patch. It'd be great if a font expert could do this on other ports also, and then after this bug (99921) is fixed, I'd recommend MathML be enabled on all ports. For instance, we enabled MathML in Chrome in r130950 (bug 96960) two weeks ago, and I believe their automated fuzzers have not detected any security issues. I plan to send a message to webkit-dev summarizing all this, after this bug is resolved. (A somewhat revised patch is coming soon.)

Thanks for your help and interest!
Comment 4 Dave Barton 2012-10-23 12:50:53 PDT
Created attachment 170208 [details]
Patch
Comment 5 Eric Seidel (no email) 2012-10-23 12:57:35 PDT
Comment on attachment 170208 [details]
Patch

LGTM.
Comment 6 Dave Barton 2012-10-23 14:14:13 PDT
Committed r132264: <http://trac.webkit.org/changeset/132264>