Bug 84649 – RenderMathMLOperator currently ignores font families, fails to use Stix

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Beth Dakin]

<rdar://problem/10445150>

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.

[Beth Dakin]

Created attachment 138456 [details]
Patch

Comment on attachment 138456 [details]
Patch

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.

[Beth Dakin]

Created attachment 138473 [details]
New Patch

Thanks Dan! Here is a new patch that addresses your comments.

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?

[Beth Dakin]

Thanks Dan! http://trac.webkit.org/changeset/114975