nsRenderContextBeOS needs implementations for the following: #ifdef MOZ_MATHML /** * Returns bounding metrics (in app units) of an 8-bit character string * @param aString string to measure * @param aLength number of characters in string * @return aBoundingMetrics struct that contains various metrics (see below) * @return error status */ NS_IMETHOD GetBoundingMetrics(const char* aString, PRUint32 aLength, nsBoundingMetrics& aBoundingMetrics) = 0; /** * Returns bounding metrics (in app units) of an Unicode character string * @param aString string to measure * @param aLength number of characters in string * @param aFontID an optional out parameter used to store a * font identifier that can be passed into the GetBoundingMetrics() * methods to speed measurements * @return aBoundingMetrics struct that contains various metrics (see below) * @return error status */ NS_IMETHOD GetBoundingMetrics(const PRUnichar* aString, PRUint32 aLength, nsBoundingMetrics& aBoundingMetrics, PRInt32* aFontID = nsnull) = 0; #endif

Daniel, I assigned this to you, since you had done work on the gfx package recently. I hope you don't mind. The attachment should at least allow the Tinderbox to build again, until we actually get time to implement these.

Oops, I should have compiled this before I posted it. Cut and past error fixed.

Second patch looks good Paul, let's just get things building again. r=mozilla@switkin.com

I checked in the temp fix.

Actual implementation

+ mCurrentFont->GetHeight(height); + aBoundingMetrics.ascent = height->ascent; + aBoundingMetrics.descent = height->descent; The bounding metrics' ascent & descent are not the same as the font' ascent & descent + mCurrentFont->GetEdges(aString,aLength,bearings); + aBoundingMetrics.width = mCurrentFont->StringWidth(aString); + aBoundingMetrics.leftBearing = bearings[0].left; + aBoundingMetrics.rightBearing = bearings[aLength -1].right; Unless GetEdges(() which I know not is adding things up, the right bearing is the width of aString[0..length-1] + the right bearking of last character -- a slightly more complicated formula can take care of kerning effets too. See the code in nsFontMetricsWin for example. A good way to test your implementation is to enable #define SHOW_BOUNDING_BOX 1: http://lxr.mozilla.org/seamonkey/source/layout/mathml/base/src/nsIMathMLFrame.h# 21 Then zoom while vieweing to check that, on MathML content, the bounding boxes that will be drawn cover exactly the text. For example, the bbox of <msup> should appear as the smallest rectangle that exactly encloses the base and the superscript.

Oops, my bad. I should have read the description in the header a little closer before just wacking out some code. I'll fix as soon as I find out how to get the proper info.

A "good" implementation of the GetBoundingMetrics methods used by the MathML renderer.

screen shot of torture test page, zoomed to 200%, with bounding boxes shown, using the above patch

+ // The origin in beos is the top left. so we switch these to give what MathML wants, bottom left + aBoundingMetrics.ascent = (nscoord)rect.bottom; + aBoundingMetrics.descent = (nscoord)rect.top; Drawing operations are now made on the baseline. I remember to have patched the BeOS code sometimes ago about that -- but it may have been reversed -- din't check. The screenshot shows that the metrics are still not returned correctly. The baseline is needed to allow you to compute: ascent = bottom - baseline descent = height - ascent (where height = bottom - top --per your comment above) I will attach a reference screenshot for your information.

btw, how does the MathML load the fonts it needs? i.e. where does it do this? I just installed the fonts, and now I'm getting blocks instead of characters.

note how at each level of nesting, elements are embedded in an hierarchy of precise bounding boxes in a way that mirrors the XML parenting relashionship per level.

>btw, how does the MathML load the fonts it needs? i.e. where does it do this? The fonts are loaded in the same way as mainline layout does (i.e., using nsIRenderingContext::SetFont(), etc). But for the fonts to be recognized properly, the ucvmath module needs to be hooked -- see similar bug 107146 - ucvmath not working with Fizzilla.

Well, I know seem to have it drawing the proper bounding boxes for the characters/strings, and the width is correct for the entire equation, but the height is wrong. Any ideas?

The testcase contrast the bounding metrics vs. the usual CSS metrics. The solid blue borders are done with the bounding metrics while the dashed red borders are done with the CSS metrics.

> but the height is wrong. Any ideas? The ascent and descent need to be relative to the baseline as I hinted earlier. The cross-platform origin expected higher-up in consumers of the GetBoundingMetrics() function is at the baseline.

as you can see, I am generating the proper bounding boxes for all of the fonts. The only bounding boxes that aren't correct, are the boxes generated based on my bounding boxes, the "global" boxes in the first 2 mathml test cases. If it is able to draw the proper bounding boxes for the strings, then it seems to me to be a problem with mathml calculating the "global" bounding box. Here's the debug output I in my patch for the test case: GetBoundingMetrics for: j (-7.000000,-76.000000,37.000000,22.000000) -> (-7.000000,37.000000,-22.000000,-76.000000) GetBoundingMetrics for: i (7.000000,-76.000000,36.000000,-1.000000) -> (7.000000,36.000000,1.000000,-76.000000) GetBoundingMetrics for: f (9.000000,-77.000000,48.000000,-1.000000) -> (9.000000,48.000000,1.000000,-77.000000) GetBoundingMetrics for: j (-5.000000,-54.000000,25.000000,15.000000) -> (-5.000000,25.000000,-15.000000,-54.000000) GetBoundingMetrics for: i (5.000000,-54.000000,25.000000,-1.000000) -> (5.000000,25.000000,1.000000,-54.000000) GetBoundingMetrics for: f (6.000000,-54.000000,33.000000,-1.000000) -> (6.000000,33.000000,1.000000,-54.000000) GetBoundingMetrics for: jif (-7.000000,-77.000000,96.000000,22.000000) -> (-7.000000,96.000000,-22.000000,-77.000000) GetBoundingMetrics for: jif (-5.000000,-54.000000,67.000000,15.000000) -> (-5.000000,67.000000,-15.000000,-54.000000)

>If it is able to draw the proper bounding boxes for the strings, then it seems >to me to be a problem with mathml calculating the "global" bounding box. The blue bounding boxes are drawn with something like (simplyfing...) : DrawRect(0, 0, ascent + descent, rightBearing - leftBearing) So although, ascent + descent may sum up to the height, it doesn't mean that ascent and descent are correct if taken indivually. As I mentioned earlier, these values need to be set relative to the baseline since this is what the other MathML rendering operations expect.

Code that is painting the bboxes: http://lxr.mozilla.org/seamonkey/source/layout/mathml/base/src/nsMathMLContainerFrame.cpp#770

Just did a lookup in google and it now seems to me that there is possibly another hole in your patch. (http://www.bespecific.com/dialog/bedevtalk/archive/990308/00000103.htm) + // Use the printing metric to get more detail + mCurrentFont->GetBoundingBoxesForStrings(&aString, 1, B_SCREEN_METRIC, &delta, &rect); Are you getting the rect of the *first* character only or what? To test this, you can edit the testcase and change "jif" to "ijf" throughout.

Errata... I was messed up with the names. The function that you are using applies to the full string indeed... GetBoundingBoxesForStrings(): returns an array of BRect objects indicating the bounding rectangles for an array of strings, one BRect per string. These rectangles enclose the entire string they represent.

Seems to be workign properly now. Still switching to top and bottom values, but now negating the descent instead of the ascent, to match how MathML expects things. I then had to offset the values by the total of ascent and descent, to move them to the proper cooridinate space.

beos rendering of test case showing that it now works.

Comment on attachment 73468 [details] [diff] [review] Hopefully the final patch + // BeOS origin at top left, so the bottom and top values are switched + // for ascent and descent, but we then need to offset those values + // by there totals to obtain the proper bounding box. + float offset = rect.bottom + rect.top; + + // Flip sign of descent for cross-platform compatibility + aBoundingMetrics.ascent = (nscoord)(rect.bottom - offset); + aBoundingMetrics.descent = (nscoord)(0.0 - rect.top + offset); It appears that the documentation (comment?) is buggy. Indeed the formulas are simply reducing to: ascent = rect.bottom - offset = rect.bottom - (rect.bottom + rect.top) = -rect.top descent = - rect.top + offset; = - rect.top + (rect.bottom + rect.top) = rect.bottom [I just checked that you didn't try this combination in your earlier patch...] So the origin is still at the baseline on BeOS, ...but it is the orientation that is _downward_ for _all_ vertical metrics (ascent & descent). The MathML engine expects what X Windows does: the orientation of the ascent is upward (i.e., a postive ascent means the glyph extends above the baseline), while the descent is oriented downward (i.e., a positive descent means the glyph extends below the baseline). So flipping the sign of the ascent on BeOS turns the ascent from downward to upward and this brings things to parity. [On Win32, the orientation is upward for all vertical metrics and that's why I had to flip its ascent to turn it from upward to downward. Hope this quick explantation makes sense...] + aBoundingMetrics.rightBearing = (nscoord)rect.right + 1; Why '+1' ? have you encouuntered fonts with bad metrics that make this necessary? On the screenshot, there seems to be little gaps as a result of this. Since BeOS is using floating arithmetic for its device values, you don't need the casting that may cause even more truncation errors, you could directly use: + aBoundingMetrics.leftBearing = NSToCoordRound(rect.left * mP2T); + aBoundingMetrics.rightBearing = NSToCoordRound(rect.right * mP2T); + aBoundingMetrics.width = NSToCoordRound(width * mP2T); + aBoundingMetrics.ascent = NSToCoordRound(-rect.top * mP2T); + aBoundingMetrics.descent = NSToCoordRound(rect.bottom * mP2T);

I think I was a bit too excited that it was drawing properly, that I did not notice the uglyness of my code. It is now cleaned up, and hopefully the comment explains things better.

+ uint8 *utf8str = new uint8[aLength * 4 + 1]; + uint8 *utf8ptr = utf8str; I was concentratimg on the correctness and didn't pay much attention to other parts... More often than not, there is only a single character passed to the function, and you will be |new|ing all the time. It might be best to use a stack variable and only |new| if the length exceeds the stack. With this, the |new| is rarely going to happen. So: uint8 utf8buf[1024]; uint8* utf8str = &utf8buf; if (aLength * 4 + 1 > 1024) utf8str = new uint8[aLength * 4 + 1]; ...use it... if (utf8str != utf8buf) delete[] utf8str; [There is similar code along these lines in GfxWin, BTW]

updated to try and keep the utf8 conversion on the stack for single char strings

Comment on attachment 73496 [details] [diff] [review] updated patch, as per comments Need some null check in case |new| fails, but just looked up and noted that file is not bothering doing null checks for GetWidth/DrawString/... and to be meaningful, null checks would have to be scattered everywhere throughout that file at some stage. So r=rbs for the patch

Comment on attachment 73496 [details] [diff] [review] updated patch, as per comments a=roc+moz

Patch checked in. Marking fixed.

v