Bug 84649 - RenderMathMLOperator currently ignores font families, fails to use Stix
Summary: RenderMathMLOperator currently ignores font families, fails to use Stix
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
Keywords: InRadar
Depends on:
Reported: 2012-04-23 16:31 PDT by Beth Dakin
Modified: 2012-04-23 18:12 PDT (History)
1 user (show)

See Also:

Patch (208.74 KB, patch)
2012-04-23 16:45 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
New Patch (210.50 KB, patch)
2012-04-23 17:36 PDT, Beth Dakin
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2012-04-23 16:31:57 PDT

RenderMathMLOperator ignores the RenderStyle's font-family. This means that it ignores the mathml.css preference for Stix fonts and ends up hunting for all of the operator glyphs in system fallback fonts. It also means that if a content author specified a different preferred font via @font-face, that would also be ignored by the operators.

Patch forthcoming.
Comment 1 Beth Dakin 2012-04-23 16:45:51 PDT
Created attachment 138456 [details]
Comment 2 mitz 2012-04-23 16:59:14 PDT
Comment on attachment 138456 [details]

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

I made all the comments in the change log even though some of them are about the code!

> Source/WebCore/ChangeLog:13
> +         This patch makes RenderMathMLOperator honor the font-family list. This means that 
> +         by default, Stix glyphs will now be used for operators just like for other MathML 
> +         content. Unfortunately, just doing that resulted in a bug because of the fragile 
> +         hardcoded glyph sizes. The Stix vertical bar glyph is much smaller than the code 
> +         assumed any glyphs would be. That code should be re-written, but in the meantime, 
> +         I put a fix in place to try to make it work for small glyphs.

Something is off with the indentation here…

> Source/WebCore/ChangeLog:22
> +        (WebCore::RenderMathMLOperator::heightForGlyph):
> +        (WebCore::RenderMathMLOperator::lineHeightForGlyph):

Since these functions take a UChar, not a Glyph, I think they should be named differently. Perhaps glyphHeightForCharacter() and lineHeightForCharacter() (or glyphLineHeightForCharacter(), whatever “glyph line height” means).

> Source/WebCore/ChangeLog:26
> +        (WebCore::RenderMathMLOperator::updateFromElement):

You mention earlier in the change log why you’re using the style’s FontDescription (so that you get the right family list), but you should also mention why you’re using its FontSelector (so that you get @font-face fonts).

> Source/WebCore/ChangeLog:32
> +        (WebCore::RenderMathMLOperator::createStackableStyleForGlyph):

Again, “ForGlyph” here is wrong, since the function takes a UChar (and naming that parameter “glyph” is wrong for the same reason). I am also wondering whether the it should be the caller’s responsibility to compute the line height and pass it in, since (a) it’s strange to pass one size but not the other and (b) we’re calling this function from a loop and the value never changes, so it should be more efficient.
Comment 3 Beth Dakin 2012-04-23 17:36:48 PDT
Created attachment 138473 [details]
New Patch

Thanks Dan! Here is a new patch that addresses your comments.
Comment 4 mitz 2012-04-23 17:59:42 PDT
Comment on attachment 138473 [details]
New Patch

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

> Source/WebCore/ChangeLog:11
> +        used for operators just like for other MathML  content. 

Extra space after MathML.

> Source/WebCore/ChangeLog:19
> +        (WebCore):
> +

Don’t need these two lines.

> Source/WebCore/ChangeLog:34
> +

Extra newline here.

> Source/WebCore/ChangeLog:38
> +        again it, use the style's FontDescription and FontSelector.

again it, use?
Comment 5 Beth Dakin 2012-04-23 18:12:47 PDT
Thanks Dan! http://trac.webkit.org/changeset/114975