James Kitchener (:jkitch) Reporter
	Description • 11 years ago

Bug 114365 uses fontstyle and fontweight to implement bold/italic/bold-italic mathvariants due to concerns over people not having a font supporting the mathematical alphanumeric block.

Once support of the mathematical alphanumeric block is sufficiently prevalent, the character transformation method used by the other mathvariant types should be adopted instead of fontstyle/fontweight.

	James Kitchener (:jkitch) Reporter
	Comment 1 • 10 years ago

The original function was designed for easy removal of the offending bits.

Fixing the test suites was the tedious part.

	James Kitchener (:jkitch) Reporter
	Comment 2 • 10 years ago

Comment on attachment 8361652 [details] [diff] [review]
fix.diff

One change in this patch that warrants additional confirmation:

mathvariant="bold" and font-weight="bold" no longer produce equivalent output.

Is this fine?

	Frédéric Wang (:fredw) Assignee
	Comment 3 • 10 years ago

Comment on attachment 8361652 [details] [diff] [review]
fix.diff

Review of attachment 8361652 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Yes, the deprecated fontweight/fontstyle attributes should map to CSS font-weight/font-style while the mathvariant attribute should do the Unicode remapping, so they will render differently.

We will need to wait that we support more MATH fonts before taking this patch.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 4 • 10 years ago

Can this be checked in?

	James Kitchener (:jkitch) Reporter
	Comment 5 • 10 years ago

This is up to :fredw, but I think we are still waiting on improved math font support.

	Frédéric Wang (:fredw) Assignee
	Comment 6 • 10 years ago

I think we will need at least 407059 and then the font-family update of 947654. Then we can ask users to switch to MATH fonts and this patch can be taken safely (and will be the right thing to do for MATH fonts).

	Frédéric Wang (:fredw) Assignee
	Comment 7 • 10 years ago

This patch makes menclose-3-radical.html pass but subscript-italic-correction.html fail.

https://tbpl.mozilla.org/?tree=Try&rev=e5962df46065

I suspect subscript-italic-correction.html could be a bit less strict. However:

- on mobile devices, I think the problem is that no fonts with italic alphanum char is installed so they are just boxes without italic correction.

- on Windows, I suspect Cambria Math is used and the handling of italic correction might be different (see bug 407059 comment 5 and bug 961482).

James: can you check?

I would be fine with marking subscript-italic-correction fail and fix that later. However, the missing glyphs on mobile devices is more a concern. On the other hand, without that patch the rendering looks of e.g. italic variable with new Open Type MATH fonts is "ugly". We make this patch applies conditionally with a #if macro, but that might not be convenient to handle the mathvariant tests differently.

	Frédéric Wang (:fredw) Assignee
	Comment 9 • 10 years ago

So it seems that math fonts have different way to set the advance width & left/right bearings.

In particular for STIX Math, Latin Modern Math and Cambria Math, the advance width seems to almost match the right bearing, so our computed italic correction is zero.

Not sure what is the right way to do...

	James Kitchener (:jkitch) Reporter
	Comment 10 • 10 years ago

subscript-italic-correction.ref uses STIX-Regular for me, but it still fails due to the width/rightbearing issue of comment 9.

This also has repercussions on non script uses of maths fonts.

> <math>
> <mtext mathvariant="italic" style="font-family:Cambria Math">office</mtext>
> </math>
Output shown in attachment.  "office", "of fice" or "of f ice"?

I've tested with Microsoft's equation editor and get an equivalent rendering, so they don't appear to apply any reverse italics correction.

It is almost as if we need to override the metrics of specific glyphs.  However that is probably a very nasty solution.

	Frédéric Wang (:fredw) Assignee
	Comment 11 • 10 years ago

(In reply to James Kitchener (:jkitch) from comment #10)
> I've tested with Microsoft's equation editor and get an equivalent
> rendering, so they don't appear to apply any reverse italics correction.

Did you try to write f_0 with the equation editor?

	Khaled Hosny
	Comment 12 • 10 years ago

Per MATH table spec, italic correction is applied only “when a run of slanted characters is followed by a straight character (such as an operator or a delimiter)“ (and when positioning limits and super/subscripts of course).

	James Kitchener (:jkitch) Reporter
	Comment 13 • 10 years ago

Not until now.  The subscript gets an "inverse italics correction" where appropriate.

	Frédéric Wang (:fredw) Assignee
	Comment 14 • 10 years ago

I checked glyph info of the italic f (Cambria Math) with fontforge :

- the advance width and right ink bound are almost the same (delta = 15)

- the italic correction is only 60 so even if it would be reversed, it would not be enough to get the placement on attachment 8411836 [details].

- however the bottom right kernings are (for various heights) -400, -320 and 0. So that seems to be what ensure the correct placement of the subscript.

So I think we need to fix bug 961482 to get the correct placement for Cambria Math.

	Khaled Hosny
	Comment 15 • 10 years ago

Regarding the missing fonts, can’t we bundle a MATH font with the tests so that it works everywhere?

	Frédéric Wang (:fredw) Assignee
	Comment 16 • 10 years ago

(In reply to Khaled Hosny from comment #15)
> Regarding the missing fonts, can’t we bundle a MATH font with the tests so
> that it works everywhere?

We can use Web fonts to avoid missing fonts or override the behavior of Cambria Math ; or we could even mark the test as "fails". The main problem is not really to make the test pass but about what users will see in actual content (that is bad position of scripts with Cambria Math on Windows and more, seriously, empty boxes instead of variables on mobile devices). I think we should try to fix at least the missing fonts on mobile devices.

	Frédéric Wang (:fredw) Assignee
	Comment 17 • 10 years ago

The changes to ensure math fonts on mobile devices might take some time and it's a shame that the rendering (especially italic variables) still does not work well with the new MATH fonts on desktop... So perhaps an alternative would be to take this change under an #ifdef, so that we only keep the old behaviors on mobile devices for now? (and mark the reftest failures appropriately)

	Frédéric Wang (:fredw) Assignee
	Comment 18 • 10 years ago

What do you think about comment 17?

	Karl Tomlinson (:karlt)
	Comment 19 • 10 years ago

I'm OK with an ifdef.

An alternative could be to test whether the requested characters have glyphs, but I don't know whether that would be worthwhile or not.

	Frédéric Wang (:fredw) Assignee
	Comment 20 • 10 years ago

Let's see what's the #ifdef gives:
https://tbpl.mozilla.org/?tree=Try&rev=e7534acc25fd

	James Kitchener (:jkitch) Reporter
	Comment 21 • 10 years ago

On second thoughts there are two other problematic platforms to consider
Windows XP (without Office 2007+ installed)
OSX10.6
Neither have maths fonts available by default.

Both of those platforms are officially supported (at least in the immediate future) and an #ifdef won't help them.

	Frédéric Wang (:fredw) Assignee
	Comment 22 • 10 years ago

(In reply to James Kitchener (:jkitch) from comment #21)
> On second thoughts there are two other problematic platforms to consider
> Windows XP (without Office 2007+ installed)
> OSX10.6
> Neither have maths fonts available by default.

However, we need a bit less than OpenType MATH fonts, here. We only need coverage for the Unicode Math Alpha num. OSX>=10.5 seems to have https://en.wikipedia.org/wiki/Apple_Symbols. I'm not sure for Windows XP, though (I've added a test mi-mathvariant-3 to check that)

	Frédéric Wang (:fredw) Assignee
	Comment 23 • 10 years ago

reftest for OSX 10.6 failed, so I'm trying again with the Apple Symbols explicitly stated:
https://tbpl.mozilla.org/?tree=Try&rev=754157d11a1b

	Frédéric Wang (:fredw) Assignee
	Comment 25 • 10 years ago

it looks like it does not work on MacOS10.6 even with Apple Symbols listed and there does not seem to be any font for XP. So I'm trying another approach: https://tbpl.mozilla.org/?tree=Try&rev=2040bb10469d

	James Kitchener (:jkitch) Reporter
	Comment 26 • 10 years ago

Note that with Android and B2G, reftests are only run for Opt builds so "-p do" or two separate runs are required for complete coverage.

	Frédéric Wang (:fredw) Assignee
	Comment 27 • 10 years ago

The previous one with only HasChar on the first font didn't seem to work: https://tbpl.mozilla.org/?tree=Try&rev=a25686d26b9e

This one uses FindFontForChar to test more fonts, although I suspect it is less efficient.

	Frédéric Wang (:fredw) Assignee
	Comment 28 • 10 years ago

So here is an updated patch with the result of the Try server.

B2G, Android, /^Windows\x20NT\x205\.1/.test(http.oscpu) and OSX10.6 will still use the CSS style and so some mathvariant-* tests will fail. Also, OSX==10.8 does not seem to have the italic form for the dotless i, j (probably an issue with the STIXGeneral fonts?). subscript-italic-correction.html will fail with Cambria Math per discussions above.

Karl, what do you think about that? Isn't it a perf problem to call FindFontForChar for each MathML letter in bold/italic/bold-italic?

	Frédéric Wang (:fredw) Assignee
	Comment 30 • 10 years ago

(In reply to Frédéric Wang (:fredw) from comment #28)
> Karl, what do you think about that? Isn't it a perf problem to call
> FindFontForChar for each MathML letter in bold/italic/bold-italic?

Perhaps a faster option would be to use gfxFontGroup::GetFirstMathFont() suggested by Jonathan on bug 961365 and call HasChar on it.

	Karl Tomlinson (:karlt)
	Comment 31 • 10 years ago

Comment on attachment 8422475 [details] [diff] [review]
Patch

(In reply to Frédéric Wang (:fredw) from comment #28)
> Karl, what do you think about that? Isn't it a perf problem to call
> FindFontForChar for each MathML letter in bold/italic/bold-italic?

FindFontForChar() is essentially what shaping the TextRun will do (if it
chooses to use these chars, so shouldn't be a perf problem, even without the
unknown script optimization (see below).

(In reply to Frédéric Wang (:fredw) from comment #30)
> Perhaps a faster option would be to use gfxFontGroup::GetFirstMathFont()
> suggested by Jonathan on bug 961365 and call HasChar on it.

The gfx code is optimized for finding fonts for characters, including caches
of character support.  I don't know whether similar info is cached about
whether the font has a math table, but I'd guess support is not as good.

>+        uint8_t matchType = gfxTextRange::kFontGroup;

No need to initialize matchType, as it is not used.
(It is an outparam, that is sometimes not set if aPrevMatchedFont is
non-null.)

>+          FindFontForChar(ch2, 0, HB_SCRIPT_UNKNOWN, nullptr, &matchType);

Note that some platforms will not search all fonts for HB_SCRIPT_UNKNOWN. [1]
Author specified and some interpretation of the preferences for the generic
(which I don't understand) will be searched.  Perhaps that's what you want if
you think that using style with one of the author-specified fonts would be
better.  If so, please add a comment indicating this.  Otherwise,
HB_SCRIPT_COMMON should be a thorough search, and I expect most of
these characters would be script common.  The performance difference should
not be significant factor in the decision here.

[1] http://hg.mozilla.org/mozilla-central/annotate/93e03b8c127e/gfx/thebes/gfxFont.cpp#l5516

	Karl Tomlinson (:karlt)
	Comment 33 • 10 years ago

Comment on attachment 8422928 [details] [diff] [review]
Patch

>-  font-family: MathJax_Main, STIXGeneral, DejaVu Serif, DejaVu Sans, Cambria, Cambria Math, Times, Lucida Sans Unicode, OpenSymbol, Standard Symbols L, serif;
>+  font-family: MathJax_Main, STIXGeneral, DejaVu Serif, DejaVu Sans, Cambria, Cambria Math, Times, Lucida Sans Unicode, OpenSymbol, Standard Symbols L, Apple Symbols, serif;

Is there still a reason to include "Apple Symbols" here?
If it doesn't help, then including an extra family will only slow down stretching.

	Frédéric Wang (:fredw) Assignee
	Comment 34 • 10 years ago

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #33)
> Is there still a reason to include "Apple Symbols" here?
> If it doesn't help, then including an extra family will only slow down
> stretching.

No, that didn't help for OSX10.6... I'll remove it.

	Frédéric Wang (:fredw) Assignee
	Comment 36 • 10 years ago

Green try:
https://tbpl.mozilla.org/?tree=Try&rev=0ee059cb7ec8

	Ryan VanderMeulen [:RyanVM]
	Comment 37 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f0668d2d9c

	Frédéric Wang (:fredw) Assignee
	Comment 38 • 10 years ago

I'll try to submit this for Aurora, so that mathvariants work correctly with MATH fonts in Gecko 31 ESR.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 39 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/a2f0668d2d9c

	Frédéric Wang (:fredw) Assignee
	Comment 40 • 10 years ago

Comment on attachment 8423118 [details] [diff] [review]
Patch for Aurora

[Approval Request Comment]

Bug caused by (feature/regressing bug #): basic mathvariants in MathML rendering when modern math fonts are used. The bug has always been present but until Gecko 31, these new math fonts were not usable for MathML so that was not a concern (see below for details).

User impact if declined: The basic mathvariants with the new math fonts will be displayed with a lower quality, especially for the ubiquitous case of italic math variables, so people won't be able/willing to use these new fonts and that will delay the transition (see below for details).

Testing completed (on m-c, etc.): This change was tested on Try, Inbound and landed in mozilla-central last Thursday without problem. This change was already covered by existing MathML tests. A new mi-mathvariant-3.html has been added to ensure that the regression of attachment 8361652 [details] [diff] [review] does not happen with this new version (see below for details).

Risk to taking this patch (and alternatives if risky): Low. This patch only touches MathMLTextRunFactory::RebuildTextRun and thus will only affect pages with MathML formulas. Only a few lines of code are added to check fonts that have Mathematical Alphanumeric Symbols and should not be a performance problem per comment 31.

String or IDL/UUID changes made by this patch: None

Details of why the patch is needed on aurora:

Gecko 31 will have initial support for math fonts with an Open Type MATH table (https://developer.mozilla.org/en-US/Firefox/Releases/31#MathML). We want users to migrate to these modern Open Type fonts and to deprecate old fonts, so that we will be able to clean up our MathML/font code. It might take some time for this move to happen on all the platforms and so it seems a good idea to start making these modern fonts usable as soon as possible, and in particular in the ESR version.

The basic mathvariants (italic, bold and bold italic) are currently handled via CSS style in Aurora. This works well for old fonts that are made of several font faces, but the Open Type MATH fonts only have a single "regular" font face and are normally designed to instead use the Mathematical Alphanumeric Symbols (http://en.wikipedia.org/wiki/Mathematical_Alphanumeric_Symbols). This means that the basic mathvariants with the new Open Type MATH fonts are currently displayed with a lower quality, especially for the ubiquitous case of italic math variables.

Attachment 8361652 [details] [diff] was not taken before Gecko 31 moved to Aurora, because the default fonts on mobiles platforms and some old desktop platforms do not cover the Mathematical Alphanumeric Symbols, causing an even worse rendering with missing characters. The approach in this new patch is to use Mathematical Alphanumeric Symbols only when they are available, and to fallback to CSS style otherwise.

	Sylvestre Ledru [:Sylvestre]
	Comment 41 • 10 years ago

The approval-mozilla-aurora+ is enough to see the patch checked-in.

	Ryan VanderMeulen [:RyanVM]
	Comment 42 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/c80940c98d56

	Frédéric Wang (:fredw) Assignee
	Comment 43 • 10 years ago

https://developer.mozilla.org/en-US/Firefox/Releases/31#MathML