STEPS TO REPRODUCE <mfrac bevelled="true"> <mi>x</mi> <mi>y</mi> </mfrac> ACTUAL RESULTS , a / b ' EXPECTED RESULTS a / / / b (Well, maybe not as exaggerated as that, but you get the idea.) See: http://www.w3.org/TR/MathML2/image/f3008.gif

@Exo 11.6 in the TeXbook - Appendix A

\newcommand{\sfrac}[2]{% \hbox{\kern 0.1em% \raise 0.5ex\hbox {\scriptsize$#1$}% \kern -0.1em $/$% \kern -0.15em% \lower 0.25ex\hbox {\scriptsize$#2$}}% \kern 0.2em}

Though probably unrelated, it is interesting to note here is that in the latest nightly, <mo>/</mo> doesn't stretch in accordance to the specification http://www.w3.org/TR/MathML2/chapter3.html#id.3.2.5.8.2 . In 1.8 branch (Seamonkey 1.1.13) with tex fonts installed, vertical stretching for / operator works, and scales according to its components, though the positioning still doesn't conform to the example stated by OP.

Currently to draw the <mfrac/> with bevelled="true", we use the methods of nsMathMLmfencedFrame with a SlashChar as a separator. We should rather directly implement the positioning in nsMathMLmfracFrame so that baselines could be shifted. If we choose to keep the SlashChar, then this bug depends on other bugs related to stretching according to comment 4 . Moreover, <mfrac/> has a linethickness attribute that should also be taken into account (I suppose the *align attributes may be ignored for bevelled mfrac). The font-weight property can be used on the SlashChar, but I'm not sure it will be precise enough. An alternative way to draw the bar could be to replace the SlashChar by a gfxContext::Line.

One advantage of the SlashChar was that its thickness could grow appropriately with the height (when bug 407101 is fixed), but for linethickness = length or thin we'd need to revert to drawing a line, so perhaps its easiest and more consistent to always use a line.

Here is an experimental patch. It applies on top of bug 297467, since the slash bar is drawn using nsMathMLmencloseFrame::DisplayNotation. I don't know if it's worth sharing the implementation of this function between nsMathMLmfracFrame and nsMathMLmencloseFrame (I'm aware that the way this is currently made is not good).

Now bug 297467 is fixed, I see two possibilities to draw the slash: 1) Implement a new class nsDisplayMathMLSlash inside nsMathMLmfracFrame.cpp, that inherits from nsDisplayItem. 2) Use the class nsDisplayNotation that has been written for menclose and draw a NOTATION_UPDIAGONALSTRIKE. The second option allows to share implementation but I think it would require to move the class nsDisplayNotation and the enum nsMencloseNotation to a higher level, presumably in nsMathMLFrame.cpp where there is already nsDisplayMathMLBar. So maybe it's simpler to use the first option.

What do you think about a nsDisplayMathMLStroke (or similar) in nsMathMLFrame that has start point, end point, and thickness as arguments? The differences from nsDisplayMathMLBar would be that the line would not need to be axis aligned, and the ends of the stoke would be round. Can you include function names and additional lines of context in future patches, please? This can be done automatically by adding a [diff] section to ~/.hgrc or .hg/hgrc: https://developer.mozilla.org/en/Mercurial_FAQ#Configuration

(In reply to comment #12) > What do you think about a nsDisplayMathMLStroke (or similar) in nsMathMLFrame > that has start point, end point, and thickness as arguments? > The differences from nsDisplayMathMLBar would be that the line would not need > to be axis aligned, and the ends of the stoke would be round. > That seems a good idea, but in the special case of mfrac slash, I don't know if the ends should be rounded. I rather see the slash as a "parallelogram". > Can you include function names and additional lines of context in future > patches, please? This can be done automatically by adding a [diff] > section to ~/.hgrc or .hg/hgrc: > https://developer.mozilla.org/en/Mercurial_FAQ#Configuration OK, I've just configured my hgrc according to these instructions.

(In reply to comment #13) > I don't know if the ends should be rounded. > I rather see the slash as a "parallelogram". Yes. I agree with your preference, and most fonts seem to draw SOLIDUS as a parallelogram. That would suggest that a new class nsDisplayMathMLSlash inside nsMathMLmfracFrame.cpp would be best.

(In reply to comment #11) > Created an attachment (id=397551) [details] > Position numerator & denominator relative to the baseline Oops, I sent the wrong patch. I'll give an updated version later.

This is the actual patch where numerator & denominator are placed relative to the baseline. Moreover, I wrote a function nsDisplayMathMLSlash inside nsMathMLmfracFrame.cpp so that the slash is drawn as a parallelogram.

More testcases: http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/mfrac/mfracAbevelled16-simple.xhtml http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/mfrac/mfracZComp1-simple.xhtml http://www.maths-informatique-jeux.com/international/mathml_test/mfrac.xml

Here is how the patch currently works: As in previous implementation, the elements are placed from left to right according to the following order: Numerator, Slash, Denominator. Numerator (respectively Denominator) is raised (respectively lowered) by 50% (respectively 25%) of its height. These values are inspired from comment 3. The size of the slash is determined in a way that it covers at least the height given by (top of numerator - bottom of denominator). Because the slash is supposed to separate numerator and denominator, it must be drawn "almost vertically". To achieve this, I chose an arbitrary relation slashHeigh = 3 * slashWidth so that the slash bounding box is vertical. Moreover for large thickness, if we want to be able to draw the slash inside its bounding box we need a minimal size. Hence I added a minimal height of slashRatio * 5 * mLineThickness (5 is arbitrary) and consequently a minimal width of 5 * mLineThickness. The choice of this minimal height can be an issue for large thickness because in this case the slash become much bigger than the Numerator/Denominator. The only workaround I saw is to limit the value of thickness. But this is not done for classical mfrac so I don't see why it should be the case for bevelled mfrac...

Comment on attachment 397637 [details] [diff] [review] Position of num/den + drawing Slash as a parallelogram nsMathMLmfenceFrame.cpp functions that check whether they are called from mfrac should be updated too.

Is it correct to remove these lines when mfrac no longer uses nsMathMLmfencedFrame.cpp? - // compute the bounding metrics right now for mfrac - nscoord childDescent = childDesiredSize.height - childDesiredSize.ascent; - if (descent < childDescent) - descent = childDescent; - if (ascent < childDesiredSize.ascent) - ascent = childDesiredSize.ascent; - aDesiredSize.mBoundingMetrics += childDesiredSize.mBoundingMetrics; - http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmfencedFrame.cpp#326

(In reply to comment #21) Looks like aDesiredSize.mBoundingMetrics is only calculated for if (!NS_MATHML_WILL_STRETCH_ALL_CHILDREN_VERTICALLY(presentationData.flags)) { // case when the call is made for mfrac, we only need to stretch the '/' separator containerSize = aDesiredSize.mBoundingMetrics; // computed earlier } It might be best to leave the code for ascent and descent though as I'm don't think child frames are necessarily nsIMathMLFrames.

> It might be best to leave the code for ascent and descent though as I'm don't > think child frames are necessarily nsIMathMLFrames. But it seems that, when the call is not made from mfrac, these values are updated after stretching: http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmfencedFrame.cpp#369

mathmlChild will only be non-NULL when childFrame is an nsIMathMLChild. http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmfencedFrame.cpp#356

childFrame is an nsIMathMLFrame, I mean.

This new version removes the functions doReflow and doGetIntrinsicWidth from nsMathMLmfencedFrame and move their bodies in Reflow and GetIntrinsicWidth. In the former function, the computation of aDesiredSize.mBoundingMetrics discussed above and the special case when the call is made for mfrac are removed.

The patch needs a "replace all": PR_MIN => NS_MIN PR_MAX => NS_MAX

Comment on attachment 400532 [details] [diff] [review] Update nsMathMLmfencedFrame In nsMathMLmfencedFrame.cpp, can you order the functions so that as much code as possible stays in the same order as in previous versions of the file, please? This makes it easier for me now and others in the future to work out exactly what changes are being made in the code. Also, although I would like new code to be within 80 columns, I'd prefer not to make multiple whitespace changes to wrap old code into 80 columns. Again, this is make it easier for people in the future to work out which code changed and when. Please keep the spelling correction and this change though: return nsMathMLContainerFrame:: UpdatePresentationDataFromChildAt(aFirstIndex, aLastIndex, - aFlagsValues, aFlagsToUpdate); + aFlagsValues, aFlagsToUpdate); (as it makes the code significantly easier to follow). (In reply to comment #19) > Here is how the patch currently works: > > As in previous implementation, the elements are placed from left to right > according to the following order: Numerator, Slash, Denominator. Numerator > (respectively Denominator) is raised (respectively lowered) by 50% > (respectively 25%) of its height. These values are inspired from comment 3. > > The size of the slash is determined in a way that it covers at least the > height given by (top of numerator - bottom of denominator). Because the slash > is supposed to separate numerator and denominator, it must be drawn "almost > vertically". To achieve this, I chose an arbitrary relation slashHeigh = 3 * > slashWidth so that the slash bounding box is vertical. + mBoundingMetrics.ascent = + PR_MAX(bmNum.ascent + numShift, slashMinHeight / 2); + mBoundingMetrics.descent = + PR_MAX(bmDen.descent + denShift, slashMinHeight / 2); This looks like it should work well when the numerator and denominator are similar heights, but it seems that additional rules may be needed when one is much taller than the other. I think the top of the slash should be at least as high as the top of the denominator and the bottom of the slash at least as low as the bottom of the numerator (at least while the slash is within the maximum limit I consider below). Also, do you think the top of the numerator should be at least as high as the top of the denominator (and the reverse for the bottoms)? > Moreover for large > thickness, if we want to be able to draw the slash inside its bounding box we > need a minimal size. Hence I added a minimal height of slashRatio * 5 * > mLineThickness (5 is arbitrary) and consequently a minimal width of 5 * > mLineThickness. > > The choice of this minimal height can be an issue for large thickness > because in this case the slash become much bigger than the > Numerator/Denominator. The only workaround I saw is to limit the value of > thickness. But this is not done for classical mfrac so I don't see why it > should be the case for bevelled mfrac... + nscoord slashMinHeight = slashRatio * 5 * mLineThickness; + mLineRect.width = + (mBoundingMetrics.ascent + mBoundingMetrics.descent) / slashRatio; With this formula, the slope of the slash depends on the line thickness. Also, all that's really needed for large thicknesses is that mLineRect.width > mLineThickness. How about this?: mLineRect.width = (mBoundingMetrics.ascent + mBoundingMetrics.descent) / slashRatio + mLineThickness; There may be some value in making sure that the slash is at least as long as it is wide. slashMinHeight = 2 * mLineThickness; However, a slash that can become arbitrarily wide, depending on the height of other elements, causes problems for our layout algorithm when the mfrac is a descendant of a table. The best I can suggest is a maximum width (above which maybe the slope will change), and use this width when the intrinsic widths are calculated. If the maximum width is hit it may be better to also limit the height to avoid the slash becoming too vertical - I'm not sure which is the lesser evil here. + // by default, leave at least one-pixel padding at either end, or use + // lspace & rspace that may come from <mo> if we are an embellished + // container (we fetch values from the core since they may use units that + // depend on style data, and style changes could have occurred in the + // core since our last visit there) + nsEmbellishData coreData; + GetEmbellishDataFrom(mEmbellishData.coreFrame, coreData); + nscoord leftSpace = PR_MAX(onePixel, coreData.leftSpace); + nscoord rightSpace = PR_MAX(onePixel, coreData.rightSpace); I think leftSpace and rightSpace should be considered even when an mfrac embellishing an operator is bevelled. (I'm not commenting on onePixel though; your padding is likely better). nsStyleContext* nsMathMLmfracFrame::GetAdditionalStyleContext(PRInt32 aIndex) const { - if (!mSlashChar) { - return nsnull; - } - switch (aIndex) { - case NS_SLASH_CHAR_STYLE_CONTEXT_INDEX: - return mSlashChar->GetStyleContext(); - break; - default: - return nsnull; - } + return nsnull; } void nsMathMLmfracFrame::SetAdditionalStyleContext(PRInt32 aIndex, nsStyleContext* aStyleContext) { - if (!mSlashChar) { - return; + return; +} These methods can be removed now that they don't do anything. The class will inherit the methods from nsFrame. + PRBool mIsBevelled; nsRect mLineRect; nsMathMLChar* mSlashChar; + nscoord mLineThickness; To help packing of member variables, a good rule of thumb is to put larger members first. Also we usually use PRPackedBool for member variables, so mIsBevelled would be last.

(In reply to comment #28) > (From update of attachment 400532 [details] [diff] [review]) > In nsMathMLmfencedFrame.cpp, can you order the functions so that as much code > as possible stays in the same order as in previous versions of the file, > please? This makes it easier for me now and others in the future to work out > exactly what changes are being made in the code. As I said in comment 26, I moved the bodies of doReflow and doGetIntrinsicWidth into Reflow and GetIntrinsicWidth. As a consequence, the static functions they call need either to be moved above or to be pre-declared. However, I understand it is difficult to review this... sorry. I think I can split the patch, one to locally modify the code and another to move blocks of code. What do you suggest I do? > > Also, although I would like new code to be within 80 columns, I'd prefer not > to make multiple whitespace changes to wrap old code into 80 columns. Again, > this is make it easier for people in the future to work out which code changed > and when. I find really uncomfortable to read code with more than 80 columns, and would appreciate if this can be fixed. Nevertheless, I'll remove these changes from the patch so that it will be more readable. > I think the top of the slash should be at least as high as the top of the > denominator and the bottom of the slash at least as low as the bottom of the > numerator (at least while the slash is within the maximum limit I consider > below). It is what the patch currently does (except when we need to increase the slash height because of a too large mLineThickness). > Also, do you think the top of the numerator should be at least as high as the > top of the denominator (and the reverse for the bottoms)? Yes I agree. I suggest to apply a correction before the shifts of 50%/25%. If you orient the Y-axis from bottom to top, let delta = max(den_top - num_top, den_bottom - num_bottom). If delta > 0, then raise/lower the num/denum by delta/2 (hence the relative shift is delta). In the worst case, the num and denum are aligned. Then we can apply the shifts of 50%/25%. > With this formula, the slope of the slash depends on the line thickness. > Also, all that's really needed for large thicknesses is that mLineRect.width > > mLineThickness. How about this?: > > mLineRect.width = > (mBoundingMetrics.ascent + mBoundingMetrics.descent) / slashRatio > + mLineThickness; > > There may be some value in making sure that the slash is at least as long as > it is wide. > > slashMinHeight = 2 * mLineThickness; > Normally I wrote this in a way that the slope is given by slashRatio (BTW I can call it slashSlope if you prefer). + aDesiredSize.height = + mBoundingMetrics.ascent + mBoundingMetrics.descent + 2 * padding; + mLineRect.SetRect(padding + bmNum.width, + padding, + mLineRect.width, + aDesiredSize.height - 2 * padding); So at the end mLineRect.width = (mBoundingMetrics.ascent + mBoundingMetrics.descent) / slashRatio mLineRect.height = aDesiredSize.height - 2 * padding = (mBoundingMetrics.ascent + mBoundingMetrics.descent) I think what you suggest would be more or less the result obtained with these parameters: slashRatio = 2 slashMinHeight = slashRatio * mLineThickness (but I prefer a vertical slash, so I think slashRatio = 3 is good) > However, a slash that can become arbitrarily wide, depending on the height of > other elements, causes problems for our layout algorithm when the mfrac is a > descendant of a table. The best I can suggest is a maximum width (above which > maybe the slope will change), and use this width when the intrinsic widths are > calculated. > Yes, I forgot this issue again. So I guess I need to reimplement GetIntrinsicWidth... I'm not really sure which value to take for the maximum width of the slash, maybe something like max_width = max(mLineThickness, some constant)? > If the maximum width is hit it may be better to also limit the height to avoid > the slash becoming too vertical - I'm not sure which is the lesser evil here. > I think I would prefer a slash too vertical that cover the whole height than a slash too small (in the later case, there could be some strange vertical spaces between the slash and the num/denum).

(In reply to comment #29) > (In reply to comment #28) > > (From update of attachment 400532 [details] [diff] [review] [details]) In > > nsMathMLmfencedFrame.cpp, can you order the functions so that as much code > > as possible stays in the same order as in previous versions of the file, > > please? This makes it easier for me now and others in the future to work > > out exactly what changes are being made in the code. > > As I said in comment 26, I moved the bodies of doReflow and > doGetIntrinsicWidth into Reflow and GetIntrinsicWidth. As a consequence, the > static functions they call need either to be moved above or to be > pre-declared. However, I understand it is difficult to review > this... sorry. I think I can split the patch, one to locally modify the code > and another to move blocks of code. What do you suggest I do? If you put GetIntrinsicWidth where doGetIntrinsicWidth was then the code contained won't need move and the static functions won't need to move. > > Also, although I would like new code to be within 80 columns, I'd prefer > > not to make multiple whitespace changes to wrap old code into 80 columns. > > Again, this is make it easier for people in the future to work out which > > code changed and when. > > I find really uncomfortable to read code with more than 80 columns, and would > appreciate if this can be fixed. Nevertheless, I'll remove these changes from > the patch so that it will be more readable. If it's causing problems, then a patch that only changes whitespace could be OK if landed with a comment that indicates the changeset is only whitespace changes. > > > I think the top of the slash should be at least as high as the top of the > > denominator and the bottom of the slash at least as low as the bottom of the > > numerator (at least while the slash is within the maximum limit I consider > > below). > > It is what the patch currently does (except when we need to increase the slash > height because of a too large mLineThickness). The code I quoted in comment 28 made the top of the slash at least as high as the top of the _numerator_. I didn't see anything that ensured that it was as high as the top of the denominator. > > Also, do you think the top of the numerator should be at least as high as > > the top of the denominator (and the reverse for the bottoms)? > > Yes I agree. I suggest to apply a correction before the shifts of 50%/25%. If > you orient the Y-axis from bottom to top, let delta = max(den_top - num_top, > den_bottom - num_bottom). If delta > 0, then raise/lower the num/denum by > delta/2 (hence the relative shift is delta). In the worst case, the num and > denum are aligned. Then we can apply the shifts of 50%/25%. Something like that I think. However, if the denominator is much larger than the numerator, then moving the denominator down after alignment by half its height would mean a large vertical gap between the numerator and denominator. I wonder whether one of the motivations for using bevelled fractions might be a desire to use less vertical space? Currently for very tall operands of similar height they only overlap by about 1/4 of their height, which means that the bevelled fraction is still 7/8 of the height of the stacked fraction. Note that the TeX shifts in comment 3 are absolute values rather than depending on their height. Perhaps the sum of the alignment delta and an absolute shift might work well? You've done more research and experimentation on this than I so you probably have a better feel for what works. > > With this formula, the slope of the slash depends on the line thickness. > > Also, all that's really needed for large thicknesses is that > > mLineRect.width > mLineThickness. How about this?: > > > > mLineRect.width = > > (mBoundingMetrics.ascent + mBoundingMetrics.descent) / slashRatio > > + mLineThickness; > > > > There may be some value in making sure that the slash is at least as long as > > it is wide. > > > > slashMinHeight = 2 * mLineThickness; > > > > Normally I wrote this in a way that the slope is given by slashRatio (BTW I > can call it slashSlope if you prefer). I like slashRatio. > > + aDesiredSize.height = > + mBoundingMetrics.ascent + mBoundingMetrics.descent + 2 * padding; > > + mLineRect.SetRect(padding + bmNum.width, > + padding, > + mLineRect.width, > + aDesiredSize.height - 2 * padding); > > So at the end > > mLineRect.width = (mBoundingMetrics.ascent + mBoundingMetrics.descent) / > slashRatio > mLineRect.height = aDesiredSize.height - 2 * padding = (mBoundingMetrics.ascent > + mBoundingMetrics.descent) This makes the slope of the diagonal of mLineRect equal to slashRatio. However, nsDisplayMathMLSlash::Paint subtracts mLineThickness from the horizontal dimension making the actual slash slope more vertical, and it gets more vertical as the thickness increases. I think it would be better if slashes of different thicknesses maintained the same slope. > I'm not really sure which value to take for the maximum > width of the slash, maybe something like max_width = max(mLineThickness, some > constant)? For similar reasons as above, I'd suggest some_constant + mLineThickness. We know that space will be needed for mLineThickness. (If anything, a larger mLineThickness might be a hint of larger operands and the need for a larger slash.) some_constant will just have to be some compromise between having reasonable slope with large operands and not too much excess space in tables with small operands.

(In reply to comment #30) > The code I quoted in comment 28 made the top of the slash at least as high as > the top of the _numerator_. I didn't see anything that ensured that it was as > high as the top of the denominator. OK, you're right. But now it is not a problem with the shift alignment. > I wonder whether one of the motivations for using bevelled fractions might be > a desire to use less vertical space? Maybe. It does not seem to be the case in the screenshot given in comment 1. The patch now deals with two cases according to whether displaystyle is true or false (see below). > Currently for very tall operands of similar height they only overlap by about > 1/4 of their height, which means that the bevelled fraction is still 7/8 of > the height of the stacked fraction. Note that the TeX shifts in comment 3 are > absolute values rather than depending on their height. > > Perhaps the sum of the alignment delta and an absolute shift might work well? The current patch adds the constants given by TeX to the alignment delta. If we are in displaymode="true", these constants are replaced by 50% of the smallest height. ------- I add a slashMaxWidthConstant, so that mLineRect.width = mLineThickness + slashMaxWidthConstant for GetIntrinsicWidth. Moreover slashMinHeight is now NS_MIN(slashRatio * 2 * mLineThickness, slashMaxWidthConstant * slashRatio)

Comment on attachment 406263 [details] [diff] [review] slight changes >- const nsStyleFont* font = aForFrame->GetStyleFont(); >+ const nsStyleFont* font = this->GetStyleFont(); just "= GetStyleFont();" There are several other similar "this->" calls in this file. > return nsMathMLContainerFrame:: >- AttributeChanged(aNameSpaceID, aAttribute, aModType); >+ AttributeChanged(aNameSpaceID, aAttribute, aModType); No need for this whitespace change. >- if (mSlashChar) { >- // bevelled rendering >- rv = mSlashChar->Display(aBuilder, this, aLists); >- } else { >+ if (mIsBevelled) >+ rv = DisplaySlash(aBuilder, this, mLineRect, mLineThickness, aLists); >+ else > rv = DisplayBar(aBuilder, this, mLineRect, aLists); >- } https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide says "Always brace controlled statements, even a single-line consequent of an if else else. This is redundant typically, but it avoids dangling else bugs, so it's safer at scale than fine-tuning." These files already have unbraced control substatements so the code style doesn't need to be applied too strongly, but I'd rather not remove the braces that are already there. (BTW, one case where I personally don't think bracing is actually necessary is when the substatement is a single jump statement - break, continue, return, or goto - because then it is always clear that no subsequent statements are intended to be part of a compound statement.) I haven't really studied all of nsMathMLmfracFrame::PlaceInternal() yet, but some comments/questions: >+ nscoord slashMaxWidthConstant = 1500; I think font size and zoom should be taken into account somewhere, and that could probably be done reasonably easily by including a factor of em height or similar. >+ if (aWidthOnly) { >+ mLineRect.width = mLineThickness + slashMaxWidthConstant; >+ } else { >+ mLineRect.width = mLineThickness + >+ (mBoundingMetrics.ascent + mBoundingMetrics.descent) / slashRatio; >+ } The maximum value of mLineRect.width for !aWidthOnly really should be limited to the value returned for aWidthOnly. (Otherwise we'll get more expression protruding out of tables.) From (the end of) comment 29, it sounds like you preferred making the slash more vertical to fit (by simply placing a maximum on the width). >+ nscoord slashMinHeight = slashRatio * >+ NS_MIN(2 * mLineThickness, slashMaxWidthConstant); I'm having trouble understanding what the goal of considering slashMaxWidthConstant here is. Can you explain, please?

(In reply to comment #35) > >+ if (aWidthOnly) { > >+ mLineRect.width = mLineThickness + slashMaxWidthConstant; > >+ } else { > >+ mLineRect.width = mLineThickness + > >+ (mBoundingMetrics.ascent + mBoundingMetrics.descent) / slashRatio; > >+ } > > The maximum value of mLineRect.width for !aWidthOnly really should be limited > to the value returned for aWidthOnly. Yes, sorry it's a mistake. I should rather use mLineRect.width = mLineThickness + PR_MAX(mBoundingMetrics.ascent + mBoundingMetrics.descent) / slashRatio, slashMaxWidthConstant); > > >+ nscoord slashMinHeight = slashRatio * > >+ NS_MIN(2 * mLineThickness, slashMaxWidthConstant); > > I'm having trouble understanding what the goal of considering > slashMaxWidthConstant here is. Can you explain, please? I also have trouble to remember why I did that. What I'm sure is that making slashMinHeight only depends on mLineThickness produced strange rendering for large values of thickness: the slash was much more taller than the num & denum and consequently we could have a lot of space above and below the mfrac. Hence I prefered to limit the height: the slash is no longer vertical for large value but it's not worth that having this strange rendering. I think the rationale for this particular value is that slashRatio * slashMaxWidthConstant is the least height that sets the slash width to mLineThickness + slashMaxWidthConstant, so in a sense is the maximum height expected.

(In reply to comment #36) > PR_MAX(mBoundingMetrics.ascent + mBoundingMetrics.descent) / slashRatio, > slashMaxWidthConstant); I meant PR_MIN, of course...

(In reply to comment #36) > ... I think the rationale > for this particular value is that slashRatio * slashMaxWidthConstant is the > least height that sets the slash width to mLineThickness + > slashMaxWidthConstant, so in a sense is the maximum height expected. OK. Can you add a comment, please. Something like "For large line thicknesses the minimum slash height is limited to the largest expected height of a fraction".

Comment on attachment 406263 [details] [diff] [review] slight changes > nsMathMLmfracFrame::GetIntrinsicWidth(nsIRenderingContext* aRenderingContext) > { >- if (mSlashChar) { >- // bevelled rendering >- return nsMathMLmfencedFrame::doGetIntrinsicWidth(aRenderingContext, this, >- nsnull, nsnull, >- mSlashChar, 1); >- } >+ nsHTMLReflowMetrics desiredSize; > >- // default rendering >- return nsMathMLContainerFrame::GetIntrinsicWidth(aRenderingContext); >+ PlaceInternal(*aRenderingContext, >+ PR_FALSE, >+ desiredSize, >+ PR_TRUE); >+ >+ return desiredSize.width; I think you mean to replace GetIntrinsicWidth with MeasureForWidth; otherwise child widths are not (necessarily) calculated. >+ nscoord xHeight = 0; >+ nsCOMPtr<nsIFontMetrics> fm; >+ aRenderingContext.GetFontMetrics(*getter_AddRefs(fm)); fm has already been declared and set in this function, so you should be able to just reuse that. PlaceInternal() looks to me like it should work well.

(In reply to comment #35) > >+ nscoord slashMaxWidthConstant = 1500; > > I think font size and zoom should be taken into account somewhere, and that > could probably be done reasonably easily by including a factor of em height or > similar. I now use GetEmHeight, but it seems it is still independent of zoom: http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLFrame.h#352

Comment on attachment 420898 [details] [diff] [review] Patch >+ GetEmHeight(fm, em); (In reply to comment #41) > I now use GetEmHeight, but it seems it is still independent of zoom: > http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLFrame.h#352 Use fm->GetEmHeight(em) directly. (Looks like nsMathMLFrame::GetEmHeight() is probably no longer needed and we'd be better off without it.) >+ PR_MIN(slashMaxWidthConstant, Use NS_MIN. (It's an inline (template) function not a macro.) > virtual nscoord >- GetIntrinsicWidth(nsIRenderingContext* aRenderingContext); >+ MeasureForWidth(nsIRenderingContext* aRenderingContext); The signature needs to match that of nsMathMLContainerFrame::MeasureForWidth (which is different).

Comment on attachment 421027 [details] [diff] [review] Patch Can you check that fractions with tall operands don't overflow in tables, please? It should be possible to add a case to this reftest: http://hg.mozilla.org/mozilla-central/annotate/cea14ad20b61/layout/reftests/mathml/table-width-1.xhtml It looks like the wrong MeasureForWidth will be used: In file included from /home/karl/moz/dev/layout/mathml/nsMathMLmfracFrame.cpp:51: /home/karl/moz/dev/layout/mathml/nsMathMLContainerFrame.h:243: warning: 'virtual nsresult nsMathMLContainerFrame::MeasureForWidth(nsIRenderingContext&, nsHTMLReflowMetrics&)' was hidden /home/karl/moz/dev/layout/mathml/nsMathMLmfracFrame.h:108: warning: by 'virtual nsresult nsMathMLmfracFrame::MeasureForWidth(nsIRenderingContext*, nsHTMLReflowMetrics&)'

(In reply to comment #44) > (From update of attachment 421027 [details] [diff] [review]) > Can you check that fractions with tall operands don't overflow in tables, > please? I tested with a MeasureForWidth that returns 0 and the one of attachment 421294 [details] [diff] [review], overflows in table can happen with the former but not the latter. > It should be possible to add a case to this reftest: > http://hg.mozilla.org/mozilla-central/annotate/cea14ad20b61/layout/reftests/mathml/table-width-1.xhtml > OK, I'll do that later.

(In reply to comment #44) > (From update of attachment 421027 [details] [diff] [review]) > Can you check that fractions with tall operands don't overflow in tables, > please? > It should be possible to add a case to this reftest: > http://hg.mozilla.org/mozilla-central/annotate/cea14ad20b61/layout/reftests/mathml/table-width-1.xhtml OK, I see what you mean now. Apparently, the width of the operands is not computed correctly for aWidthOnly = PR_TRUE, when using GetReflowAndBoundingMetricsFor. I need to modify the patch to use nsLayoutUtils::IntrinsicForContainer(aRenderingContext, childFrame, nsLayoutUtils::PREF_WIDTH) at some point.

Please ignore comment 48. The widths of children are well computed in nsMathMLContainerFrame::GetIntrinsicWidth and retrieved by nsMathMLmfracFrame::MeasureForWidth. The problem of the testcase is due to the use of <mspace/>'s whose width is not well estimated.

Comment on attachment 421294 [details] [diff] [review] Fix signature of MeasureForWidth >+ FinishReflowChild(frameNum, presContext, nsnull, sizeNum, >+ leftSpace, >+ aDesiredSize.ascent - numShift - bmNum.ascent, 0); >+ FinishReflowChild(frameDen, presContext, nsnull, sizeDen, >+ leftSpace + bmNum.width + mLineRect.width, >+ aDesiredSize.ascent + denShift - bmDen.ascent, 0); Sorry I didn't spot this before, but bmNum.ascent and bmDen.ascent should be sizeNum.ascent and sizeDen.ascent, as it is the top of the logical rect that is positioned. Otherwise looks good, thanks!

(In reply to comment #53) > http://hg.mozilla.org/mozilla-central/rev/2c2c3169ebba Thanks. This bug also contains a reftest in a separate patch that has not been checked in (attachment 421421 [details] [diff] [review]).

Comment on attachment 421421 [details] [diff] [review] Add a reftest for large width http://hg.mozilla.org/mozilla-central/rev/2acaf0829cba