297467 - menclose is shown wrong (not implemented)

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Martin Mokrejs]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050612
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050612

All these tests are shown wrong:

http://www.w3.org/Math/testsuite/testsuite/Presentation/GeneralLayout/menclose/rec-enclose1.xml
http://www.w3.org/Math/testsuite/testsuite/Presentation/GeneralLayout/menclose/rec-enclose2.xml
http://www.w3.org/Math/testsuite/testsuite/Presentation/GeneralLayout/menclose/rec-enclose3.xml


MOZ_DEBUG_FLAGS="-g3 -O0 -ggdb" CFLAGS="-g3 -O0 -ggdb" CXXFLAGS="-g3 -O0 -ggdb"
./configure --enable-debug --disable-optimize --enable-debug-modules=all
--enable-debugger-info-modules --enable-detect-webshell-leaks --enable-svg
--enable-svg-renderer-libart --enable-image-decoders=all --with-qtdir=/usr/qt/3
--enable-application=suite --disable-freetype2 --enable-jprof
--enable-default-toolkit=gtk2 --enable-xft

Reproducible: Always

Comment 1

[rbs]

menclose is not yet implemented.

Comment 2

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 3

[Gervase Markham [:gerv]]

This bug has been automatically resolved after a period of inactivity (see above
comment). If anyone thinks this is incorrect, they should feel free to reopen it.

Comment 4

[Martin Mokrejs]

Still problem with:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051122 SeaMonkey/1.5a

./configure --disable-optimize --enable-debug='-g3 -O0' --enable-debug-modules=all --enable-debugger-info-modules --enable-detect-webshell-leaks --enable-svg --enable-svg-renderer-libart --enable-image-decoders=all --with-qtdir=/usr/qt/3 --enable-application=suite --disable-freetype2 --enable-default-toolkit=gtk2 --enable-xft --disable-gssapi

Comment 5

[Heather L Bailey]

Problem continues with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5.  <menclose> is not rendering properly.

Comment 6

[rbs]

<menclosed> is not yet implemented.

Comment 7

[Martin Mokrejs]

Still an issue with current trunk build.

Comment 8

[Martin Mokrejs]

Still an issue with current cvs HEAD build:

./configure --disable-optimize --enable-debug='-g3 -O0 -ggdb' --enable-debug-modules=all --enable-debugger-info-modules --enable-detect-webshell-leaks --disable-svg --enable-image-decoders=all --with-qtdir=/usr/qt/3 --enable-application=suite --disable-freetype2 --enable-jprof --enable-default-toolkit=gtk2 --enable-xft --disable-gssapi

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a2pre) Gecko/20070126 SeaMonkey/1.5a

Comment 9

[distler]

Of the 14 @notation values listed in the MathML2 Specification[1], half are trivially implemented in CSS[2]. Implementing the others would be some work, but would have the advantage of preventing people from coming up with foolish workarounds[3] for the lack of <menclose> support in Gecko.

[1] http://www.w3.org/TR/2003/REC-MathML2-20031021/chapter3.html#presm.menclose
[2] http://www.w3.org/TR/2007/WD-mathml-for-css-20070427/#menclose
[3] http://golem.ph.utexas.edu/~distler/blog/archives/001264.html

Comment 10

[Frédéric Wang (:fredw)]

Eliot Kimber recently submitted a related bug where he asks for "strike" attributs in menclose :

https://bugzilla.mozilla.org/show_bug.cgi?id=430019

( note that he also sent a mail to the MathML mailing list entitled "Representing Cancelations of Terms in Fractions" :
http://lists.w3.org/Archives/Public/www-math/2008Apr/0195.html )

If someone wants to implement menclose, here is a test page that could help :
http://www.maths-informatique-jeux.com/international/mathml_test/menclose.xml

Comment 11

[Martin Mokrejs]

The display of the original testcase http://www.w3.org/Math/testsuite/testsuite/Presentation/GeneralLayout/menclose/rec-enclose1.xml is badly broken as of current TRUNK.

Comment 12

[Frédéric Wang (:fredw)]

Attachment: parse "notation" attribute + display the radical notation

I wrote an experimental patch to prepare the support of the <menclose/> element. For the moment it parses the "notation" attribute to check the enclosing symbols that need to be displayed. I used the code of the <msqrt/> element, so the "radical" notation is supported.

Comment 13

[Frédéric Wang (:fredw)]

Attachment: testcase

Comment 14

[Frédéric Wang (:fredw)]

Attachment: screenshot of the testcase

left hand side: the page edited in Amaya.
right hand side: the page in Firefox+patch.

Comment 15

[Frédéric Wang (:fredw)]

Attachment: display notations

Here is a new version of my patch that allows to display enclosing symbols for 15 notation values (14 from MathML2 + madruwb). Some of them are currently approximated:
- diagonal strikes are drawn as horizontal strikes.
- circle and roundedbox are drawn as "box".
- madruwb is drawn as "bottom+left"
Also, padding certainly needs to be increased.

Comment 16

[Frédéric Wang (:fredw)]

Attachment: screenshot of my test page

http://www.maths-informatique-jeux.com/international/mathml_test/menclose.xml

Comment 17

[Frédéric Wang (:fredw)]

Attachment: Current version of the patch.

Waiting for more info from the Math WG about how to deal with several notation values...
http://lists.w3.org/Archives/Public/www-math/2008Oct/0002.html

Comment 18

[Frédéric Wang (:fredw)]

Attachment: code clean up + add padding

Comment 19

[Frédéric Wang (:fredw)]

Attachment: Screenshot - MathML testsuite

left hand side: current rendering
right hand side: sample rendering

Comment 20

[Frédéric Wang (:fredw)]

In my local changes, I added support for new notations: longdiv, roundedbox and madruwb. Hence the remaining notations are now:

- circle
- updiagonal & downdiagonal strikes

Does anyone have an idea about how to implement them ?

Comment 21

[Karl Tomlinson (:karlt)]

nsIRenderingContext has DrawLine() and DrawEllipse()

http://mxr.mozilla.org/mozilla-central/source/gfx/public/nsIRenderingContext.h

Or you can get the gfxContext from the nsIRenderingContext::ThebesContext() and have a bit more functionality:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/public/gfxContext.h

Comment 22

[Frédéric Wang (:fredw)]

Attachment: screenshot - MathML testsuite

OK, thanks Karl! I follow your proposal to implement the remaining notations (see screenshot). I've still some improvements to do, but I guess I could propose a patch and ask for a review soon.

Comment 23

[Frédéric Wang (:fredw)]

Attachment: Show the remaining issues

There are 2 minor issues:

1) For madruwb, the character 'ARABIC LETTER LAM INITIAL FORM' does not cover the whole enclosing content (third line of the screenshot). Do I need to add something in the source to make the symbol stretch ?
2) stroke-width of diagonal strikes, roundedbox and ellipse is not the same as the one of the other bars when you zoom-in the page. I know it is possible to use gfxContext to set stroke-width, but I wonder whether there exists something more straightforward.

I also have an additional question (not related to menclose): why does RoundedRectangle approximate the rounded angles with Bézier curves? Wouldn't it be better to use elliptical arcs?
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxContext.cpp#803

Comment 24

[Frédéric Wang (:fredw)]

Attachment: Patch

Comment 25

[Karl Tomlinson (:karlt)]

(In reply to comment #23)
> 1) For madruwb, the character 'ARABIC LETTER LAM INITIAL FORM' does not cover
> the whole enclosing content (third line of the screenshot). Do I need to add
> something in the source to make the symbol stretch ?

This will be difficult.  If you have a font called MyFont with support for
stretchy LAM, then mathfontMyFont.properties is the file that needs to be set up to describe how the stretched character can be produced.

But existing stretched characters either stretch vertically or horizontally but not both.  If a font provides parts of the madruwb as separate characters so that the horizontal and vertical components can be considered separately, then perhaps two nsMathMLChars can be used for the two components.

If you don't have a font with support for stretchy LAM, then simply scaling a character (which nsMathMLChar does not yet do - bug 414277) may be the best option, but estimating the space to allow for the vertical parts of the stroke is going to involve some heuristics.

Have you seen this implemented in other software?  What does it do?

Also, don't you want U+FEDD ARABIC LETTER LAM ISOLATED FORM, or simply U+0644
ARABIC LETTER LAM here?

> 2) stroke-width of diagonal strikes, roundedbox and ellipse is not the same
> as the one of the other bars when you zoom-in the page. I know it is
> possible to use gfxContext to set stroke-width, but I wonder whether there
> exists something more straightforward.

I can't think of anything simpler using nsIRenderingContext.

Also, I think nsIRenderingContext will eventually be replaced by gfxContext,
so it would be better to use gfxContext directly than add to
nsIRenderingContext.

> I also have an additional question (not related to menclose): why does
> RoundedRectangle approximate the rounded angles with Bézier curves?

I don't know.  Maybe a performance optimization.

Comment 26

[Frédéric Wang (:fredw)]

(In reply to comment #25)
> Have you seen this implemented in other software?  What does it do?
> 
> Also, don't you want U+FEDD ARABIC LETTER LAM ISOLATED FORM, or simply U+0644
> ARABIC LETTER LAM here?
> 

	Actually, I don't know much about arabic notations. I read a pdf file in which it is proposed to represent madruwb by underlining the content, putting a "ARABIC LETTER LAM INITIAL FORM" character on its left and stretching this character vertically. That's what I try to do, but apparently stretching is likely to be really complex. What I understood is that the "madruwb" notation was proposed by moroccan researchers in a W3C note. They made a fork called "Dadzilla" where they work on arabic mathematical notations. I think I should questionned them directly.

> I can't think of anything simpler using nsIRenderingContext.

I guess you mean gfxContext context here?

> 
> Also, I think nsIRenderingContext will eventually be replaced by gfxContext,
> so it would be better to use gfxContext directly than add to
> nsIRenderingContext.

OK, I'll try to use gfxContext.

Comment 27

[Frédéric Wang (:fredw)]

Attachment: all mathml 2 notations +approximation of madruwb

Comment 28

[Frédéric Wang (:fredw)]

Attachment: screenshot of the mathml testsuite

Comment 29

[Frédéric Wang (:fredw)]

Attachment: written arabic text with lam letters

Comment 30

[Frédéric Wang (:fredw)]

	Apparently Azzeddine Lazrek & Mustapha Eddahibi don't have examples of an implementation of the madruwb symbol with a curvilinear underline ( http://www.w3.org/TR/arabic-math/#N10AEE ). Eddahibi thinks it should be better to follow TeX directives i.e. drawing the lam symbol with a combination of streched characters. However in my patch, the Lam symbol is currently drawn with curves and lines because it is easier to implement. I submitted screenshot to Azzeddine who asked a calligrapher for advice. Then he sent me some screenshots of text with Lam symbols. Here is a translation of the description he gave: "Lam has a slight curvature, no curvature at the beginning, a slight tail, see attachments". It is not exactly how I draw it.

Currently, I haven't a lot of time to improve the madruwb notation. I think the patch can be commited despite its lack of support for the madruwb notation, because it integrates all the notations of MathML 2 that some people asked for.

Comment 31

[Karl Tomlinson (:karlt)]

(In reply to comment #30)
> Eddahibi thinks it should be better to follow TeX directives
> i.e. drawing the lam symbol with a combination of streched characters.

Using curves to connect a set of partial characters for the horizontal component of the Lam would be difficult.

Similarly, the slope on the vertical component of the Lam makes constructing that from parts difficult.

> However in my patch, the Lam symbol is currently drawn
> with curves and lines because it is easier to implement.

Yes, that seems the sensible solution.

> Currently, I haven't a lot of time to improve the madruwb notation. I think the
> patch can be commited despite its lack of support for the madruwb notation,
> because it integrates all the notations of MathML 2 that some people asked for.

Yes, it's not necessary to add madruwb support at the same time as the other notations.  However, what you have for madruwb looks close (to my untrained eye).

Comment 32

[Karl Tomlinson (:karlt)]

Comment on attachment 349302 [details] [diff] [review]
all mathml 2 notations +approximation of madruwb

I think the precise "right" way to draw madruwb is sufficiently unclear that I
don't want the rest of this patch to be held up by spending time on madruwb,
so I'd like to consider that separately, after the notations that are
listed in MathML2.

(I also wonder whether adding a single Arabic notation is going to be useful,
with Mozilla's current level of support for other Arabic notations?)


This frame type covers drawing of many different notations, but usually only a
small subset are actually used.

I'd like to reduce the amount of data stored on each of these frames.

+  nsRect       mMenclose;
+  nsRect       mRadicalBar;
+  nsRect       mLeftBar;
+  nsRect       mRightBar;
+  nsRect       mTopBar;
+  nsRect       mBottomBar;
+  nsRect       mVerticalStrike;
+  nsRect       mHorizontalStrike;

Can most of these rects be replaced with a single nsRect mEnclosedRect that
encloses the contents, and other specific rects be calculated during
BuildDisplayLists?

+#define NB_CHAR 2
+  nsMathMLChar mChar[NB_CHAR];
+#define mLongdivChar mChar[0]
+#define mRadicalChar mChar[1]

Similarly the nsMathMLChars are usually not used, but they are moderate sized
objects.  I think an nsTArray<nsMathMLChar> that is built up with only the
members that are used could work here.  In order to locate the particular
nsMathMLChars, it will probably be necessary to have mLongDivCharIndex and
mRadicalCharIndex or similar.

+#define NB_NOTATIONS 15
+  PRBool draw_notation[NB_NOTATIONS];
+#define DRAW_LONGDIV            draw_notation[0]
+#define DRAW_ACTUARIAL          draw_notation[1]
+#define DRAW_RADICAL            draw_notation[2]
+#define DRAW_BOX                draw_notation[3]
+#define DRAW_ROUNDEDBOX         draw_notation[4]
+...

MACROs can often hide what is really happening.

A bit map in a single PRUint32 would work well here.  The bit-masks
for each notation could be defined in the same enum as used for
nsDisplayNotation (below).

+  #define NS_FRAME_ELLIPSE 0
+  #define NS_FRAME_ROUNDEDRECTANGLE 1
+  #define NS_FRAME_MADRUWB 2

An enum would be better here.

Adding entries for eNotationUpDiagonalStrike and eNotationDownDiagonalStrike
and unifying nsDisplayMathMLStrike and nsDisplayMathMLFrame into
nsDisplayNotation would reduce the number of lines of code.

And nsDisplayNotation can be moved to nsMathMLmencloseFrame, as it is only
used by that file/class.


<menclose notation=radical/> is equivalent to <msqrt/> and so should share
implementation.  The best way to do this might be to derive
nsMathMLmsqrtFrame from nsMathMLmencloseFrame.


I think "circle" (ellipse) needs to be large enough to completely enclose the
contents.  i.e. I think the sample rendering here is wrong:

http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/menclose/menclose1-full.xhtml

Consider a matrix (fenced mtable) in a circle.

(A circle shouldn't require any padding though.)


In nsMathMLmencloseFrame.cpp and nsMathMLmencloseFrame.h, I don't think all
those Contributors should be listed.  Only list contributors who have written
code that you have copied.

+  virtual PRIntn GetSkipSides() const { return 0; }

This need not be provided, because the base class nsMathMLContainerFrame
provides an equivalent definition.

+  void CheckNotation(const nsAString& aNotation);

AddNotation would be a clearer name, I think.

+  static const nsString notationsVals[] =
+    { NS_LITERAL_STRING("longdiv"),
+      NS_LITERAL_STRING("actuarial"),
+      NS_LITERAL_STRING("radical"),
+      NS_LITERAL_STRING("box"),
+      NS_LITERAL_STRING("roundedbox"),
+      NS_LITERAL_STRING("circle"),
+      NS_LITERAL_STRING("left"),
+      NS_LITERAL_STRING("right"),
+      NS_LITERAL_STRING("top"),
+      NS_LITERAL_STRING("bottom"),
+      NS_LITERAL_STRING("updiagonalstrike"),
+      NS_LITERAL_STRING("downdiagonalstrike"),
+      NS_LITERAL_STRING("verticalstrike"),
+      NS_LITERAL_STRING("horizontalstrike"),
+      NS_LITERAL_STRING("madruwb")
+    };

This is effectively storing each string twice, once for the NS_LITERAL_STRINGs
on the right hand side and another (when notationsVals is initialized) in the
nsString array on the left hand side.  Also, I think static nsStrings will
confuse Mozilla's memory leak testing tools.

I considered suggesting NS_MULTILINE_LITERAL_STRING(l) with l = NS_LL("..")
here but these are not used much and the logic is not very intuitive.

I think the easiest thing would be to just use nsAString::EqualsASCII() or
EqualsLiteral().  The strings could be in a const char* notations[].  However,
instead having a set of conditionals with EqualsLiteral() would make the
mapping from the string to the bit mask clearer, and would enable the
actuarial and box and nsMathMLChar logic in nsMathMLmencloseFrame::Init() to
be moved into CheckNotation().


+    // longdiv:
+    PRUnichar(')'),

Can you add this comment please?:
"Unicode 5.1 assigns U+27CC to LONG DIVISION, but a right parenthesis renders
better with current font support."

+  mMenclose(),
+  mRadicalBar(),
+  mLeftBar(),
+  mRightBar(),
+  mTopBar(),
+  mBottomBar(),
+  mVerticalStrike(),
+  mHorizontalStrike()

It's not necessary to explicitly initialize fields that have explicitly
defined constructors.

+  // No need to tract the style context given to our MathML chars. 

tract -> track


+      gfxCtx->RoundedRectangle( gfxRect( rect.x/u,
+                                         rect.y/u,
+                                         rect.width/u,
+                                         rect.height/u),

nsPresContext* presContext = mFrame->PresContext()

gfxCtx->RoundedRectangle(presContext->AppUnitsToGfxUnits(rect),

+  gfxCtx->SetLineWidth(mThickness / GetAppUnitsPerDevPixel(aCtx));

gfxCtx->SetLineWidth(presContext->AppUnitsPerDevPixel(mThickness));

There is also presContext->AppUnitsPerDevPixel() if you need it, so
GetAppUnitsPerDevPixel should not be needed.  (It should be safe to assume
that AppUnitsPerDevPixel is non-zero.  We'll have plenty of other problems if
that happens.)

+  aCtx->SetColor(mFrame->GetStyleColor()->mColor);

+    case NS_FRAME_ELLIPSE:
+      gfxCtx->SetLineWidth(e);
+      aCtx->DrawEllipse(rect);

+  gfxCtx->SetLineWidth(mThickness / GetAppUnitsPerDevPixel(aCtx));
+
+  if(mDownDiagonal)
+    aCtx->DrawLine(aRect.x, aRect.y, aRect.XMost(), aRect.YMost());
+  else
+    aCtx->DrawLine(aRect.x, aRect.YMost(), aRect.XMost(), aRect.y);

I see that using nsIRenderingContext methods saves some lines of code by
performing the NewPath() and Stroke(), but it is best not to mix
nsIRenderingContext and gfxContext use, as that would be making assumptions
about their interaction.  nsIRenderingContext::DrawLine assumes that the line
width is always 1:

http://hg.mozilla.org/mozilla-central/annotate/7c17efd1d883/gfx/src/thebes/nsThebesRenderingContext.cpp#l434

Can a single SetLineWidth() be used for all mTypes?

+    default:
+      gfxCtx->SetLineWidth(e);
+      aCtx->DrawRect(rect);
+      break;

This is not expected to be executed IIUC.
If so, an NS_NOTREACHED would be better here.


       aTag == nsGkAtoms::mo_ ||
+      aTag == nsGkAtoms::menclose_ ||
       aTag == nsGkAtoms::mfrac_ ||

   else if (aTag == nsGkAtoms::mrow_ ||
            aTag == nsGkAtoms::merror_)
     newFrame = NS_NewMathMLmrowFrame(mPresShell, aStyleContext);
+  else if (aTag == nsGkAtoms::menclose_)
+    newFrame = NS_NewMathMLmencloseFrame(mPresShell, aStyleContext);
   else if (aTag == nsGkAtoms::math) { 
     // root <math> element

I can't see an obvious order to these sequences of tags, but the two sequences
are reasonably consistent in order, and I would like them to remain as
consistent as practical.

Comment 33

[Frédéric Wang (:fredw)]

(In reply to comment #32)
>I think the precise "right" way to draw madruwb is sufficiently unclear that I
>don't want the rest of this patch to be held up by spending time on madruwb,
>so I'd like to consider that separately, after the notations that are
>listed in MathML2.
>(I also wonder whether adding a single Arabic notation is going to be useful,
>with Mozilla's current level of support for other Arabic notations?)

	I agree with your suggestion. Actually, even the authors of the W3C note don't seem to know exactly how to deal with the case of madruwb. Moreover I understood that they are several way to write arabic mathematical notation and that madruwb is supposed to be used in a context where maths are written from right to left, thing that Mozilla does not support. So let's deal with it separately.
	By the way, I will be too busy in the next weeks in order to modify my patch. I hope I will be able to do it before Firefox 3.1 is released though.
	
> I think "circle" (ellipse) needs to be large enough to completely enclose the
> contents.  i.e. I think the sample rendering here is wrong:
> http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/menclose/menclose1-full.xhtml
> Consider a matrix (fenced mtable) in a circle.
> (A circle shouldn't require any padding though.)

	I can increase the size of the ellipse but be aware that if we want to enclose the whole content, then its size needs to be at least sqrt(2) greater than the box of the content (see attachement below). This means that in the example "nested circle" the outer circle will be twice greater than the bounding box of "abcdefg". This can appear a bit too much and that's why I tried to have a result similar to the one of the sample rendering.
	
>In nsMathMLmencloseFrame.cpp and nsMathMLmencloseFrame.h, I don't think all
>those Contributors should be listed.  Only list contributors who have written
>code that you have copied.

	I used nsMathMLmsqrt* files and didn't remove the names of contributors. I don't know who made what.
	
>I see that using nsIRenderingContext methods saves some lines of code by
>performing the NewPath() and Stroke(), but it is best not to mix
>nsIRenderingContext and gfxContext use, as that would be making assumptions
>about their interaction

	Yes, I wrote this before you indicated me that I can use gfxContext... I'll will rewrite this.

Comment 34

[Frédéric Wang (:fredw)]

Attachment: smaller size for the "circle notation"

oops, I forgot to send this.

Comment 35

[Karl Tomlinson (:karlt)]

(In reply to comment #33)
>     By the way, I will be too busy in the next weeks in order to modify my
> patch. I hope I will be able to do it before Firefox 3.1 is released though.

Firefox 3.1 has mostly reached feature freeze, sorry, so drivers are unlikely
to accept this for 3.1.  (Focus is currently on getting bugs ironed out in
existing features so that they can be made available to users.)  But
development has already started on the next release which won't be too far
away, so please don't let this stop you.
 
> > I think "circle" (ellipse) needs to be large enough to completely enclose the
> > contents.  i.e. I think the sample rendering here is wrong:
> > http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/menclose/menclose1-full.xhtml
> > Consider a matrix (fenced mtable) in a circle.
> > (A circle shouldn't require any padding though.)
> 
>     I can increase the size of the ellipse but be aware that if we want to
> enclose the whole content, then its size needs to be at least sqrt(2) greater
> than the box of the content (see attachement below). This means that in the
> example "nested circle" the outer circle will be twice greater than the
> bounding box of "abcdefg". This can appear a bit too much and that's why I
> tried to have a result similar to the one of the sample rendering.

Yes, the issues here stem from the fact that we only have a rectangle
describing the content, but I expect erring on the side of too large will
avoid the worst situations.

Removing the padding will help a little here, and perhaps the size can be
reduced by (half?) the line-thickness too, but watch out for
<menclose notation="circle">&cdot;</menclose>.  A minimum size might be the
best for this.

How often do you expect nested circles to be used?

If this is a common use case, then perhaps it can be detected and treated
specially, but I think that would be best left for a follow-up patch.

Comment 36

[Frédéric Wang (:fredw)]

(In reply to comment #35)
> Yes, the issues here stem from the fact that we only have a rectangle
> describing the content, but I expect erring on the side of too large will
> avoid the worst situations.
> 
> Removing the padding will help a little here, and perhaps the size can be
> reduced by (half?) the line-thickness too, but watch out for
> <menclose notation="circle">&cdot;</menclose>.  A minimum size might be the
> best for this.
> 
> How often do you expect nested circles to be used?
> 
> If this is a common use case, then perhaps it can be detected and treated
> specially, but I think that would be best left for a follow-up patch.

	Actually I don't really see when nested circles are likely to be used, I just took the example of the testsuite to illustrate that the size of nested circles increases faster. For nested boxes, we simply add a padding to the size (arithmetic growth) while in the is case of circle we need to scale the box by a factor k (geometric growth). I'll remove the padding and reduce the line-thickness but this will not change a lot how fast the size of nested circles grows. However, we can imagine that one will not use a lot of circles in the same sequence of nested menclose. So I think we can take k = sqrt(2) (or maybe reduce "by half", that I understand as taking the geometric mean between 1 and sqrt(2), i.e. k = 2^(1/4)).

Comment 37

[Frédéric Wang (:fredw)]

Attachment: fix a compiling error + other trivial fixes

Comment 38

[Frédéric Wang (:fredw)]

Attachment: several fixes

oops, I sent the wrong patch.

Comment 39

[Frédéric Wang (:fredw)]

Attachment: all mathml 2 notations

This patch addresses almost all the issues, except sharing of implementation between menclose/msqrt and the dynamic allocation of Rects and nsMathMLChars. I tried to use nsTArray<nsMathMLChar> as you suggested, but when I do this the characters no longer stretch and I don't understand why.

Comment 40

[Karl Tomlinson (:karlt)]

Attachment: nsTArray<nsMathMLChar> changes

(In reply to comment #39)
> I tried to use nsTArray<nsMathMLChar> as you suggested, but when
> I do this the characters no longer stretch and I don't understand why.

This patch applies on top of attachment 354331 [details] [diff] [review].
radical and longdiv stretch around an mfrac for example (but I'm not sure I did anything differently from you to make this work).

Comment 41

[Frédéric Wang (:fredw)]

Attachment: Testcase: circle notation

Comment 42

[Frédéric Wang (:fredw)]

Attachment: Merge the 2 previous patches + make msqrt inherit from menclose

OK, thanks it works now. Here is a new patch that makes msqrt inherit from menclose.

The remaining issues are:
- allocation of rects
- add a minimal size for the circle notation

Comment 43

[Karl Tomlinson (:karlt)]

(In reply to comment #42)
> - allocation of rects

I wasn't actually suggesting an nsTArray<nsRect> on the frame (though maybe that's an option).  Can creation of the specific individual rects be delayed until BuildDisplayLists so they don't need to be stored on the frame?

Comment 44

[Frédéric Wang (:fredw)]

Attachment: Improve circle notation + dynamic allocation of rects

> Can creation of the specific individual rects be delayed
> until BuildDisplayLists so they don't need to be stored on the frame?

I'm not sure to understand what you mean here. Rects are not only used in BuildDisplayLists but also in FixInterFrameSpacing, so I think they must be stored in nsMathMLmencloseFrame.

In this last patch I use nsTArray<nsRect> so that only the rects needed are created. In addition, I improve the "circle" notation (see attachment 356243 [details]).

Comment 45

[Frédéric Wang (:fredw)]

Attachment: Updated patch

Previous patch was broken due to changes in CSSFrameConstructor.cpp

Comment 46

[Frédéric Wang (:fredw)]

Attachment: Updated patch

Comment 47

[Karl Tomlinson (:karlt)]

Comment on attachment 358010 [details] [diff] [review]
Updated patch

Thanks for making the requested changes, Frédéric.  The overall structure is
looking good.  In the www-math thread, we're still not describing precisely
the same interpretation of the spec for the position and size of notations
when there are more than one, but I'll follow-up there

http://lists.w3.org/Archives/Public/www-math/2009Jan/0015.html

+  void allocateMathMLChar(PRUint32 mask);

+  void allocateMathMLRect(PRUint32 mask);

+  PRBool isToDraw(PRUint32 mask);

Member functions in Mozilla should normally start with a capital letter
(AllocateMathMLChar, IsToDraw).

+  PRUint32 notationsToDraw;

+  nscoord      ruleThickness;

Member variables should start with "m" (mNotationsToDraw).

+  if(mask == NOTATION_LONGDIV)
+    {

Mozilla style is a space after the "if" and the block's opening brace on the
same line as the condition.  See section "Control Structures" at
https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

+static const PRUnichar kRadicalChar = PRUnichar(0x221A);

The PRUnichar() cast shouldn't be necessary.

+  (mMathMLChar[i]).SetData(presContext, Char);

The first set of parentheses is not necessary.

+  UpdatePresentationDataFromChildAt(0, -1,
+                                    NS_MATHML_COMPRESSED,
+                                    NS_MATHML_COMPRESSED);

-  // 1. The REC says:
-  //    The <msqrt> element leaves both attributes [displaystyle and scriptlevel]
-  //    unchanged within all its arguments.
-  // 2. The TeXBook (Ch 17. p.141) says that \sqrt is cramped 

I'd prefer to retain at least comment 2 in
nsMathMLmencloseFrame::TransmitAutomaticData so as to give a partial
explanation for the NS_MATHML_COMPRESSED arguments.
(I can't see comment 1 in the MATHML 3 spec so I'm happy for that to be left
out.)

+  if (!NS_MATHML_HAS_ERROR(mPresentationData.flags))
+    {
+      if(isToDraw(NOTATION_RADICAL))

An early return statement when NS_MATHML_HAS_ERROR would save having to indent
the rest of the function.

+  if(isToDraw(NOTATION_RADICAL) || isToDraw(NOTATION_LONGDIV))
+    {

+    }
+  else if(isToDraw(NOTATION_CIRCLE))
+    {

+    }
+  else
+    {

Each of these blocks adjusts aDesiredSize, but they each only consider a
subset of the notations and only one of these blocks will be executed.

What we need is to consider all notations that will be drawn and set
aDesiredSize to the maximum dimensions.

+      // Increase height
+      if(bmBase.ascent * ratio < bmBase.ascent + psi ||
+         bmBase.descent * ratio < bmBase.descent + psi)

Why do both the ascent and descent need to be greater than the threshold?
Isn't it only the height (ascent + descent) that is important?

+        {
+          mBoundingMetrics.ascent = bmBase.ascent + psi;
+          mBoundingMetrics.descent = bmBase.descent + psi;
+        }
+      else
+        {
+          mBoundingMetrics.ascent = bmBase.ascent * ratio;
+          mBoundingMetrics.descent = bmBase.descent * ratio;

This else block looks like it will skew the position of an ellipse according
to whether ascent or descent is larger.  The centre of the ellipse should be
midway between the top and bottom of the child content.

+        }
+      contentDy = mBoundingMetrics.ascent;
+      aDesiredSize.height = mBoundingMetrics.ascent + mBoundingMetrics.descent;
+      mBoundingMetrics.ascent = aDesiredSize.height/2;
+      mBoundingMetrics.descent = aDesiredSize.height/2;
+      aDesiredSize.ascent = mBoundingMetrics.ascent;

This looks like it is moving the baseline to the centre of the ellipse.  The
baseline of the menclose element should be the same as the baseline of the
child content.

+          mBoundingMetrics.width = bmBase.width * ratio;
+          mBoundingMetrics.leftBearing = bmBase.leftBearing * ratio;
+          mBoundingMetrics.rightBearing = bmBase.rightBearing * ratio;
+          contentDx = (ratio - 1)*bmBase.width/2;

+    mMenclose.SetRect(0, 0,
+                      aDesiredSize.width, aDesiredSize.height);
+  

There is an inconsistency here.  The height of the ellipse depends on the ink
extents of the child content while the width depends on the logical (not ink)
width.  Also multiplying both leftBearing and rightBearing by ratio is not
what you want.  Think about the extents of the ellipse centred on the child content.

+  else
+    {
+      psi = 3*ruleThickness;
+      nscoord delta = psi % onePixel;
+      if (delta)
+        psi += onePixel - delta; // round up
+
+      if (ruleThickness < onePixel)ruleThickness = onePixel;
+
+      // add a padding
+      mBoundingMetrics.ascent = bmBase.ascent + psi;
+      mBoundingMetrics.descent = bmBase.descent + psi;
+      mBoundingMetrics.width = bmBase.width + 2*psi;

If there is only a "top" notation, then space should not be added to the sides
nor the bottom.

+      mBoundingMetrics.leftBearing = bmBase.leftBearing;
+      mBoundingMetrics.rightBearing = bmBase.rightBearing;

The left and right bearing should include the ink of the notations.

+      contentDy = mBoundingMetrics.ascent;

+      contentDy = mBoundingMetrics.ascent;

+      contentDy = aDesiredSize.ascent;

+      PositionRowChildFrames(contentDx, contentDy);

I can't think of a good reason why contentDy should ever not be the position
of the baseline (ascent), so this variable should not be needed.

+/* virtual */ nscoord
+nsMathMLmencloseFrame::GetIntrinsicWidth(nsIRenderingContext* aRenderingContext)
+{
+  // The child frames form an mrow
+  nscoord width = nsMathMLContainerFrame::GetIntrinsicWidth(aRenderingContext);
+
+  if(isToDraw(NOTATION_RADICAL))
+    {
+      // Add the width of the radical symbol
+      width += mMathMLChar[mRadicalCharIndex].GetMaxWidth(PresContext(),
+                                                          *aRenderingContext);
+    }
+
+  if(isToDraw(NOTATION_LONGDIV))
+    {
+      // Add the width of the longdiv symbol
+      width += mMathMLChar[mLongDivCharIndex].GetMaxWidth(PresContext(),
+                                                          *aRenderingContext);
+    }
+
+  return width;
+}
+

The width needs to depend on other notations also.  This is needed when the
menclose is within a mtable.  There is a bit of computation involved in
finding the maximum width, which would be similar to what Place is measuring,
so it would be good to share this code.

The main reason why we can't currently use nsMathMLmencloseFrame::Place for
measuring the width through nsMathMLContainerFrame::GetIntrinsicWidth is that
Place currently uses nsMathMLChar::Stretch, whereas for GetIntrinsicWidth we
want to use nsMathMLChar::GetMaxWidth (because the height is not known).

I think this can be handled by moving most of the Place measuring (but not the
placement) to a method called Measure, which takes a parameter aWidthOnly,
which will be TRUE when called from GetIntrinsicWidth.  The Measure function
would then use Stretch or GetMaxWidth according to aWidthOnly.

nsMathMLContainerFrame::GetIntrinsicWidth could then be used for
nsMathMLmencloseFrame and could call the Measure virtual method instead of
MeasureChildFrames, and nsMathMLContainerFrame::MeasureChildFrames could be
renamed to Measure (with the extra argument).  (I'm wasn't fond of the
situation that we currently have with MeasureChildFrames anyway.)

(This would all be simpler if there were a nsMathMLContainerFrame for the
implied child mrow around child frames, but I suspect adding an anonymous
nsMathMLContainerFrame would be more complex.)

(In reply to comment #44)
> > Can creation of the specific individual rects be delayed
> > until BuildDisplayLists so they don't need to be stored on the frame?
> 
> I'm not sure to understand what you mean here. Rects are not only used in
> BuildDisplayLists but also in FixInterFrameSpacing, so I think they must be
> stored in nsMathMLmencloseFrame.

+nscoord
+nsMathMLmencloseFrame::FixInterFrameSpacing(nsHTMLReflowMetrics& aDesiredSize)
+{
+  nscoord gap = nsMathMLContainerFrame::FixInterFrameSpacing(aDesiredSize);
+  if (!gap) return 0;
+
+  nsRect rect;
+
+  if(isToDraw(NOTATION_ROUNDEDBOX) ||
+     isToDraw(NOTATION_CIRCLE) ||
+     isToDraw(NOTATION_UPDIAGONALSTRIKE) ||
+     isToDraw(NOTATION_DOWNDIAGONALSTRIKE))
+    mMenclose.MoveBy(gap, 0);

I think that the only rectangle that needs to be stored on the frame object is
a rectangle bounding the child content.  All of the other rectangles only need
to be moved in FixInterFrameSpacing because they are calculated in Place.  If
they are instead calculated in BuildDisplayList, then they will not need to be
moved.  (The position of the radical bar can be determined from
mMathMLChar[mRadicalCharIndex] rect).

+
+  if(isToDraw(NOTATION_LONGDIV))
+    {
+      mMathMLChar[mLongDivCharIndex].GetRect(rect);
+      rect.MoveBy(gap, 0);
+      mMathMLChar[mLongDivCharIndex].SetRect(rect);
+    }
+
+  if(isToDraw(NOTATION_RADICAL))
+    {
+      mMathMLChar[mRadicalCharIndex].GetRect(rect);
+      rect.MoveBy(gap, 0);
+      mMathMLChar[mRadicalCharIndex].SetRect(rect);

I would do this as a loop over each mMathMLChar[i].

+class nsDisplayMathMLFrame : public nsDisplayItem

+  nsresult DisplayFrame(nsDisplayListBuilder* aBuilder,
+                        nsIFrame* aFrame, const nsRect& aRect,
+                        const nsDisplayListSet& aLists,
+                        nscoord aThickness, PRUint32 aType);

I'd prefer to call this nsDisplayNotation and DisplayNotation or similar,
because "frame" tends to mean something quite different in Mozilla.

+enum {
+  NS_FRAME_DOWNDIAGONALSTRIKE,
+  NS_FRAME_ELLIPSE,
+  NS_FRAME_ROUNDEDRECTANGLE,
+  NS_FRAME_UPDIAGONALSTRIKE
+};

I don't think this needs to be separate from the
nsMathMLmencloseFrame::NOTATION_* enum.  The identifiers from that enum can
also be used in DisplayFrame/DisplayNotation.

Comment 48

[Karl Tomlinson (:karlt)]

(In reply to comment #47)
> In the www-math thread, we're still not describing precisely
> the same interpretation of the spec for the position and size of notations
> when there are more than one, but I'll follow-up there

I think I'm starting to see the merits of modifying individual notations so that they look good with the other notations in the same menclose.  I'll get my thoughts together and followup to www-math.

Comment 49

[Karl Tomlinson (:karlt)]

(In reply to comment #47)
> If there is only a "top" notation, then space should not be added to the sides
> nor the bottom.

Actually, adding space to the sides may be fine if you think it looks better to have an overhang.  It would be nice to have a minimum length for the "top" so that it doesn't ever look like a dot, and always having an overhang is one way of enforcing a minimum.

Comment 50

[Frédéric Wang (:fredw)]

Attachment: Circle notation: alignment of the center of the ellipse with the baseline

Comment 51

[Frédéric Wang (:fredw)]

Attachment: circle notation alignement and other fixes

Thanks for the review. Here is a new patch that fixes trivial issues and prevents the circle notation to change the baseline. Now I have to move the determination of the maximum size to an auxiliary function and to improve GetIntrinsicWidth & FixInterFrameSpacing.

> What we need is to consider all notations that will be drawn and set
> aDesiredSize to the maximum dimensions.

	You're right. If I want to be consistent with the algorithm I proposed, I should not mix the setting of aDesiredSize and computation of the maximum dimensions. Anyway when I remove this computation from Place the two steps will be really separated.

>Why do both the ascent and descent need to be greater than the threshold?
>Isn't it only the height (ascent + descent) that is important?

>This else block looks like it will skew the position of an ellipse according
>to whether ascent or descent is larger.  The centre of the ellipse should be
>midway between the top and bottom of the child content.

>This looks like it is moving the baseline to the centre of the ellipse.  The
>baseline of the menclose element should be the same as the baseline of the
>child content.

>There is an inconsistency here.  The height of the ellipse depends on the ink
>extents of the child content while the width depends on the logical (not ink)
>width.  Also multiplying both leftBearing and rightBearing by ratio is not
>what you want.  Think about the extents of the ellipse centred on the child
>content.

>I can't think of a good reason why contentDy should ever not be the position
>of the baseline (ascent), so this variable should not be needed.

	I don't know why I chosed to move the baseline rather than the ellipse but the last patch should fix all these problems. However, I think another issue can appear when the other notations increase the size: the center moves and the ellipse may overlap with its content (under the assumption that I draw the ellipse inscribed exactly in the new bounding box).

> If there is only a "top" notation, then space should not be added to the sides
> nor the bottom.

> Actually, adding space to the sides may be fine if you think it looks better to
> have an overhang.  It would be nice to have a minimum length for the "top" so
> that it doesn't ever look like a dot, and always having an overhang is one way
> of enforcing a minimum.

	OK, I now add a space only when it's necessary (especially so they are not needed in the case of strikes).

Comment 52

[Frédéric Wang (:fredw)]

Among the rects stored in the frame, I removed all but mMenclose. Now, I wonder whether it would be more straightforward to use mMenclose.width for GetIntrinsicWidth() rather than introducing the function Measure. Why do you think about this?

(By the way, I changed the rendering of notation=circle again because it was incorrect in the last patch)

Comment 53

[Karl Tomlinson (:karlt)]

(In reply to comment #52)
> Among the rects stored in the frame, I removed all but mMenclose. Now, I wonder
> whether it would be more straightforward to use mMenclose.width for
> GetIntrinsicWidth() rather than introducing the function Measure. Why do you
> think about this?

I'm not sure that I understand your proposal fully, but I don't think this deals with the nsMathMLChar::Stretch/GetMaxWidth issue.

GetIntrinsicWidth() will be called before Place, when the height of child content (and accurate widths) is not yet known, so it is not possible to set mMenclose accurately.  (It doesn't really matter if it does get set, because it will be overwritten with better values, but positions only need to be set/recorded when Place is called with aPlaceOrigin TRUE.)

Also, think about whether mMenclose is even needed.  I was suggesting keeping one rectangle when I was imagining positioning notations based on child content.
If you are positioning notations based on the boundary of the entire menclose element, and aDesiredSize gets set to the rectangle that you need, then this rectangle will be stored for you and available through nsIFrame::GetRect().

Comment 54

[Frédéric Wang (:fredw)]

> I'm not sure that I understand your proposal fully, but I don't think this
> deals with the nsMathMLChar::Stretch/GetMaxWidth issue.
> 
> GetIntrinsicWidth() will be called before Place, when the height of child
> content (and accurate widths) is not yet known, so it is not possible to set
> mMenclose accurately.  (It doesn't really matter if it does get set, because it
> will be overwritten with better values, but positions only need to be
> set/recorded when Place is called with aPlaceOrigin TRUE.)

OK, I was not sure whether "Place" was called before "GetIntrinsicWidth" or not. So I believe I will use a shared "Measure" method as you suggested.

> Also, think about whether mMenclose is even needed.  I was suggesting keeping
> one rectangle when I was imagining positioning notations based on child
> content.
> If you are positioning notations based on the boundary of the entire menclose
> element, and aDesiredSize gets set to the rectangle that you need, then this
> rectangle will be stored for you and available through nsIFrame::GetRect().

In this case I think mMenclose is no longer needed so I'll remove it.

Comment 55

[Frédéric Wang (:fredw)]

Attachment: Add a shared Measure function

Hi Karl,

	Today I have worked on the patch again. I've implemented the shared function Measure. I don't know how to test the behavior of GetIntrinsicWith, but I hope it is correct now.
	The code to get the maximum size is closer to the algorithm I proposed on the MathML mailing list. I ignore leftBearing/rightBearing when computing the maximum size and set them to 0 and mBoundingMetrics.width respectively. This should be the only difference to the original <msqrt/> code. It does not seem to affect the rendering.
	Finally, this new version of the patch improves rendering of "circle" notation and no longer uses Rects stored in the frame.

Comment 56

[Frédéric Wang (:fredw)]

Attachment: Fix conflicts with current trunk.

Comment 57

[Karl Tomlinson (:karlt)]

Comment on attachment 366651 [details] [diff] [review]
Fix conflicts with current trunk.

Sorry, for the delay on this.  I was thinking over a couple of issues.

(In reply to comment #47)
> I think this can be handled by moving most of the Place measuring (but not
> the placement) to a method called Measure, which takes a parameter
> aWidthOnly, which will be TRUE when called from GetIntrinsicWidth.

What I was trying to achieve here was to do all the measuring in one function.
But I overlooked the need to return contentDx for placement, sorry.  I now
think it will be tidier to perform measuring and placement in the same
function.  That function could be a non-virtual:

 PlaceInternal(nsIRenderingContext& aRenderingContext,
               PRBool               aPlaceOrigin,
               nsHTMLReflowMetrics& aDesiredSize,
               PRBool aWidthOnly)

Currently for mrow we have:

nsMathMLContainerFrame::GetIntrinsicWidth
 -> nsMathMLContainerFrame::MeasureChildFrames
     -> nsMathMLContainerFrame::Place

and for msqrt:

nsMathMLmsqrtFrame::GetIntrinsicWidth
 -> nsMathMLContainerFrame::GetIntrinsicWidth (for measuring child frames only)
     -> nsMathMLmsqrtFrame::MeasureChildFrames
         -> nsMathMLContainerFrame::Place

These could change, so that for mrow:

nsMathMLContainerFrame::GetIntrinsicWidth
 -> nsMathMLContainerFrame::MeasureForWidth
     -> nsMathMLContainerFrame::Place

and for menclose:

(no nsMathMLmencloseFrame::GetIntrinsicWidth)
nsMathMLContainerFrame::GetIntrinsicWidth
 -> nsMathMLmencloseFrame::MeasureForWidth (measure whole frame)
     -> nsMathMLmencloseFrame::PlaceInternal
         -> nsMathMLContainerFrame::Place (for measuring child frames only)

and also for menclose:

nsMathMLmencloseFrame::Place
 -> nsMathMLmencloseFrame::PlaceInternal
     -> nsMathMLContainerFrame::Place (for measuring child frames only)

(In reply to comment #55)
> I don't know how to test the behavior of GetIntrinsicWidth,
> but I hope it is correct now.

Test with something like:

  <mrow>
    <mo>|</mo>
    <mtable columnalign="left">
      <mtr>
	<mtd>
	  <mrow>
	  <mo stretchy="false">|</mo>
	  <menclose notation="circle">
	    <mi>abcdefg</mi>
	  </menclose>
	  <mo stretchy="false">|</mo>
	  </mrow>
	</mtd>
      </mtr>
    </mtable>
    <mo>|</mo>
  </mrow>

The distance between the two "|"s on each side gives an indication of the
difference between the widths returned from Place and GetIntrinsicWidth.  If
there are stretchy characters involved, then there will be extra space between
the two "|"s on the right, from GetIntrinsicWidth (to accommodate larger
versions of the stretchy characters), but other notations should have
consistent spacing between the "|"s.

It looks like space for notations is added twice with this patch as it is now.

>     The code to get the maximum size is closer to the algorithm I proposed on
> the MathML mailing list. I ignore leftBearing/rightBearing when computing the
> maximum size and set them to 0 and mBoundingMetrics.width respectively. This
> should be the only difference to the original <msqrt/> code. It does not seem
> to affect the rendering.

The nsBoundingMetrics ascent, descent, leftBearing, and rightBearing (should)
represent tight ink bounds.  That is the extents of the glyphs that are drawn.

nsHTMLReflowMetrics and nsBoundingMetrics::width represent logical bounds
derived from global font metrics and character advances.  These are an
indication of how much space should be occupied.  But note that
nsHTMLReflowMetrics::ascent/height are based on global font metrics, and so
tend to be poor metrics for positioning.

When positioning adjacent items, sometimes the MathML code uses the ink
bounds, sometimes the logical bounds, and sometimes a function of both.  When
horizontal distances only are involved, often the maximum of the two type of
metrics works well.

The metrics returned for menclose (including the notations) should keep these
distinct meanings.  None of these sets of metrics should get smaller when
notations are added, though both are often likely to get larger.  (Currently
it looks like this patch ignores ascent/height of baseSize.)

Currently this patch uses ink extents for vertical distances and logical
extents for horizontal distances.  Consider, for example:

	  <menclose notation="updiagonalstrike">
	    <mo>&minus;</mo>
	  </menclose>

Because logical extents are used for the width, the stroke extends beyond the
minus horizontally.  But ink extents are used for the height, so the stroke is
almost horizontal.  I think ink extents are probably best used in 
the vertical sizing calculations (especially because logical vertical metrics
are often poor), but either a minimum size or some overlap will be needed.

Consider also &centerdot;.  With ink extents, a box or circle will be close to
the dot, which I /think/ would be preferred.  With logical extents the
notation would enclose a character cell.  Currently the box is close
vertically but wide horizontally.  I think similar metrics should usually be
used in each direction if the situation is similar.  e.g. all sides of a box
should use similar metrics.

However different notations may use different metrics.  msqrt even uses ink
metrics, with a minimum size of a "1" (which is really a logical metric) for
the position of the bar, but uses logical metrics for horizontal distances.

What do you think is best for strikes?  I guess ink extents would work if used
with either be a minimum length or some overlap beyond the extents of the
glyph.  Or should the strike extend across the whole character cell, which I
think is normal for CSS line-through/overline/underline text-decorations?

If the strike should extend across the whole character cell, then the maximum
of ink metrics and an em ascent/height or the size of a 1 (similar to msqrt)
could be a suitable vertical metric, given the problems with the logical vertical
metrics.

>     Finally, this new version of the patch improves rendering of "circle"
> notation and no longer uses Rects stored in the frame.

Great, thanks.

+  PRUint8 mLongDivCharIndex , mRadicalCharIndex;

+  // Is the char already allocated?
+  if (mask & mNotationsToDraw)
+    return;
+

Initializing mLongDiv/mRadicalCharIndex in the constructor and testing those
variables instead of mNotationsToDraw would make AllocateMathMLChar more
self-contained rather than needing to rely on mNotationsToDraw being set
elsewhere.

+                         &(mMathMLChar[i]),

The parentheses are unnecessary here.

+  else if (aNotation.EqualsLiteral("circle"))
+    mNotationsToDraw |= NOTATION_CIRCLE;
+  else if (aNotation.EqualsLiteral("left")) {
+    mNotationsToDraw |= NOTATION_LEFT;
+  }

Some of these conditional substatements have braces and some do not.  If any
substatement has braces, I find it least confusing to have every alternative
substatement also use braces.

+  else
+    {

Put the opening brace beside the "else".

+  // The TeXBook (Ch 17. p.141) says that \sqrt is cramped 
+  UpdatePresentationDataFromChildAt(0, -1,
+                                    NS_MATHML_COMPRESSED,
+                                    NS_MATHML_COMPRESSED);

I'm not really clear what setting NS_MATHML_COMPRESSED does, but it feels
weird changing the rendering because there is a notation.  (Strikes, for
example, shouldn't change how things are rendered.)  Let's do this only when
mNotationsToDraw == NOTATION_RADICAL.

+    mMathMLChar[mRadicalCharIndex].GetRect(rect);
+    nscoord dx = rect.width;
+    nscoord dy = nscoord(0.2f * mEmHeight);
+    rect.SetRect(dx, dy,
+                 mencloseRect.width - dx, mRuleThickness);

Use dy = rect.y so that mEmHeight is not needed.

+  if (IsToDraw(NOTATION_VERTICALSTRIKE)) {
+    rect.SetRect(0, mencloseRect.height/2,
+                 mencloseRect.width, mRuleThickness);
+
+    rv = DisplayBar(aBuilder, this, rect, aLists);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  if (IsToDraw(NOTATION_HORIZONTALSTRIKE)) {
+    rect.SetRect(mencloseRect.width/2, 0,
+                 mRuleThickness, mencloseRect.height);
+    
+    rv = DisplayBar(aBuilder, this, rect, aLists);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }

Should vertical/horizontal be swapped here?

Also, consider mRuleThickness/2 when positioning to center the bar.

+  nscoord psi = 3*mRuleThickness;

Mozilla style is to have spaces around binary operators.

+  if (mRuleThickness < onePixel)mRuleThickness = onePixel;

Add a space after ")" too.

+  if(IsToDraw(NOTATION_ROUNDEDBOX) ||
+     IsToDraw(NOTATION_TOP) ||
+     IsToDraw(NOTATION_LEFT) ||
+     IsToDraw(NOTATION_RIGHT) ||
+     IsToDraw(NOTATION_BOTTOM))
+    mBoundingMetrics.width += psi;

I don't think we want NOTATION_LEFT here.

+  // Set vertical parameters
+  if(!aWidthOnly)

Don't bother testing aWidthOnly for simple vertical metric calculations.
It doesn't matter if the vertical metrics get set.
Added the tests for aWidthOnly adds more code (and may even make it slower).
(aWidthOnly does need to be tested when dealing with mMathMLChar though.)

+  for (PRInt32 i = 0; i < (PRInt32)mMathMLChar.Length(); i++)

Use PRUint32 i, so that the cast is not necessary.

+class nsDisplayMathMLFrame : public nsDisplayItem

class nsDisplayNotation

+  private:

Align "private" consistently with "public".

+nsresult
+nsMathMLmencloseFrame::nsDisplayNotation(nsDisplayListBuilder* aBuilder,

nsMathMLmencloseFrame::DisplayNotation

+                   gfxPoint(rect.XMost(), rect.Y())
+                   );

Closing parentheses in Mozilla are usually at the end of the line, rather than
isolated like this.

Comment 59

[Karl Tomlinson (:karlt)]

(In reply to comment #57)
> +  // The TeXBook (Ch 17. p.141) says that \sqrt is cramped 
> +  UpdatePresentationDataFromChildAt(0, -1,
> +                                    NS_MATHML_COMPRESSED,
> +                                    NS_MATHML_COMPRESSED);
> 
> I'm not really clear what setting NS_MATHML_COMPRESSED does, but it feels
> weird changing the rendering because there is a notation.  (Strikes, for
> example, shouldn't change how things are rendered.)  Let's do this only when
> mNotationsToDraw == NOTATION_RADICAL.

Actually, I guess when IsToDraw(NOTATION_RADICAL) would be better than when
mNotationsToDraw == NOTATION_RADICAL, so that the contents of "horizontalstrike radical" are rendered the same as "radical".

Comment 60

[Frédéric Wang (:fredw)]

Attachment: make the patch compile with current trunk and other fixes according to Karl's comments

Finally I found time to work on this patch. The next step is to reorganize the measurement functions. I hope I'll be able to do it next week. I guess fixing bounds computation will require a more careful study, so I can't say when it will be ready. I attach a testcase for GetIntrinsicWidth.

Comment 61

[Frédéric Wang (:fredw)]

Attachment: GetIntrinsicWidth

Comment 62

[Frédéric Wang (:fredw)]

Attachment: replacement of MeasureChildFrames

Hi Karl,

	I've just made the requested change to fix the MeasureChildFrames issue and... this change actually removed the unexpected spaces in get_intrinsic_width test! Now the radical symbol is displayed in the same way as before the introduction of menclose. Do you really think it is so important to modify the way ink/logical measures are used?

Comment 63

[Frédéric Wang (:fredw)]

Attachment: Testcase for vertical ink extent

Comment 64

[Frédéric Wang (:fredw)]

Attachment: Add a minimal value for height

I made two slight modifications to the previous patch. Before the computation of the bounding box of menclose, I check that the base height is not below the one of a "1" (for boxes, circle & strikes). I think this change addresses the issues you mentioned. Also, I replace the IsToDraw function by a macro.

Comment 65

[Karl Tomlinson (:karlt)]

Comment on attachment 386840 [details] [diff] [review]
Add a minimal value for height

(In reply to comment #62)
> replacement of MeasureChildFrames

(In reply to comment #64)
> Add a minimal value for height

This looks much better, thanks.

A mechanism is needed to deal with dynamic changes to the notation attribute.
It looks like the AttributeChanged() method is for dealing with this.
It's probably easiest to reset the appropriate member variables and
recalculate them.  Watch out that the notation attribute should not affect
msqrt frames.

>diff --git a/layout/mathml/nsMathMLContainerFrame.h b/layout/mathml/nsMathMLContainerFrame.h

>-  // if in an <mrow>, and so <msqrt> frames implement MeasureChildFrames to
>+  // if in an <mrow>, and so their frames implement MeasureChildFrames to

Missed a MeasureChildFrames -> MeasureForWidth

>diff --git a/layout/mathml/nsMathMLmencloseFrame.cpp b/layout/mathml/nsMathMLmencloseFrame.cpp

>+#define IS_TO_DRAW(mask) (mask & mNotationsToDraw)

I preferred the method that you had previously over the macro.  If you want to
make sure that it is considered for inlining you can move the definition to
the class body.

Macros have disadvantages.  Although many of the disadvantages don't apply in
this situation, the general rule should be to avoid macros unless there is a
good reason to use them.

http://www.research.att.com/~bs/bs_faq2.html#macro
http://en.wikipedia.org/wiki/Inline_function#Comparison_to_macros
http://www.parashift.com/c++-faq-lite/inline-functions.html#faq-9.5

>+  mMathMLChar.AppendElement();
>+  mMathMLChar[i].SetData(presContext, Char);

Should check for out of memory during AppendElement() (unfortunately).

>+  nsRect mencloseRect = nsIFrame::GetRect(), rect;
>+  mencloseRect.x = mencloseRect.y = 0;

rect is better declared separately.

>+  if (IS_TO_DRAW(NOTATION_RADICAL)) {
>+    rv = mMathMLChar[mRadicalCharIndex].Display(aBuilder, this, aLists);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    mMathMLChar[mRadicalCharIndex].GetRect(rect);
>+    nscoord dx = rect.width;
>+    nscoord dy = rect.y;
>+    rect.SetRect(dx, dy,
>+                 mencloseRect.width - dx, mRuleThickness);

An nsRect declaration with the default constructor is only needed for this
block, so move the declaration or rect to here.

>+  if (IS_TO_DRAW(NOTATION_BOTTOM)) {
>+    rect.SetRect(0, mencloseRect.height - mRuleThickness,
>+                 mencloseRect.width, mRuleThickness);

These other uses can have their own declaration using the appropriate
constructor instead of SetRect().


Shouldn't radical and longdiv be handled similarly?
Currently the bar for the radical is drawn at the top of the char but the bar
for the longdiv is drawn at the top of the menclose.

>+  // Set vertical parameters
>+  if (((IS_TO_DRAW(NOTATION_TOP) && IS_TO_DRAW(NOTATION_RIGHT) &&
>+        IS_TO_DRAW(NOTATION_BOTTOM) && IS_TO_DRAW(NOTATION_LEFT)) ||
>+       IS_TO_DRAW(NOTATION_UPDIAGONALSTRIKE) ||
>+       IS_TO_DRAW(NOTATION_DOWNDIAGONALSTRIKE) ||
>+       IS_TO_DRAW(NOTATION_VERTICALSTRIKE) ||
>+       IS_TO_DRAW(NOTATION_CIRCLE) ||
>+       IS_TO_DRAW(NOTATION_ROUNDEDBOX)) &&
>+

No blank line here.

>+      bmBase.ascent + bmBase.descent < bmOne.ascent + bmOne.descent

Is this condition necessary?  It creates a discontinuity when the test depends
on the sum but the values being set are ascent and descent individually.

>+      )

Closing parentheses in Mozilla are usually at the end of the line, rather than
isolated like this.

>+      bmBase.descent = PR_MAX(bmOne.descent, bmBase.descent);

I wonder whether "PR_MAX(0, bmBase.descent)" would work fine here.
Could this block then also apply to radical and longdiv?

There is already similar code for radical below, and I can't think why longdiv
should be different to radical.

It looks like perhaps the existing implementation of msqrt could potentially
result in the radical symbol not reaching the baseline, but that seems like a
bug.

>+  // radical notation:
>+  if (IS_TO_DRAW(NOTATION_RADICAL)) {

>+      // built-in: adjust clearance psi to emulate \mathstrut using '1'
>+      // (TexBook, p.131)
>+      if (bmOne.ascent > bmBase.ascent)
>+        psi += bmOne.ascent - bmBase.ascent;

It looks like this can be removed by adding the difference to bmBase.ascent
above.

>+
>+      if (mRuleThickness < onePixel)
>+        mRuleThickness = onePixel;

This can be removed as it has already been done above.

>+        psi += onePixel - delta; // round up
>+
>+
>+
>+      // Stretch the radical symbol to the appropriate height if it is not

One blank line is enough.

>+  ///////////////  
>+  // The maximum size is now computed: set the remaining parameters
>+  mBoundingMetrics.leftBearing = 0;
>+  mBoundingMetrics.rightBearing = mBoundingMetrics.width;

These should also consider the corresponding values in bmBase.

>+
>+  if (IS_TO_DRAW(NOTATION_RADICAL)) {
>+    // get the leading to be left at the top of the resulting frame
>+    // this seems more reliable than using fm->GetLeading() on suspicious
>+    // fonts
>+    leading = nscoord(0.2f * mEmHeight);
>+
>+    aDesiredSize.ascent = mBoundingMetrics.ascent + leading;

The leading should be added to the top of the radical bar "bmBase.ascent + psi
+ mRuleThickness", and aDesiredSize.ascent can be the maximum of the result
and mBoundingMetrics.ascent.

>+    aDesiredSize.height = aDesiredSize.ascent +
>+      PR_MAX(bmBase.descent,
>+             mBoundingMetrics.descent + mRuleThickness);

Similarly, mRuleThickness should be added to the ink descent for the radical
not the entire menclose.  Probably worth recording the value calculated for
radical above.

>+  }
>+  else {
>+    aDesiredSize.ascent = mBoundingMetrics.ascent;
>+    aDesiredSize.height = mBoundingMetrics.ascent + mBoundingMetrics.descent;

These should also consider the corresponding values in baseSize.

>+  //////////////////
>+  // Set position and size
>+  if (IS_TO_DRAW(NOTATION_RADICAL))
>+    mMathMLChar[mRadicalCharIndex].SetRect(nsRect(0, leading, bmRadical.width,
>+                                                  bmRadical.ascent +
>+                                                  bmRadical.descent));
>+
>+  if (IS_TO_DRAW(NOTATION_LONGDIV))
>+    mMathMLChar[mLongDivCharIndex].SetRect(nsRect(0, 0, bmLongdiv.width,
>+                                                  bmLongdiv.ascent +
>+                                                  bmLongdiv.descent));

These characters are stretched considering only the content (which makes sense
to me) and so should be placed relative to the content rather than relative to
the menclose frame.

Setting the rects can move to within the "if (aPlaceOrigin) {" block.

>+
>+  mReference.x = 0;
>+  mReference.y = aDesiredSize.ascent;
>+
>+  if (aPlaceOrigin) {
>+    //////////////////
>+    // Finish reflowing child frames
>+    PositionRowChildFrames(contentDx, aDesiredSize.ascent);
>+  }
>+
>+  return NS_OK;
>+}

>+  NS_DISPLAY_DECL_NAME("MathMLFrame")

Can you use a more precise name here?

>+  private:

Align "private" consistently with "public".

>+  nscoord   mThickness;
>+  PRUint32   mType;

Alignment.

>+void nsDisplayNotation::Paint(nsDisplayListBuilder* aBuilder,
>+                              nsIRenderingContext* aCtx,
>+                              const nsRect& aDirtyRect)
>+{
>+  // get the gfxRect
>+  nsPresContext* presContext = mFrame->PresContext();
>+  gfxRect rect = presContext->
>+    AppUnitsToGfxUnits(mRect + aBuilder->ToReferenceFrame(mFrame));

Does cairo stroke lines inside the path or centered on the path?
I suspect this rect needs to be Inset() appropriately.

>+  nsIDeviceContext *aContext;
>+  aCtx->GetDeviceContext(aContext);
>+  gfxFloat e = mThickness / aContext->AppUnitsPerDevPixel();

gfxFloat e = presContext->AppUnitsToGfxUnits(mThickness);

>+  gfxCtx->SetLineWidth(e);
>+  
>+

Only one blank line.

>+      gfxCtx->Ellipse(rect.pos + rect.size / 2, rect.size);

"/ 2.0" to make it clear that floating point arithmetic is being used.

>+    case NOTATION_UPDIAGONALSTRIKE:
>+      gfxCtx->Line(gfxPoint(rect.X(), rect.YMost()),
>+                   gfxPoint(rect.XMost(), rect.Y()));
>+      break;
>+
>+    case NOTATION_DOWNDIAGONALSTRIKE:
>+      gfxCtx->Line(rect.pos, rect.pos + rect.size);
>+      break;

gfxRect has TopLeft() TopRight() BottomLeft() and BottomRight(), which would
makes things clear here.

>diff --git a/layout/mathml/nsMathMLmencloseFrame.h b/layout/mathml/nsMathMLmencloseFrame.h
>new file mode 100644
>--- /dev/null
>+++ b/layout/mathml/nsMathMLmencloseFrame.h

>+  nscoord      mRuleThickness;

Unnecessary spaces.

>+  PRInt8 mLongDivCharIndex , mRadicalCharIndex;

Remove the space before the ",".

>diff --git a/layout/mathml/nsMathMLmsqrtFrame.cpp b/layout/mathml/nsMathMLmsqrtFrame.cpp

Can some of the header includes be removed from nsMathMLmsqrtFrame.cpp?

>+NS_IMETHODIMP
>+nsMathMLmsqrtFrame::Init(nsIContent*      aContent,

> nsMathMLmsqrtFrame::~nsMathMLmsqrtFrame()
> {
> }
>-
>-NS_IMETHODIMP
>-nsMathMLmsqrtFrame::Init(nsIContent*      aContent,

Can the definition of Init remain after the destructor, please?
That seems more consistent with other MathML frame implementations, and I
don't see any reason for it to move.

Comment 66

[Frédéric Wang (:fredw)]

> Shouldn't radical and longdiv be handled similarly?

Consider this example in the MathML testsuite:

http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/menclose/rec-enclose1-plain.xhtml

	I think the longdiv notation should match the whole cell of the mtable. It is not the case if we draw longdiv like msqrt. Imagine that we replace <mn>131</mn> by <msup><mn>3</mn><mn>5</mn></sup>. Then the top bar of the longdiv would be below the top of this new number, while in my opinion we want them to be aligned.

Comment 67

[Karl Tomlinson (:karlt)]

(In reply to comment #66)
If I understand your example correctly, for the top bar of the longdiv to be aligned with the top of the divisor in this example, the menclose would need to stretch to fill the height of the mtd.  It would be kind of like treating the long division sign as a stretchy operator and menclose as an embellishment of the operator.

The spec doesn't say anything about menclose stretching, and many of the other notations I would not expect to stretch.

Having the longdiv notation expand to the bounds of the menclose rather than bound the content may be OK, but would require stretching the longdiv char to the height of the menclose. It seems simpler to just stretch according to the height of the content.

Comment 68

[Frédéric Wang (:fredw)]

Your are right, streching the longdiv to the height of menclose is not enough. In this case, let's treat longdiv like radical.

Comment 69

[Frédéric Wang (:fredw)]

Attachment: Test dynamic change of notation attribute

Comment 70

[Frédéric Wang (:fredw)]

Attachment: Patch

I think this patch fixes all the problems you mentioned, except for one remark that I do not really understand:

> These characters are stretched considering only the content (which makes sense
> to me) and so should be placed relative to the content rather than relative to
> the menclose frame.

Comment 71

[Karl Tomlinson (:karlt)]

(In reply to comment #70)
> except for one remark that I do not really understand:
> 
> > These characters are stretched considering only the content (which makes
> > sense to me) and so should be placed relative to the content rather than
> > relative to the menclose frame.

The height used for nsMathMLChar::Stretch() is based on the height of the
children.

However, the position passed to nsMathMLChar::SetRect() is relative to the top
of the menclose frame (which may include other notations).

It is better to think of positioning the nsMathMLChars relative to the
baseline.  A vertical position (relative to the top of the menclose) for SetRect() of "aDesiredSize.ascent - bmRadicalChar.ascent" should do the right thing, I think.

I'm less fussy about the horizontal position, but I think
"contentDx - bmRadicalChar.width" would be better.

Comment 72

[Karl Tomlinson (:karlt)]

(In reply to comment #71)
> "aDesiredSize.ascent - bmRadicalChar.ascent"

Actually, aDesiredSize.ascent - radicalAscent.

Comment 73

[Karl Tomlinson (:karlt)]

+      // Update horizontal parameters
+      contentDx = PR_MAX(contentDx, bmRadicalChar.width);
+      mBoundingMetrics.width = PR_MAX(mBoundingMetrics.width,
+                                      bmRadicalChar.width + bmBase.width);

There may be a few places in the patch that are involved in calculation of maximum content offset and maximum width.

What I think we really want is the maximum for the contentDx (= distance to content origin) and the maximum distance from content origin to frame right.
Then the frame width is the sum of these two values.

It may be easier and clearer to have another variable for the distance from content origin to frame right, instead of trying to work with the total width.

Comment 74

[Frédéric Wang (:fredw)]

(In reply to comment #32)
> +  // No need to tract the style context given to our MathML chars. 
> 
> tract -> track
> 

Some files still have this typo. Do you want I include fixes for them in the menclose patch?

fred@localhost:~/mozilla/src/layout/mathml$ grep "No need to tract" *.{h,cpp}
nsMathMLmfencedFrame.cpp:  // No need to tract the style contexts given to our MathML chars. 
nsMathMLmoFrame.cpp:// No need to tract the style context given to our MathML char. 
nsMathMLmrootFrame.cpp:  // No need to tract the style context given to our MathML char.

(In reply to comment #71)
> It is better to think of positioning the nsMathMLChars relative to the
> baseline.  A vertical position (relative to the top of the menclose) for
> SetRect() of "aDesiredSize.ascent - bmRadicalChar.ascent" should do the right
> thing, I think.
> 
> I'm less fussy about the horizontal position, but I think
> "contentDx - bmRadicalChar.width" would be better.

OK, I'll do that. Thanks.

(In reply to comment #73)
> +      // Update horizontal parameters
> +      contentDx = PR_MAX(contentDx, bmRadicalChar.width);
> +      mBoundingMetrics.width = PR_MAX(mBoundingMetrics.width,
> +                                      bmRadicalChar.width + bmBase.width);
> 
> There may be a few places in the patch that are involved in calculation of
> maximum content offset and maximum width.
> 
> What I think we really want is the maximum for the contentDx (= distance to
> content origin) and the maximum distance from content origin to frame right.
> Then the frame width is the sum of these two values.
> 
> It may be easier and clearer to have another variable for the distance from
> content origin to frame right, instead of trying to work with the total width.

	Actually, I think we should even use two variables dx_left and dx_right, and try to maximize them. Then contentDx = dx_left and the width is dx_left + bmBase.width + dx_right. dx_right is generally computed in the same way as dx_left (and is 0 for radical & longdiv). Moreover, I think this will fix another issue of the "circle" notation. Currently, when you use for instance "circle radical", the content is not centered horizontally inside the ellipse (compare with "circle" alone). An easy way to fix that is to make dx_right and dx_left equal (by taking the maximum) when the "circle" notation is drawn.
	
	For vertical positioning, I think ascents/descents are sufficient. Currently the same problem exists for the "circle" notation (content is not always centered vertically inside the ellipse) but I think we can also fix it as described above.

Comment 75

[Frédéric Wang (:fredw)]

Attachment: Show why dx_right is necessary

- radical+box: there is no space between content and left bar
- radical+circle: some part of the content go out of the ellipse

Comment 76

[Karl Tomlinson (:karlt)]

(In reply to comment #74)
> > tract -> track
> > 
> 
> Some files still have this typo. Do you want I include fixes for them in the
> menclose patch?

Yes, would be good to catch those before they spread further, thanks.
Either in this patch or a separate patch (which you can attach to this bug).

>     Actually, I think we should even use two variables dx_left and dx_right,
> and try to maximize them. Then contentDx = dx_left and the width is dx_left +
> bmBase.width + dx_right.

That sounds good.

Comment 77

[Frédéric Wang (:fredw)]

Attachment: tract -> track

Comment 78

[Frédéric Wang (:fredw)]

Attachment: Position of radical/longdiv chars + use of dx_left & dx_right + center content inside ellipse + update boundingmetrics ascent & descent

Here are the announced changes. Also, I update mBoundingMetrics ascent & descent after computing aDesiredSize vertical extent for some notations.

> rect.SetRect(dx, rect.y,
>              mencloseRect.width - dx, mRuleThickness);
  
	Currently for longdiv & radical, the overbar stops at the left of the mencloseRect (see attachment 389511 [details] ). I wonder whether it should rather cover only the enclosed content. I suppose this would require to store bmBase.width in the menclose frame, in order to replace "mencloseRect.width - dx".

Comment 79

[Karl Tomlinson (:karlt)]

Comment on attachment 389721 [details] [diff] [review]
tract -> track

Thank you

Comment 80

[Karl Tomlinson (:karlt)]

(In reply to comment #78)
>     Currently for longdiv & radical, the overbar stops at the left of the
> mencloseRect (see attachment 389511 [details] ). I wonder whether it should rather cover
> only the enclosed content.

Probably.

> I suppose this would require to store bmBase.width
> in the menclose frame, in order to replace "mencloseRect.width - dx".

Yes.  I think storing that on the frame would be a good solution.

I think the shorter bar would be best, but I think I could tolerate a long bar in these unusual situations too.

Comment 81

[Frédéric Wang (:fredw)]

Attachment: patch menclose

Comment 82

[Frédéric Wang (:fredw)]

+  // Set horizontal parameters
+  if (IsToDraw(NOTATION_ROUNDEDBOX) ||
+      IsToDraw(NOTATION_TOP) ||
+      IsToDraw(NOTATION_LEFT) ||
+      IsToDraw(NOTATION_BOTTOM))
+    dx_left = padding;
+
+  if (IsToDraw(NOTATION_ROUNDEDBOX) ||
+      IsToDraw(NOTATION_TOP) ||
+      IsToDraw(NOTATION_RIGHT) ||
+      IsToDraw(NOTATION_BOTTOM))
+    dx_right = padding;
+

	Actually, we can apply the instructions above to NOTATION_CIRCLE and always set padding2 to (ratio - 1) * bmBase.width / 2 below (and similarly for vertical parameters).

+    // Update horizontal parameters
+    padding2 = padding;
+    if (bmBase.width * ratio > bmBase.width + 2 * padding2)
+      padding2 = (ratio - 1) * bmBase.width / 2;
+
+    dx_left = PR_MAX(dx_left, padding2);
+    dx_right = PR_MAX(dx_right, padding2);

Comment 83

[Karl Tomlinson (:karlt)]

Comment on attachment 390040 [details] [diff] [review]
patch menclose

>+nsresult nsMathMLmencloseFrame::AllocateMathMLChar(PRUint32 mask)
>+{
>+  // Is the char already allocated?
>+  if ((mask == NOTATION_LONGDIV && mLongDivCharIndex >= 0) ||
>+      (mask == NOTATION_RADICAL && mRadicalCharIndex >= 0))
>+    return NS_OK;
>+
>+  // No need to track the style context given to our MathML chars.
>+  // The Style System will use Get/SetAdditionalStyleContext() to keep it
>+  // up-to-date if dynamic changes arise.
>+  PRUint32 i = mMathMLChar.Length();
>+  nsAutoString Char;
>+
>+  if (mask == NOTATION_LONGDIV) {
>+    Char.Assign(kLongDivChar);
>+    mLongDivCharIndex = i;
>+  } else if (mask == NOTATION_RADICAL) {
>+    Char.Assign(kRadicalChar);
>+    mRadicalCharIndex = i;
>+  }
>+
>+  if (!mMathMLChar.AppendElement())
>+    return NS_ERROR_OUT_OF_MEMORY;
>+

>+  if (aNotation.EqualsLiteral("longdiv")) {
>+    rv = AllocateMathMLChar(NOTATION_LONGDIV);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    mNotationsToDraw |= NOTATION_LONGDIV;

If "longdiv" is specified twice but the allocation fails in the first call
to AllocateMathMLChar(), then NOTATION_LONGDIV would be (incorrectly) added
to mNotationsToDraw after the second AllocateMathMLChar.
Check AppendElement() before setting mLongDivCharIndex, I think.

>+  nscoord delta = padding % onePixel;
>+  if (delta)
>+    padding += onePixel - delta;

Add "// round up" to the last line here, please.

>+  if ((IsToDraw(NOTATION_TOP) && IsToDraw(NOTATION_RIGHT) &&
>+       IsToDraw(NOTATION_BOTTOM) && IsToDraw(NOTATION_LEFT)) ||

>+      // set a minimal value for the base height

Is there a reason why this shouldn't be done when only NOTATION_LEFT or(/and)
NOTATION_RIGHT is present?

>+  mBoundingMetrics.leftBearing = bmBase.leftBearing;
>+  mBoundingMetrics.rightBearing = bmBase.leftBearing + mBoundingMetrics.width;

What is needed here is the maximum ink extents of the notations and the base.
I think the following should be close enough:

mBoundingMetrics.leftBearing = PR_MIN(0, dx_left + bmBase.leftBearing);
mBoundingMetrics.rightBearing =
  PR_MAX(mBoundingMetrics.width, dx_left + bmBase.rightBearing);

>+  aDesiredSize.height = PR_MAX(mBoundingMetrics.ascent +
>+                               mBoundingMetrics.descent, baseSize.height);

Use dimensions relative to the baseline.
This would be better:

aDesiredSize.height = aDesiredSize.ascent +
  PR_MAX(mBoundingMetrics.descent, baseSize.height - baseSize.ascent);

but

>+    aDesiredSize.ascent = PR_MAX(aDesiredSize.ascent,
>+                                 longdivAscent + leading);
>+    aDesiredSize.height = PR_MAX(aDesiredSize.height,
>+                                 aDesiredSize.ascent +
>+                                 PR_MAX(bmBase.descent, longdivDescent +
>+                                        mRuleThickness));

this doesn't consider the changes needed in height when ascent becomes
longdivAscent + leading.

I think it might be best to use another variable for descent and then
calculate the maximum ascent and descent rather than maximum ascent and
height.

>+    // center the content (vertically)

I think a better description would be "center the menclose around the
content", as it is really the menclose being changed here, not the position of
the content (as laid out in my mind, at least).

>+  nsDisplayNotation(nsIFrame* aFrame, const nsRect& aRect,
>+                    nscoord aThickness, PRUint32 aType)

>+  PRUint32 mType;

>+  nsresult DisplayNotation(nsDisplayListBuilder* aBuilder,
>+                           nsIFrame* aFrame, const nsRect& aRect,
>+                           const nsDisplayListSet& aLists,
>+                           nscoord aThickness, PRUint32 aType);

By giving the type of the NOTATION_* enum a name, all these PRUint32 types can
be replaced with the specific enum type (which provides better compile-time
type checking).

(In reply to comment #82)
>     Actually, we can apply the instructions above to NOTATION_CIRCLE and always
> set padding2 to (ratio - 1) * bmBase.width / 2 below (and similarly for
> vertical parameters).

Yes, please.  That would simply things.

The rest looks good, thank you.

Comment 84

[Frédéric Wang (:fredw)]

Attachment: changes according to previous comments

Comment 85

[Karl Tomlinson (:karlt)]

> -      // round up psi
>       delta = psi % onePixel;
>       if (delta)
> -        psi += onePixel - delta;
> +        psi += onePixel - delta; // round up

This change is ok, but I was actually asking for a comment for the rounding up of
padding (not psi).

Thanks for the other changes.  I'll see if I can try this out today.

Comment 86

[Karl Tomlinson (:karlt)]

Comment on attachment 392551 [details] [diff] [review]
changes according to previous comments

Looks nice!
r=me with "// round up" here:

>+  nscoord delta = padding % onePixel;
>+  if (delta)
>+    padding += onePixel - delta;

Comment 87

[Frédéric Wang (:fredw)]

(In reply to comment #85)
> This change is ok, but I was actually asking for a comment for the rounding up
> of
> padding (not psi).
> 

Oops, sorry :-) I'll fix it.

(In reply to comment #86)
> (From update of attachment 392551 [details] [diff] [review])
> Looks nice!
> r=me with "// round up" here:

Thank you for the review!

Comment 88

[Frédéric Wang (:fredw)]

Attachment: Final menclose patch

Comment 89

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 392745 [details] [diff] [review]
Final menclose patch

rubber-stamp=me --- this doesn't really need sr

Comment 90

[Karl Tomlinson (:karlt)]

Checked in, thanks:

http://hg.mozilla.org/mozilla-central/rev/dc8ef04fc953

Needed a small change

http://hg.mozilla.org/mozilla-central/rev/7326f167a78b

for

e:/builds/moz2_slave/mozilla-central-win32-debug/build/layout/mathml/nsMathMLmencloseFrame.cpp(464) : error C2668: 'sqrt' : ambiguous call to overloaded function
        D:\msvs8\VC\INCLUDE\math.h(581): could be 'long double sqrt(long double)'
        D:\msvs8\VC\INCLUDE\math.h(533): or 'float sqrt(float)'
        D:\msvs8\VC\INCLUDE\math.h(128): or 'double sqrt(double)'
        while trying to match the argument list '(int)'

Apparently MSVS has some extra sqrt functions in its math.h, but usually its best to use double constants in double arithmetic anyway.

Comment 91

[Frédéric Wang (:fredw)]

Thank you.
What about attachment 389721 [details] [diff] [review]?

Comment 92

[Karl Tomlinson (:karlt)]

(In reply to comment #91)
> What about attachment 389721 [details] [diff] [review]?

Sorry, that slipped my mind.  I've made a note to myself to check that in, but it may be several days before I do.

Comment 93

[Karl Tomlinson (:karlt)]

Comment on attachment 389721 [details] [diff] [review]
tract -> track

http://hg.mozilla.org/mozilla-central/rev/7cebc1512da6

Comment 94

[Eliot Kimber]

Thanks for resolving this issue. I'm trying to determine when I can expect the fixes to be released into Firefox? Quick testing with 3.5.4 on OSX indicates that most, if not all, of menclose is not implemented, so I guess this fix hasn't made its way to the Firefox base.

Cheers,

Eliot

Comment 95

[Frédéric Wang (:fredw)]

(In reply to comment #94)
> Thanks for resolving this issue. I'm trying to determine when I can expect the
> fixes to be released into Firefox? Quick testing with 3.5.4 on OSX indicates
> that most, if not all, of menclose is not implemented, so I guess this fix
> hasn't made its way to the Firefox base.
> 
> Cheers,
> 
> Eliot

Apparently, the target milestone is mozilla 1.9.2 so I guess I will be in Firefox 3.6

Comment 96

[Eliot Kimber]

Thanks--I wasn't sure how to map the Mozilla releases to Firefox releases.

Cheers,

Eliot

Comment 97

[Eliot Kimber]

Got the 3.6 beta and the menclose test looks great. Thanks again for getting all this implemented.

Cheers,

Eliot