257978 - Firefox does not display minus sign inside mathml 'mo' tag but Netscape 7.1 does.

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Alex Milowski]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a3) Gecko/20040720 Firefox/0.9.1+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a3) Gecko/20040720 Firefox/0.9.1+

Firefox behaves inconsistently with Netscape 7.1.  Basic support of rendering
the 'mo' (math operator) tag seems to not be working.  The example URL should
render '-1' but it doesn't in Firefox.  In Netscape 7.1, it works just fine. 
So, the big question is whether there is a different version of the Mathml
implemenation at work here given that these two browsers were released at
different times.

Obviously, this is a severe bug.  The minus sign is rather important.  :)

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Actual Results:  
1

Expected Results:  
-1

Comment 1

[Hiro]

There is no problem with the Trunk version Camino.

Mac OS X 10.3.5
2004090321 (v0.8+)

Comment 2

[Alex Milowski]

(In reply to comment #1)
> There is no problem with the Trunk version Camino.
> 
> Mac OS X 10.3.5
> 2004090321 (v0.8+)
> 

Netscape 7.1 will render the document correctly but Netscape 7.2 and Firefox 0.9
will not

Comment 3

[Alex Milowski]

This is on OS X 10.3.5 just in case that changes anything.

Comment 4

[Alex Milowski]

Camino does not have MathML enabled.  Please look at:

 http://www.w3.org/Math/testsuite/testsuite/Presentation/TokenElements/mo/mo5.xml

You should see a fraction of a/b and you do not in Camino.

Yes, the minus sign shows up in Camino but that is because it is not rendering
the MathML.

Comment 5

[Hiro]

oops...

It reproduced with the trunk version mozilla.
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a4) Gecko/20040910

Comment 6

[Alex Milowski]

On line 230 of nsMathMLmoFrame.cpp the regular minus sign is suppose to be
translated to 0x2212.  See:

  
http://lxr.mozilla.org/mozilla/source/layout/mathml/base/src/nsMathMLmoFrame.cpp#230

Interestingly enough, the character 0x2212 (minus sign from 'Math Operators') 
does display properly on Mac OS X.  See:

   http://www.milowski.com/badminus.xhtml

But the regular minus sign does not display.

Comment 7

[rbs]

Care to attach screenshots to allow non-Mac users to see what you see?

Comment 8

[Alex Milowski]

Attachment: Window Capture of no minus sign

Comment 9

[Alex Milowski]

Here's an additional document:

   http://www.milowski.com/badminus.xhtml

It shows how the regular minus sign (0x0d) does not show up but the
math code page minus does.

Comment 10

[rbs]

Can you update you document (badminus) with this:

<p>The hypen-minus sign: -</p> <!--- add this line -->
<p>The Unicode minus sign: &#x2212;</p>
... keep same content ...

Then attach a screenshot? The reason I am asking this is to confirm/deny whether
the problem comes just from the re-mapping of '-' to 0x2212 (cf. your comment
6). Or if there is something deeper.

Comment 11

[Alex Milowski]

Attachment: Screenshoot for badminus.xhtml

Comment 12

[Alex Milowski]

I've updated http://www.milowski.com/badminus.xhtml as requested and attached a
screenshot.

Comment 13

[rbs]

Is this bug still there with recent builds?

Comment 14

[YAMASHITA Makoto]

> Is this bug still there with recent builds?
Yes, there is, and in fact there is aleady a workaround for this.
Set "font.mathfont-family.\u2212.base" to "Times" or "Lucida Grande" or whatever a valid font name on Mac OS X.
I guess this is caused by the line
font.mathfont-family.\u2212.base     = Symbol
in mathfont.properties together with the fact that we should write "Sigma-psi-mu-beta-omicron-lambda" to refer to the Symbol font (at least on Mac). Have soomething to do with the bug 213702 ?
Interestingly, if the minus sign is specified directly as u2212 in the source mathml this problem does not happen. If we use u002D, it's not rendered (of course there is no problem outside mathml).

Comment 15

[rbs]

Do you mean that <mo>&#x2212;</mo> works fine? (but <mo>-</mo> doesn't work.)

Comment 16

[YAMASHITA Makoto]

Yes. Mac people please check out the rendering of
http://www.ms.u-tokyo.ac.jp/~makotoy/ffmath/minus.xml
by out-of-box DeerPark and by the one with font.mathfont-family.\u2212.base configuration.

Comment 17

[rbs]

What about ≤ (u2264) and ≥ (u2265)? [These are the other characters for which the .base font is set as Symbol in mathfont.properties]
Do they disappear too?

Comment 18

[YAMASHITA Makoto]

Oddly they don't. Moreover if I set font.mathfont-family.\u2264.base to "Sigma-lambda-...", u2264 is rendered as a double prime or something. u2265 is not affected anyway.

Comment 19

[rbs]

I am beginning to suspect that this is a general problem that also happens in plain HTML -- not just MathML. For example, it seems to me that you might also see the problem with:

<span style="font-family:Symbol"> &#x2212; &ge; &le; </span> inside HTML.

Comment 20

[YAMASHITA Makoto]

Attachment: screenshot of http://www.ms.u-tokyo.ac.jp/~makotoy/ffmath/minus.xml

I've forgotten to point out important things...
#14> whatever a valid font name
includes "Sigma-psi-.." and commenting out the line ..base of mathfont.properties works too.

I incorporated your suggestion into the test page
http://www.ms.u-tokyo.ac.jp/~makotoy/ffmath/minus.xml
and it looks like general-HTML (unicode) codepoint-glyph correspondence is broken for Symbol.
The attribute style="font-family:Symbol" is no-op and style="font-family:Sigma-psi-.." (actually greek letters used) makes the english text "greeked." For "true greek" letters some fallback font is used instead of Symbol.
I attached the screenshot of that test page by Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20051024 Firefox/1.6a1 with no configuration. u2264 and u2265 are disappearing there but this does not reproduce always (contrary to u002D), see the test page images.

Comment 21

[rbs]

Any luck when adding Math1 in mathfont.properties? That is, try changing:

font.mathfont-family.\u2264.base     = Symbol
font.mathfont-family.\u2265.base     = Symbol
font.mathfont-family.\u2212.base     = Symbol

to:

font.mathfont-family.\u2264.base     = Symbol, Math1
font.mathfont-family.\u2265.base     = Symbol, Math1
font.mathfont-family.\u2212.base     = Symbol, Math1


Math1 has all the glyphs that Symbol has (and more):
http://www.mozilla.org/projects/mathml/fonts/encoding/math1.gif
http://www.mozilla.org/projects/mathml/fonts/encoding/symbol.gif

Comment 22

[rbs]

Or alternatively, "Math1, Symbol". This will however give precedence to Math1 on all platforms (for the listed characters). If it works on the Mac, it will be worth the trade-off.

Comment 23

[YAMASHITA Makoto]

I've put screenshots of
http://www.ms.u-tokyo.ac.jp/~makotoy/ffmath/minus.xml
under different configs in that page.
Math1 is not working for the minus sign (but working for le & ge), u002D and u2212
 are rendered differently in some cases, etc...
It looks best to just eliminate those three lines from mathfont.properties if avoid Sigma-psi-.. or Times or something.
I'm goin to look into gfx/src/mac to see if there can be a fundamental fix for this bug (e.g. nsATSUIUtils.cpp has some ovious bugs).

Comment 24

[rbs]

simon, do you have a Mac build environment? If so, would you mind chiming in here?!?

Here is the story: to paint normal chars like − (\u2212), all we do here is set the font-family (fetched from font.mathfont-family.\u2212.base) in the font.name struct (using the SetBaseFamily() helper function), then call aRenderingContext.SetFont() and then DrawString() in the usual way. This is the corresponding code fragment. You can uncomment the printf and the color to see it red:

http://lxr.mozilla.org/mozilla/source/layout/mathml/base/src/nsMathMLChar.cpp#2019

2021       PRUint32 len = PRUint32(mData.Length());
2022       if (1 == len) {
2023         SetBaseFamily(mData[0], theFont);
2024       }
2025       aRenderingContext.SetFont(theFont, nsnull);
2026 //printf("Painting %04X like a normal char\n", mData[0]);
2027 //aRenderingContext.SetColor(NS_RGB(255,0,0));
2028       aRenderingContext.DrawString(mData.get(), len, mRect.x, mRect.y + mBoundingMetrics.ascent)

This should in fact be equivalent to what we do for <span style="font-family: Symbol">...</span>, except that the langGroup parameter is passed as null in the MathML's SetFont() call -- this defaults to the user's locale langGroup (see DeviceContextImpl::GetMetricsFor), as opposed to be the element langGroup as nsTextFrame's SetFontFromStyle() does. I am wondering if this brings a difference in the <span> vs. <mo> on the Mac that other platforms don't have.

Comment 25

[YAMASHITA Makoto]

Sorry for being so late, but I have a fix for the "minus sign missing" issue (not for the Symbol font handling issue, I've learned these two are separated)
That was caused by poor TEC fallback algorithm for GetBoundingMetrics/Draw process (ll 292-300 and 346-353 in nsUnicodeRenderingToolkit.cpp). It assumes nsUnicodeFontMapping knows about fallback font determined by the script, and does nothing if it fauks the unicode->scriptdata conversion.
Just giving up then i.e. append "else { return PR_FALSE; }" there is the solution.
In addition ASTUI fallback has a room for improvement in handling fontID and italic/boldness.
I gonna prepare the patch ASAP.
BTW, can I put "CFLAGS += -funsigned-char" in Makefile.in?

Comment 26

[YAMASHITA Makoto]

Attachment: TEC fallback gives up when the font is not known to the fontMapping

TEC fallback refinement and
nsATSUIUtils:
  get font family by Font Manager API (some fonts are not accessible via ATSUFONDtoFontID that is "not recommended" by Apple),
  Italic and boldness flags are reflected

Comment 27

[Simon Montagu :smontagu]

(In reply to comment #24)
> simon, do you have a Mac build environment?

Sorry, no. Passing the buck to Jungshik and Asaf.

Comment 28

[rbs]

Index: nsATSUIUtils.cpp
===================================================================

+#define MY_ATTR_CNT 5
+	ATSUAttributeTag 		theTag[MY_ATTR_CNT];
+	ByteCount				theValueSize[MY_ATTR_CNT];
+	ATSUAttributeValuePtr 	theValue[MY_ATTR_CNT];

Nicer to move the #define at the header -- next to the existing
"#define FloatToFixed(a)". Also drop "MY_" and simply say ATTR_CNT.


 	//--- Font ID & Face -----
 	ATSUFontID atsuFontID;
-
-	err = ::ATSUFONDtoFontID(aFontNum, (StyleField)((aBold ? bold : normal) | (aItalic ? italic : normal)), &atsuFontID);
-	if(noErr != err) {
-		NS_WARNING("ATSUFONDtoFontID failed");
-    // goto errorDoneDestroyStyle;
+	FMFontFamily myFontFam;
+	FMFontStyle fbStyle;
+	myFontFam = ::FMGetFontFamilyFromATSFontFamilyRef(aFontNum);
+    if (::FMGetFontFromFontFamilyInstance(myFontFam, 0, &atsuFontID, &fbStyle) == kFMInvalidFontErr)
+	{
+		NS_WARNING("FMGetFontFromFontFamilyInstance failed");

This was supposed to be ATSU-based... there you go back again to FM.
Some FM functions were also reported as broken in bug 246527 comment 3.
Care to give a reference/link for your comment that "ATSUFONDtoFontID
is not recommended by Apple"

 	theValue[2] = (ATSUAttributeValuePtr) &color;
 	//--- Color -----
+	//--- Boldness ----
+	Boolean isBold = false;
+	if (aBold) {
+		isBold = true;
+	}

Is this not equivalent to "Boolean isBold = aBold"?
[Or at the very least Boolean isBold = aBold ? true : false]
Also a leave a blank line after "Color" and simply say "Bold"
instead of "Boldness".

+	theTag[3] = kATSUQDBoldfaceTag;
+	theValueSize[3] = (ByteCount) sizeof(Boolean);
+	theValue[3] = (ATSUAttributeValuePtr) &isBold;
+	//--- Italicness ----
+	Boolean isItalic;
+	if (aItalic) {
+		isItalic = true;
+	}

Same as above. Also another spot to insert a blank line and say "Italic".


Index: nsUnicodeRenderingToolkit.cpp
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/mac/nsUnicodeRenderingToolkit.cpp,v

         if(fontMapping.ConvertUnicodeToGlyphs(scriptFallbackFonts[fallbackScript], aCharPt, 1,
             buf, STACK_TRESHOLD, outLen, processBytes, kUnicodeLooseMappingsMask))
         {
             ::TextFont(scriptFallbackFonts[fallbackScript]);
             GetScriptTextBoundingMetrics(buf, outLen, fallbackScript, oBoundingMetrics);
             ::TextFont(fontNum);
-        }
+        } else {
+			return PR_FALSE;
+		}
         return PR_TRUE;
     }

What about moving the |return PR_TRUE| inside the |if|. This way, you get
         if(fontMapping.ConvertUnicodeToGlyphs))
         {
           ...
           return PR_TRUE;
         }
         return PR_FALSE;
         

     for(fallbackScript = 0; fallbackScript < 32; fallbackScript++)
     {
         if(BAD_FONT_NUM != scriptFallbackFonts[fallbackScript])
         {
             if(fontMapping.ConvertUnicodeToGlyphs(scriptFallbackFonts[fallbackScript], aCharPt, 1,
                 buf, STACK_TRESHOLD, outLen, processBytes, kUnicodeLooseMappingsMask))
             {
                 NS_PRECONDITION(0 == (processBytes % 2), "strange conversion result");
                 ::TextFont(scriptFallbackFonts[fallbackScript]);
                 GetScriptTextBoundingMetrics(buf, outLen, fallbackScript, oBoundingMetrics);
                 ::TextFont(fontNum);
                 break;
-            }
+            } else {
+				return PR_FALSE;
+			}
         }
     }


Why the return here? Seems to me you want to let the char to fall through
and put in the cache.


     if(0 == outLen)
         fallbackScript = BAD_SCRIPT;

     // put into cache
     cache->Set(*aCharPt, fallbackScript);
@@ -344,17 +348,19 @@ PRBool nsUnicodeRenderingToolkit :: TECF
       return PR_FALSE;

     if(fontMapping.ConvertUnicodeToGlyphs(scriptFallbackFonts[fallbackScript], aCharPt, 1,
         buf, STACK_TRESHOLD, outLen, processBytes, kUnicodeLooseMappingsMask))
     {
         ::TextFont(scriptFallbackFonts[fallbackScript]);
         DrawScriptText(buf, outLen, x, y, oWidth);
         ::TextFont(origFontNum);
-    }
+    } else {
+		return PR_FALSE;
+	}
     return PR_TRUE;
   }
   return PR_FALSE;

Also move the |return PR_TRUE| in the |if| so as not to use the |else|.

Comment 29

[Jungshik Shin]

(In reply to comment #27)
> (In reply to comment #24)
> > simon, do you have a Mac build environment?
> 
> Sorry, no. Passing the buck to Jungshik and Asaf.

Ok. I'll try the latest patch here (well, perhaps, rbs' comments are addressed) on Mac. 
 

Comment 30

[YAMASHITA Makoto]

Attachment: TEC Fallback, nsATSUIUtils patch refinement

Sorry for naive writing :(

Comment 31

[rbs]

Comment on attachment 201794 [details] [diff] [review]
TEC Fallback, nsATSUIUtils patch refinement

Looks good to me. Asking r=jshin per comment #29 so that the extra verifications can help with getting the approval for the 1.8 branch.

Comment 32

[Jungshik Shin]

Comment on attachment 201794 [details] [diff] [review]
TEC Fallback, nsATSUIUtils patch refinement

I applied the patch and confirmed that '-' gets rendered well.

>+	// The use of ATSUFONDtoFontID is not recommended, see http://developer.apple.com/documentation/Carbon/Reference/ATSUI_Reference/atsu_reference_Reference/chapter_1.2_section_19.html
>+	// the origin of the value of aFontNum is nsDeviceContextMac.cpp#820, FMGetNextFontFamily. So we cannot expecet that to be a simple FOND resource value.

Please, make the above comment fit within 80 chars-long lines  except for the url taking a line by itself. 

// The use of .... , see
// http://developer.... very long url 
// The origin of the value of aFontNum .....
// ....So we cannot ...

Comment 33

[YAMASHITA Makoto]

Attachment: nsATSUIUtils patch refinement

Sorry for re-requesting your review but I found out that I've misunderstood the data types. FMFontFamily is not equivalent to ATSFontFamilyRef and nsDeviceContextMac.cpp#820 generates exactly an FMFontFamily value. So
+	// the origin of the value of aFontNum is nsDeviceContextMac.cpp#820, FMGetNextFontFamily. So we cannot expecet that to be a simple FOND resource value.
+	FMFontFamily fontFam;
+	FMFontStyle fbStyle;
+	fontFam = ::FMGetFontFamilyFromATSFontFamilyRef(aFontNum);
+	if (::FMGetFontFromFontFamilyInstance(fontFam, 0, &atsuFontID, &fbStyle) == kFMInvalidFontErr) {
...
should be just
+	// by FMGetNextFontFamily, as an FMFontFamily.
+	FMFontStyle fbStyle;
+	if (::FMGetFontFromFontFamilyInstance(aFontNum, 0, &atsuFontID, &fbStyle) == kFMInvalidFontErr) {
...

Comment 34

[rbs]

No need for this comment:

+	// the value of aFontNum is born in nsDeviceContextMac.cpp#820,
+	// by FMGetNextFontFamily, as an FMFontFamily.

aFontNum is an argument ('a'FontNum). Anyone patching a function must trace where arguments come from, so saying where one comes from make no difference to the basic tenet. Also, the #line notation is is not meant for permanent references, the code evolves and line numbers change all the time.

Comment 35

[rbs]

Comment on attachment 201838 [details] [diff] [review]
nsATSUIUtils patch refinement

sr=rbs, the unneeded comment can be removed at check-in,

Comment 36

[rbs]

Comment on attachment 201838 [details] [diff] [review]
nsATSUIUtils patch refinement

Transfering jshin's review.

Comment 37

[rbs]

Checked in.

Comment 38

[rbs]

Comment on attachment 201838 [details] [diff] [review]
nsATSUIUtils patch refinement

Requesting the approval of drivers for the 1.8 branch. This patch is necessary to display the minus sign in MathML. Without the patch, the minus sign does not show up, hence expressions like "a - b" show up as "a b". As you can imagine, this corrupts the meaning of expressions, making them confusing and unintelligible. The matter is made even worse by the fact that minus is a common place symbol used all the time.

The patch touches two files. In nsUnicodeRenderingToolkit.cpp, it fixes misplaced |return|s in the TEC fallback code of GetBoundingMetrics/Drawtring -- this the root of the bug. In nsATSUIUtils, it replaces a non-recommended Apple API (ATSUFONDtoFontID) by something more reliable, and in the process enables the support of bold and italic.
http://developer.apple.com/documentation/Carbon/Reference/ATSUI_Reference/atsu_reference_Reference/chapter_1.2_section_19.html 

Unfortunately, as the Mac doesn't receive as much attention as other platforms, bugs such as these tend to hang around for a long time. The patch was finally produced by YAMASHITA Makoto, tested and reviewed by jshin on his Mac, and super-reviewed by me.

Approving the patch would mean that MathML is essentially in parity on all platforms out-of-the-box (modulo getting the fonts -- which all platforms have to do anyway, and setting to pref to get rid of the harmless alert about missing fonts).

Comment 39

[Asa Dotzler [:asa]]

Comment on attachment 201838 [details] [diff] [review]
nsATSUIUtils patch refinement

we've already got final bits and this is not a new problem. Mac will have to lag behind for one more release. We can try to get this in for the follow-up release.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What are the potential effects of this patch on non-MathML content?  Is this code used only for MathML, or for other things?

Comment 41

[David Harvey]

I've tried out the fix (together with prefs.js adjustments). Looks good to me. Very much appreciated by mac users methinks!

Comment 42

[rbs]

Re: comment #40

It is a very low-risk for non-MathML content. The code affected is in a special fallback codepath where we do the ConvertUnicodeToGlyphs business to map chars to glyphs in a symbolic font. Normal content rarely gets there because their glyphs are found early on in the normal fonts that don't involve the special symbolic fonts that MathML needs.

Comment 43

[Mike Schroepfer]

Comment on attachment 201838 [details] [diff] [review]
nsATSUIUtils patch refinement

Please consider for 1.8.1 -this is too risky for a minor patch.

Comment 44

[rbs]

*** Bug 323894 has been marked as a duplicate of this bug. ***

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 201838 [details] [diff] [review]
nsATSUIUtils patch refinement

branch-1.8.1=dbaron, but please get it in reasonably early

Comment 46

[rbs]

Checked in the 1.8 branch.

Comment 47

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 330723 has been marked as a duplicate of this bug. ***

Comment 48

[rbs]

*** Bug 340409 has been marked as a duplicate of this bug. ***