Use Unicode Mathematical alphanumeric characters for bold/italic/bold-italic mathvariants

RESOLVED FIXED in Firefox 31

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jkitch, Assigned: fredw)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

unspecified
mozilla32
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox31 fixed, firefox32 fixed)

Details

Attachments

(2 attachments, 10 obsolete attachments)

(Reporter)

Description

5 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.
(Reporter)

Updated

5 years ago
Depends on: 114365
(Assignee)

Updated

5 years ago
Blocks: 947654
(Reporter)

Comment 1

5 years ago
Created attachment 8361652 [details] [diff] [review]
fix.diff

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

Fixing the test suites was the tedious part.
Assignee: nobody → jkitch.bug
Status: NEW → ASSIGNED
Attachment #8361652 - Flags: review?(roc)
(Reporter)

Comment 2

5 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?
Attachment #8361652 - Flags: review?(fred.wang)
(Assignee)

Comment 3

5 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.
Attachment #8361652 - Flags: review?(fred.wang) → review+
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
Can this be checked in?
Flags: needinfo?(jkitch.bug)
(Reporter)

Comment 5

5 years ago
This is up to :fredw, but I think we are still waiting on improved math font support.
Flags: needinfo?(jkitch.bug) → needinfo?(fred.wang)
(Assignee)

Comment 6

5 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).
No longer blocks: 947654
Depends on: 407059, 947654
Flags: needinfo?(fred.wang)
(Assignee)

Comment 7

5 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.
Flags: needinfo?(jkitch.bug)
(Assignee)

Comment 8

5 years ago
Created attachment 8411750 [details]
testcase italic f
(Assignee)

Comment 9

5 years ago
Created attachment 8411751 [details]
screenshot

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

Comment 10

5 years ago
Created attachment 8411816 [details]
office.png

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.
Flags: needinfo?(jkitch.bug)
(Assignee)

Comment 11

5 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?

Comment 12

5 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).
(Reporter)

Comment 13

5 years ago
Created attachment 8411836 [details]
f_0.png

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

Comment 14

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

Comment 15

5 years ago
Regarding the missing fonts, can’t we bundle a MATH font with the tests so that it works everywhere?
(Assignee)

Comment 16

5 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.
(Assignee)

Updated

5 years ago
Depends on: 1001570
(Assignee)

Updated

5 years ago
Depends on: 775060
(Assignee)

Comment 17

5 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)
(Assignee)

Comment 18

5 years ago
What do you think about comment 17?
Flags: needinfo?(karlt)
Flags: needinfo?(jkitch.bug)
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.
Flags: needinfo?(karlt)
(Assignee)

Comment 20

5 years ago
Let's see what's the #ifdef gives:
https://tbpl.mozilla.org/?tree=Try&rev=e7534acc25fd
(Reporter)

Comment 21

5 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.
Flags: needinfo?(jkitch.bug)
(Assignee)

Comment 22

5 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)
(Assignee)

Comment 23

5 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
(Assignee)

Comment 24

5 years ago
Created attachment 8421003 [details] [diff] [review]
Patch (test if the first font has the math alphanum char)
(Assignee)

Comment 25

5 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
(Reporter)

Comment 26

5 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.
(Assignee)

Comment 27

5 years ago
Created attachment 8421805 [details] [diff] [review]
Patch (use FindFontForChar)

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

Comment 28

5 years ago
Created attachment 8422475 [details] [diff] [review]
Patch

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?
Attachment #8421003 - Attachment is obsolete: true
Attachment #8421805 - Attachment is obsolete: true
Attachment #8422475 - Flags: feedback?(karlt)
(Assignee)

Comment 29

5 years ago
Created attachment 8422476 [details] [diff] [review]
Patch (ifdef)
(Assignee)

Comment 30

5 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.
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
Attachment #8422475 - Flags: feedback?(karlt) → feedback+
(Assignee)

Comment 32

5 years ago
Created attachment 8422928 [details] [diff] [review]
Patch
Assignee: jkitch.bug → fred.wang
Attachment #8422475 - Attachment is obsolete: true
Attachment #8422928 - Flags: review?(karlt)
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.
Attachment #8422928 - Flags: review?(karlt) → review+
(Assignee)

Comment 34

5 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.
(Assignee)

Comment 35

5 years ago
Created attachment 8423027 [details] [diff] [review]
Patch
Attachment #8361652 - Attachment is obsolete: true
Attachment #8411750 - Attachment is obsolete: true
Attachment #8411751 - Attachment is obsolete: true
Attachment #8411816 - Attachment is obsolete: true
Attachment #8411836 - Attachment is obsolete: true
Attachment #8422476 - Attachment is obsolete: true
Attachment #8422928 - Attachment is obsolete: true
(Assignee)

Comment 36

5 years ago
Green try:
https://tbpl.mozilla.org/?tree=Try&rev=0ee059cb7ec8
Keywords: checkin-needed
(Assignee)

Comment 38

5 years ago
Created attachment 8423118 [details] [diff] [review]
Patch for Aurora

I'll try to submit this for Aurora, so that mathvariants work correctly with MATH fonts in Gecko 31 ESR.
https://hg.mozilla.org/mozilla-central/rev/a2f0668d2d9c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(Assignee)

Comment 40

5 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.
Attachment #8423118 - Flags: approval-mozilla-aurora?
status-firefox31: --- → affected
status-firefox32: --- → fixed
Attachment #8423118 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

5 years ago
Attachment #8423027 - Flags: checkin+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [please commit attachment 8423118 into Aurora]
The approval-mozilla-aurora+ is enough to see the patch checked-in.
Keywords: checkin-needed
Whiteboard: [please commit attachment 8423118 into Aurora]
(Assignee)

Comment 43

5 years ago
https://developer.mozilla.org/en-US/Firefox/Releases/31#MathML
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 1078712
You need to log in before you can comment on or make changes to this bug.