363240 - incorrect MathML <mtable> width and position (nsMathMLContainerFrames have zero preferred width)

Status: RESOLVED FIXED

(Core :: MathML, defect, P2)

Description

[Jesse Ruderman]

This testcase broke between 2006-12-05 and now, probably due to the reflow branch landing.  You should see a 2x2 matrix bounded by "[" and "]", but instead you see an empty matrix.

(Note that even before the reflow branch landing, the stretchy brackets weren't stretching properly, and there were alignment issues.)

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

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

Almost certainly due to the fact that MathML does not implement GetPrefWidth/GetMinWidth.

Comment 3

[AndrewM]

(In reply to comment #0)
> This testcase broke between 2006-12-05 and now, probably due to the reflow
> branch landing.  You should see a 2x2 matrix bounded by "[" and "]", but
> instead you see an empty matrix.

I just see A = on Vista.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102205 Minefield/3.0a9pre ID:2007102205

Comment 4

[Karl Tomlinson (:karlt)]

The pref- and min- widths of nsMathMLContainerFrames are functions of parameters including the ascent and descent of bounding metrics of child frames, and overflow rectangle widths. (With appropriate coordinate transformations, the union of the bounding metrics rectangle and the frame rectangle is the overflow rectangle.)

Most of the work required in a reflow is therefore required to calculate pref- and min- widths.  So it seems that the best way to calculate these values is to actually Reflow() in Get*Width, much like how prefwidths were once calculated
(http://wiki.mozilla.org/Gecko:Reflow_Refactoring#Intrinsic_sizing_changes) or like what is done in nsFrame::GetPrefSize() (but GetPrefWidth() could not be called recursively).

Pref- and min- widths could differ when there is a line break opportunity in the text of an <mi> or <mo> element for example, or if there are non-MathML elements.  If we want to handle these cases, then this could require Reflow()ing up to three times (when in a table).

Alternatively we could just choose not to break and we may be able to only do the reflow calculations once.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: implement GetMinWidth/GetPrefWidth with a Reflow

Overrides GetMinWidth/GetPrefWidth() for nsMathMLContainerFrame.
These calculate the values by setting up an nsHTMLReflowState and reflowing
with zero width or a large width.

ComputeSize() is also overrided with a basic maximum-size calculation so that
setting up the nsHTMLReflowState doesn't require calling
GetMinWidth/GetPrefWidth().

nsMathMLContainerFrame is made a containing block so that there is no need to
set up a bogus parent reflow state for the containing block.  This seems
fairly reasonable to me as each nsMathMLContainerFrame behaves a bit like an
inline-block: i.e. none of the content is (currently) wrapped to another line.
What are the functions that a containing block should provide?

Issues:

* Widths could be cached.

* What is a suitable large enough width for the Reflow for GetPrefWidth?

   * Really we want NS_UNCONSTRAINEDSIZE, but nsHTMLReflowState and
     nsLayoutUtils::ComputeWidthDependentValue() don't like that.

   * Available width or our parent containing block width would be fine but
     we don't have access to the parent reflow state here.

   * PresContext->GetVisibleArea().Size() is OK much of the time, but there
     are times when the available size might be larger than that.

* We shouldn't need to reflow again if PrefWidth == MinWidth.
  Can we avoid doing that?

* If PrefWidth != MinWidth do we need to restore
  NS_FRAME_IS_DIRTY/HAS_DIRTY_CHILDREN after they have been cleared to make
  sure another Reflow happens?

Comment 6

[Henri Sivonen (:hsivonen)]

In Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007123004 Minefield/3.0b3pre I see the mtable contents but the mtable is too thin and too tall.

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to comment #6)
> I see the mtable contents but the mtable is too thin and too tall.

Too tall is bug 348577.  (The mtable is in the wrong position and the brackets are centered around an axis at 250/430 * xHeight above the baseline.)

Too thin is this bug (even though the symptoms are now different).

Comment 8

[Bill Gianopoulos [:WG9s]]

Linux and Windows test builds that include this patch are available at:

http://www.wg9s.com/mozilla/firefox/

Please not that this is not the final patch, and read the limitations of this patch in comment #5 before adding comments based on testing using these builds.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: implement GetMinWidth/GetPrefWidth with zero-available-width/unconstrained Reflow

This works but it's not pretty.  When getting bounds for GetPrefWidth, reflow
MathML frames with unconstrained widths, but reflow foreign child frames with
available width set to their preferred width (because foreign frames don't
know what to do with unconstrained widths).

The algorithm is order
<mi>exp</mi><mo>&ApplyFunction</mo><mfenced><mi>d</mi></mfenced> where
<mi>d</mi> is the number of non-MathML direct parents of MathML frames in an
ancestor chain from a leaf child to the MathML frame for which GetPrefWidth is
called.  Fortunately <mi>d</mi> is expected to be small.

The remaining question is: do we need to set some state bits to ensure that
another reflow happens with the calculated available width?
I haven't found any problems, but perhaps there might be problems with
incremental reflow.

Comment 10

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

+                             nsSize aMargin, nsSize aBorder, nsSize aPadding,

const nsSize& ?

+  // If PrefWidth != MinWidth, should we do something to make sure another
+  // reflow happens?  Maybe AddStateBits(NS_FRAME_IS_DIRTY/HAS_DIRTY_CHILDREN)?
+  // NS_FRAME_IS_DIRTY will be added by the nsHTMLReflowState from-parent
+  // constructor if the parent is dirty.  Could the parent be calling GetPref*
+  // on this if it didn't have NS_FRAME_IS_DIRTY?

I think the answers are "yes" and "yes", but I don't know the best way to do this. David?

Comment 11

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

It could.  The mess of possibilities there is one of the reasons we had so many problems with the pre-reflow branch code.  Do we really want to go back there?

(I though there was another bug on implementing intrinsic widths for MathML where I had a bunch of comments about what I thought needed to be done.  Is it really that hard to just implement GetMinWidth/GetPrefWidth for all the frames?)

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to comment #11)
> (I though there was another bug on implementing intrinsic widths for MathML
> where I had a bunch of comments about what I thought needed to be done.

bug 400207 comment 3 ?

Or "similar refactoring decisions as for nsLeafFrame, but the inline intrinsic
width API may be more involved here"?
http://wiki.mozilla.org/Gecko:Reflow_Refactoring#Future_Development_Tasks

> Is it really that hard to just implement GetMinWidth/GetPrefWidth for all
> the frames?)

See comment 4.

It may be possible.  The following would be required:

1) An equivalent of GetPrefWidth that returned the smallest rectangle enclosing
   glyph bounds and advance widths.

   * This could perhaps be achieved with the existing GetPrefWidth API if we
     could persuade some text frames to return set their frame bounds to
     include the entire overflow area.

2) An API to retrieve the tight ink bounds that text frames and their parent
   block frames (at least) would have if laid out at min/preferred width.

3) A less strict version of 2 for non-text non-MathML children.  GetPrefSize
   is close (but does a Reflow).  I'm not sure whether GetMinSize extends the
   height appropriately with line wrapping.

> The mess of possibilities there is one of the reasons we had so many
> problems with the pre-reflow branch code.  Do we really want to go back there?

I wonder whether we may need to go back to there at some stage for some
advanced line-wrapping algorithms.  For example it might be nice at some stage
to have something like an inline-block that shrinks when its content wraps and
doesn't take up all available width (even when less than the preferred width).

Comment 13

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

Are there plans to allow line breaks within MathML?  I didn't think there were.  If not, nothing like the inline width API is needed.  (That was the conclusion rbs and I came to when we discussed it at the Firefox summit in December 2006.)

At what points are bounding metrics considered?  Are they always considered at the lowest level of the tree, or are there cases where frames can be adjacent based on their frame bounds but then the union of the bounding metrics of both is considered by an ancestor?

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to comment #13)
> Are there plans to allow line breaks within MathML?  I didn't think there
> were.  If not, nothing like the inline width API is needed.  (That was the
> conclusion rbs and I came to when we discussed it at the Firefox summit in
> December 2006.)

http://www.w3.org/TR/2007/WD-MathML3-20071214/chapter3.html#id.3.3.1.2

  "MathML is designed to allow renderers to automatically linebreak
   expressions... This is because linebreaking positions can't be chosen well
   without knowing the width of the display device and the current font
   size..."

   "Although MathML does not require linebreaking..."

http://www.w3.org/TR/2007/WD-MathML3-20071214/chapter3.html#id.3.2.7.2
describes attributes of mspace to control line breaks.

I don't think we should make any design decisions that might preclude line
breaking.

In FF2 <math> elements does line-breaking because it is implemented using
nsBlockFrame and nsInlineFrame.  Spacing is handled by changing the frame
bounds of the child frames when they have a <math> direct parent.  But <mrow> and other elements do not do line-breaking.

Currently on 1.9, <math> elements do line-breaking, and elements that use text
frames do a sort of line-breaking using the inline-blocks, but this works
poorly because available width calculations are poor (non-existent) and
inline-blocks don't shrink-wrap.  We could turn this off with a
width:-moz-max-content on the inline-block, if we want to.

> At what points are bounding metrics considered?  Are they always considered
> at the lowest level of the tree, or are there cases where frames can be
> adjacent based on their frame bounds but then the union of the bounding
> metrics of both is considered by an ancestor?

They are usually considered by the parent or ancestor.

Ideally, both tight metrics and typo metrics (currently related but not quite
equal to frame bounds and baseline) should be available.

Some parent frames choose to use tight metrics, some typo metrics, some a
union and some a more complex TeX-derived formula.  Sometimes this is
carefully selected, other times I'm not sure if the choice is really
intentional. e.g. <msup> uses the union for the right side of the base but
typo for the left side of the script.

Usually there is only one metric (tight or typo or union or other combination)
for each direction that needs to propagate to the ancestors but which metric
depends on the ancestor.  This metric (in one direction) may only need to
propagate to one ancestor (i.e. it is the same type of metric all the way to
that ancestor) but other metrics (for other directions) may need to propagate
to different ancestors.

e.g.

<mrow>
  <munderover>
    <mo>&Sum;</mo>
    <mn>0</mn>
    <mn>2</mn>
  </munderover>
  <mfrac>
    <mi>n</mi>
    <msub><mi>y</mi><mi>n</mi></msub>
  </mfrac>
</mrow>

The placement of the numerator(/denominator) in the <mfrac> depends on a
function of the typo and tight descent(/ascent) of the child, but the mfrac must
use the tight ascent(/descent) of the child for its tight ascent(/descent)
used to determine the size (including width) of the &Sum; operator.

So getting the child to decide which metric to provide based on an analysis of
ancestors would seem an unfortunate dispersion of the logic.

Comment 15

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

(In reply to comment #12)
> I wonder whether we may need to go back to there at some stage for some
> advanced line-wrapping algorithms.  For example it might be nice at some stage
> to have something like an inline-block that shrinks when its content wraps and
> doesn't take up all available width (even when less than the preferred width).

That doesn't require going back to the pre-reflow-branch code; it just requires using the inline intrinsic width API instead (and it might require a few extensions to allow elements to contribute for every line on which they occur).


We could also extend the inline intrinsic width API (or add a new one more similar to the block one) that provides both metrics so that the ancestors can choose.


I'm not sure what to do about the summations issue.  I don't like the idea of allowing intrinsic widths to depend on layout heights, especially if you're eventually going to allow line breaking inside those layout heights.  I think it could pretty quickly end up in situations where we can't satisfy all the constraints, and I think we need to figure out some compromise.



I'm worried that this is not only going to cause a bunch of incremental layout bugs with MathML content, but that it's going to mean that other things we'd like to do in the future with intrinsic widths (like further optimization of 'top' and 'left' CSS property changes) will cause additional bugs in MathML, and that those bugs (and our regression tests) would prevent us from making those optimizations because of this change.


Do we currently allow non-MathML content inside of MathML?

Comment 16

[distler]

>Do we currently allow non-MathML content inside of MathML?

Yes.

See, e.g.:

http://golem.ph.utexas.edu/~distler/blog/archives/001533.html

for embedded SVG. A discussion of what one might want to allow in the future (or what would work, modulo current bugs):

http://golem.ph.utexas.edu/~distler/blog/archives/001475.html

Comment 17

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

(In reply to comment #15)
> I'm not sure what to do about the summations issue.  I don't like the idea of
> allowing intrinsic widths to depend on layout heights, especially if you're
> eventually going to allow line breaking inside those layout heights.  I think
> it could pretty quickly end up in situations where we can't satisfy all the
> constraints, and I think we need to figure out some compromise.

This is the key issue.

Maybe we could have GetIntrinsicWidth invoke MathML layout in a way that we don't call Reflow on non-MathML descendants, just get their intrinsic widths and fake a height. That way, we don't let evilness escape MathML and things work as long as you don't include non-MathML content in contexts where its height affects widths. This might be difficult for mtable and other places we reuse regular layout frames for MathML though...

Comment 18

[Karl Tomlinson (:karlt)]

(In reply to comment #15)
> I'm not sure what to do about the summations issue.  I don't like the idea of
> allowing intrinsic widths to depend on layout heights, especially if you're
> eventually going to allow line breaking inside those layout heights.

That is the core of the issue.  MathML frame widths depend on the heights of
other frames (and at other times heights depend on widths).  Is MathML the
only situation where this happens?

I can see that the min width computation then becomes more complex, as some
components in the equation become wider as others become narrower.  But the
simple solution to this is to fallback to the preferred width if it is
difficult to find a narrower arrangement.

> I think it could pretty quickly end up in situations where we can't satisfy
> all the constraints,

Without line-breaking it is simple to satisfy the constraints.  With
line-breaking, it would probably involve testing possibilities, and the
fallback to no break if nothing helps.

> and I think we need to figure out some compromise.

It would be unfortunate if the bracket here did not expand around the text on
line-breaking:

  <mrow>
    <mo>{<mo>
    <mtext>a paragraph of text</mtext>
  <mrow>

If really necessary perhaps we could do a distortion of the { when restricted
in width, but it would be better to request this width as part of min width.

We can disable line-breaking for 1.9 if that is going to help.

Comment 20

[Karl Tomlinson (:karlt)]

Changed summary from "MathML <mtable> appears empty" to reflect the current symptoms: incorrect width and position (also bug 412871)

Comment 21

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

So I'm having trouble seeing why MathML frame widths *ought* to depend on heights.  TeX doesn't do formatting that way, as far as I can tell, and introducing such formatting seems to pose problems both for intrinsic width calculation and for line breaking.

As far as I can tell, TeX sizes integrals and summations based on what MathML calls displaystyle and scriptlevel (not based on the size of what they're next to), and it does parentheses and brackets of various types and radicals such that they don't vary in width as their height changes.

(That said, there might be a way, in TeX, to associate summations or integrals with contents, which the language doesn't naturally do.  Using \left. and \right. seemed like the most natural way, but it didn't seem to do anything.)

Comment 22

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

Attachment: some TeX examples

Comment 23

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

Attachment: some TeX examples (PDF)

PDF of attachment 299042 [details], generated with pdflatex.

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to comment #21)

Thank you for looking into this, David.

> As far as I can tell, TeX sizes integrals and summations based on what
> MathML calls displaystyle and scriptlevel (not based on the size of what
> they're next to),

Yes, this is what TeX seems to do.  It seems this is a matter of preference.

UTR #25 indicates that these sometimes stretch.
"Large Operators include n-ary operators like summation and integration. They
may expand in size to fit their associated expressions."
http://www.unicode.org/reports/tr25/tr25-9.html#_Toc323

Sergey Malkin also pointed out to me that Knuth preferred integral operators to stay the same for all expressions inside them.  I probably share that preference. When several integrals are added together in one equation, they look better with consistent integral operators.  I also prefer summation operators that don't stretch to fit their contents.  Sums of integrals could lead to excessively large summation operators.

The MathML suggested operator dictionary has default stretchy="true" and this is
probably from where our operator dictionary was derived.
http://www.w3.org/TR/2007/WD-MathML3-20071214/appendixb.html#oper-dict.entries

The suggested operator dictionary is non-normative.  But the text of the MathML spec explicitly says:
"Also, operators such as ∑, ∫, /, and vertical arrows stretch vertically by default."
http://www.w3.org/TR/2003/REC-MathML2-20031021/chapter3.html#id.3.2.5.8

I think rendering would look better if &sum and &int sizes depended only on
displaystyle, but I hesitate to change this as it would be contrary to the
specification.

Comment 25

[Karl Tomlinson (:karlt)]

There is some leeway in the spec: "In practice, typical renderers will only be
able to stretch a small set of characters, and quite possibly will only be
able to generate a discrete set of character sizes."  So I guess authors can't
rely on operators being stretched, and, for integral and sum (but not grouping
symbols), the meaning of the expression does not change according to the
operator size.

We could even follow the letter of the spec by making the operator stretchy
but using a maxsize (and maybe minsize) attribute in the operator dictionary.
However, it could be counter-intutive to authors who actually wanted to have
stretchy operators, if they needed to specify maxsize=infinity to get that.

It certainly would reduce the magnitude of the issue if summation (and product)
operators did not stretch (by default).

Comment 26

[Karl Tomlinson (:karlt)]

Attachment: varying widths of stretchy operators in TeX

(In reply to comment #21)
> and it does parentheses and brackets of various types and radicals such
> that they don't vary in width as their height changes.

The widths do in fact change, but to a lesser extent.

Most of these operators can stretch to any size by building from parts and
therefore have a maximum width.

Operators that cannot be built from parts such as angle brackets only have a
maximum width because the font runs out of glyphs.  Currently Mozilla also
reaches this limit (which depends on the font), but I had been considering
changing the font size or transformation matrix to generate larger glyphs (the
main motivation being that most fonts don't have different glyphs for each
size).  In many circumstances the scaling in the horizontal direction could be
limited to a maximum.  Care would need to be take with angle brackets to
ensure that they didn't end up looking like vertical lines.

(If we are prepared to ignore the sentence about ∑ stretching)
Is returning some sort of maximum width in GetPrefWidth an option?

Does it matter if a frame doesn't occupy its preferred width when available?
Some likely consequences: 

* A little extra whitespace in tables.

* Linebreaking sometimes when unnecessary.

* The maxsize attribute could end up changing the preferred width even if it
  didn't change the actual width.

Anything else?

Comment 27

[Karl Tomlinson (:karlt)]

Attachment: varying widths of stretchy operators in TeX (source)

Comment 28

[distler]

Even though they don't involve <mtable>s, can I assume that the failure of square-root signs and under- and over-braces to stretch correctly on

    http://xbeta.org/wiki/show/itex+fractions+superscripts+and+subscripts

is related to this bug?

(I have a similar question about the failure of widetilde and widecheck to stretch on 

   http://xbeta.org/wiki/show/itex+punctuation

). As to integrals, getting uniform stretching is usually a simple matter of astute placement of <mrows>. See, e.g.,

    http://xbeta.org/wiki/show/itex+large+operators

(Note, though that \sum and \prod don't seem to stretch, as they should.)

Comment 29

[Karl Tomlinson (:karlt)]

(In reply to comment #28)
Thanks for testing.

> Even though they don't involve <mtable>s, can I assume that the failure of
> square-root signs and under- and over-braces to stretch correctly on
> 
>     http://xbeta.org/wiki/show/itex+fractions+superscripts+and+subscripts
> 
> is related to this bug?

Not this bug.
The square-root signs look fine to me with STIX fonts installed (bug 412880).
The medium sizes of under- and over-braces are missing wrt bug 407101.

> I have a similar question about the failure of widetilde and widecheck to
> stretch on 
> 
>    http://xbeta.org/wiki/show/itex+punctuation

Also bug 407101.

> As to integrals, getting uniform stretching is usually a simple matter of
> astute placement of <mrows>. See, e.g.,
> 
>     http://xbeta.org/wiki/show/itex+large+operators

I can imagine that placing an mrow around a whole equation involving sums of integrals would give uniform integral sizes provided none of the integrals were inside other mrow expressions.  But its harder to get this to work when some of the integrals are enclosed within grouping brackets, which should have their own mrow.

> (Note, though that \sum and \prod don't seem to stretch, as they should.)

So you like these to stretch, I assume.  Interesting.

Most fonts don't have different sizes of sum and product.  STIX fonts have U+23B2 and U+23B3 SUMMATION TOP and BOTTOM, which we could use, but the fonts have nothing for product.

I just filed bug 414277, which would help with many of these issues.

Comment 30

[distler]

>> Even though they don't involve <mtable>s, can I assume that the failure of
>> square-root signs and under- and over-braces to stretch correctly on
>> 
>>     http://xbeta.org/wiki/show/itex+fractions+superscripts+and+subscripts
>> 
>> is related to this bug?
>
>Not this bug.
>The square-root signs look fine to me with STIX fonts installed (bug 412880).

Hmmm. Funny. I tested this with the latest SeaMonkey nightly and the STIX fonts (on MacOSX). The square root stretches horizontally just fine, but not vertically.

Perhaps I should open a bug for that.

>So you like these to stretch, I assume.  Interesting.

The MathML Spec,

   http://www.w3.org/TR/2003/REC-MathML2-20031021/chapter3.html#id.3.2.5.8.2

says that most "largeop" operators are supposed to have stretchy="true". As pointed out in comment 14, TeX doesn't do that.  But the default size of these "largeop" operators in TeX is considerably larger (at least for display equations) than the current behaviour in Gecko.

For most purposes, the TeX behaviour works well enough. (It should be remembered that TeX's behaviour was determined in an era when scalable fonts didn't exist. Stretching a parenthesis with an extender glyph was one thing; stretching a summation sign would have required running Metafont on the fly.)

I'd be happy enough if the largeop operators were rendered in a larger font-size (as TeX does) in display equations. (I'll repeat this last point in the comments to bug 414277.)

Comment 31

[Karl Tomlinson (:karlt)]

(In reply to comment #30)

> Hmmm. Funny. I tested this with the latest SeaMonkey nightly and the STIX
> fonts (on MacOSX). The square root stretches horizontally just fine, but not
> vertically.
> 
> Perhaps I should open a bug for that.

Please do.  (I'd prefer to duplicate bug reports than not have any, but I don't think this has been reported.)

Comment 32

[distler]

Bug 414289 filed. And, since I was at it, I filed bug 414294 (an unrelated problem with stretchy delimiters) as well.

Comment 33

[Karl Tomlinson (:karlt)]

(swag based on an upper bound computation for each MathML frame type, and something like a GetPrefTightWidth computation for text frames and block frames, falling back to GetPrefWidth for other frame types.  I know little about lines within blocks and the inline intrinsic width API at this stage, so that is an unknown here.)

Comment 34

[Karl Tomlinson (:karlt)]

Suggested release note for Beta 3:

"Rendering of MathML with tables is currently incorrect (Bug 363240).
 Installing STIX fonts will provide the best mathematical character support.
 More information is in this post:
 http://groups.google.com/group/mozilla.dev.tech.mathml/msg/d07faab25b48035c"

Comment 35

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

Is there a reasonable upper bound you could use to approximate for our current intrinsic width methods?

Comment 36

[Karl Tomlinson (:karlt)]

(In reply to comment #35)

If we add a method to provide left and right bearing information at preferred
size (or even a union of this line segment with the frame width for an upper
bound on the metrics), then there is an upper bound for the majority of common
cases.

For cases involving operators that don't have a maximum width, we could have a
self imposed maximum width.  This value of this limit could be the normal
width of the glyph multiplied by some factor chosen to compromise between
having too much excess white space at small heights and excessively distorted
glyphs at large heights.

(Comment #26 mentions some of the issues here.)

Perhaps the compromises involved could be less significant if tables detected
cases where widths after Reflow were greater than advertised min width and
reconsidered the layout.  Is this worth considering (as a later improvement)?

Comment 37

[Karl Tomlinson (:karlt)]

Attachment: don't use frame origin offsets to store ascents

This provides a separate place to store child frame sizes and ascents during Reflow() and Stretch() (and GetPrefWidth() to come).

This doesn't fix the issue here but sets things up so that we can (usually) use the same "Place()" methods that are used to measure for Reflow/Stretch (MeasureAndMaybePlace() may be a better name) to also measure for intrinsic widths.

Comment 38

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

How about doing mChildReflowMetrics just as a property we set on child frames via nsIFrame::SetProperty?

Comment 39

[Karl Tomlinson (:karlt)]

Attachment: GetPrefHBounds API

This adds the method suggested in comment 36, and necessary for an upper bound.

I've included frame width in the return value because I assume that any computation of left and right bearing will pretty much know the width.  Getting all the metrics from one method saves doing the computation twice.

Currently there is only part of a default implementation, but this (with corresponding MathML frame changes) will be enough to make a big improvement and the rest can be filled in later.

Comment 40

[Karl Tomlinson (:karlt)]

I haven't included a GetMinHBounds() as our current MathML line breaking algorithms are not good, so I'm thinking (for now) it is generally better just to force the user to scroll rather than breaking equations in strange places.

I haven't yet decided whether we should have a GetMinHBounds when we do line-breaking.  When we do that we'll be keeping track of available width better and we may decide to just cram things closer together and distort tall operators if limited by space.

Comment 41

[Karl Tomlinson (:karlt)]

Attachment: don't use frame origin offsets to store ascents v2 [checked-in]

Same as attachment 304932 [details] [diff] [review] (comment 37), but using nsIFrame::SetProperty().

Comment 42

[Karl Tomlinson (:karlt)]

Attachment: Get*Width with GetPrefHBounds for nsMathMLContainerFrame

Includes a MathML base class implementation of GetPrefHBounds.
This provides the correct width for many elements, but mroot, msqrt, mfenced, mfrac with slash, and stretchy mo elements will need their own implementations, which will require an nsMathMLChar maximum width calculation.

Comment 43

[Karl Tomlinson (:karlt)]

Attachment 305661 [details] [diff] depends on attachment 305450 [details] [diff] [review] and attachment 305651 [details] [diff] [review].

Comment 44

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

Comment on attachment 305651 [details] [diff] [review]
don't use frame origin offsets to store ascents v2 [checked-in]

+    mParentFrame->
+      GetReflowAndBoundingMetricsFor(mChildFrame, mSize, mSize.mBoundingMetrics,

You don't need mParentFrame-> ?

(Don't forget to include the atom in nsGkAtoms when you land)

Comment 45

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

Comment on attachment 305450 [details] [diff] [review]
GetPrefHBounds API

+   * |leftBearing| and |rightBearing| are tight (foreground) ink bounds.  They

What does "foreground" mean here?

Adding new methods to nsIFrame sucks, but I guess we have no choice here. The new API looks OK to me.

Comment 46

[Karl Tomlinson (:karlt)]

Comment on attachment 305651 [details] [diff] [review]
don't use frame origin offsets to store ascents v2 [checked-in]

Checked this in (but it shouldn't make any visible difference):

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1203999480&maxdate=1203999600&who=karlt%2B%25karlt.net

Comment 47

[Karl Tomlinson (:karlt)]

Attachment: Get*Width with GetPrefHBounds for nsMathMLContainerFrame

Attachment 305661 [details] [diff] (comment 42) with some changes (I forgot to include) that are needed for correct calculations in nsMathMLTokenFrame::Place().
(Depends on attachment 305450 [details] [diff] [review])

Comment 48

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

Comment on attachment 305450 [details] [diff] [review]
GetPrefHBounds API

>+  virtual HorizontalBounds
>+  GetPrefHBounds(nsIRenderingContext *aRenderingContext) = 0;

GetPrefHBounds doesn't really say clearly what it does.  Would GetPrefWidth[And/With]InkBounds be clearer?

The comment should probably also say what it's used for (i.e., mention MathML).


>+  // TODO: include border and be consistent with ComputeTightBounds (bug 363240)

If the intent is to include the padding and border but not the margin, maybe you should call nsLayoutUtils::IntrinsicForContainer(aRenderingContext, this, nsLayoutUtils::PREF_WIDTH) instead of calling GetPrefWidth(aRenderingContext) directly.  That would also include the margin, but maybe it's better to overestimate than underestimate?  (While working on the reflow branch, I actually had a fourth parameter to IntrinsicForContainer to specify which parts you'd get, so one could ask for things like what you seem to want here.  (I think I got rid of it when I introduced nsIFrame::IntrinsicWidthOffets.)  Maybe it's worth reintroducing, although I did manage to get rid of all the other uses.)

That said, why do you want a border-box width?  Wouldn't margin-box be easier?

Comment 49

[Karl Tomlinson (:karlt)]

(In reply to comment #48)
> Would GetPrefWidth[And/With]InkBounds be clearer?

GetPrefWidthWithInkBounds is OK.  "With" rather than "And" provides the faint implication that it is only horizontal metrics.

How about GetPrefWidthWithTightBounds for consistency with ComputeTightBounds?

>  maybe
> you should call nsLayoutUtils::IntrinsicForContainer(aRenderingContext, this,
> nsLayoutUtils::PREF_WIDTH) instead of calling GetPrefWidth(aRenderingContext)
> directly.

I'll think about that, thanks, but borders are not the top priority here.

> That said, why do you want a border-box width?  Wouldn't margin-box be easier?

I'm not too fussy.  Borders are really for non-MathML children.  Border box just sounded more consistent with the notion of ink bounds.  But if someone asked for a margin, then it would seem to make sense to provide one.  I'll remove the comment about the margin anyway.

Comment 50

[Karl Tomlinson (:karlt)]

Comment on attachment 305450 [details] [diff] [review]
GetPrefHBounds API

I'm rethinking how useful it is to have advance width returned with left and
right bearing.  It seems that nsLayoutUtils::IntrinsicForContainer is the best
way to get the useful information (including padding+border+margin) from the
advance width (PrefWidth).  In contrast, it seems that left and right bearing
should be used without adding padding+border+margin (though this will already
be included when there is a border).

Comment 51

[Karl Tomlinson (:karlt)]

Comment on attachment 305681 [details] [diff] [review]
Get*Width with GetPrefHBounds for nsMathMLContainerFrame

This should be modified to use nsLayoutUtils::IntrinsicForContainer for the
child frame width (and whatever API for left and right bearing).

Currently MathML frames are not considering padding+border+margin so
GetPrefWidth is fairly consistent with MathML Reflow.  MathML frames position
child frames using the metrics returned from Reflow, but it seems that margins
should first be added.  MathML frames are also not including their own border
and padding in the metrics they return from Reflow() (when it seems they
should).

Comment 52

[Karl Tomlinson (:karlt)]

Attachment: Get*Width for nsMathMLContainerFrame

Like attachment 305681 [details] [diff] [review] but using nsLayoutUtils::IntrinsicForContainer.
As tight horizontal bounds are likely to be provided (in the future) via a different method to what is used to obtain the intrinsic width of child frames, I'm using a nsMathMLContainerFrame::GetIntrinsicWidth method to do the logic.

There's also a ClearSavedChildMetrics() in nsMathMLmoFrame::Stretch that should have been in attachment 305651 [details] [diff] [review].

Comment 53

[Bill Gianopoulos [:WG9s]]

(In reply to comment #52)
> Created an attachment (id=305858) [details]
> Get*Width for nsMathMLContainerFrame

You probably already know this, but there is not enough horizontal margin and/or padding around mtables.  See tests 23 and 24 here:

http://www.mozilla.org/projects/mathml/demo/texvsmml.xhtml

Comment 54

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

+nsMathMLContainerFrame::GetIntrinsicWidth(nsIRenderingContext *aRenderingContext,
+                                          nsLayoutUtils::IntrinsicWidthType aType)

lose the intrinsic width type. We can add it back if we really need it later.

+    nscoord width = nsLayoutUtils::IntrinsicForContainer(aRenderingContext,
+							 childFrame, aType);

fix indent

Otherwise good.

Comment 55

[Karl Tomlinson (:karlt)]

Attachment: Get*Width for nsMathMLContainerFrame v1.3 [checked-in]

Made changes requested in comment 54.

Comment 56

[Karl Tomlinson (:karlt)]

(In reply to comment #53)
> See tests 23 and 24 here:
> 
> http://www.mozilla.org/projects/mathml/demo/texvsmml.xhtml

Attachment 305929 [details] [diff] is the base class implementation.

The elements, mroot, msqrt, mfenced, mfrac with slash, and mo each need their special implementation.

The problem in 23 is that the missing implementation for stretchy operators.

The problem in 24 is that the lspace and rspace attributes of operators haven't been included.  I'll attach a patch for this.

Comment 57

[Karl Tomlinson (:karlt)]

Attachment: include lspace and rspace [checked-in]

(This provides an mo element implementation, but it doesn't yet handle stretchy operators.)

Comment 58

[Karl Tomlinson (:karlt)]

+  nscoord width =
+    nsMathMLTokenFrame::GetIntrinsicWidth(aRenderingContext);

That can go on one line now.

Comment 59

[Karl Tomlinson (:karlt)]

Comment on attachment 305929 [details] [diff] [review]
Get*Width for nsMathMLContainerFrame v1.3 [checked-in]

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1204108974&maxdate=1204109172&who=karlt%2B%25karlt.net

Comment 60

[Karl Tomlinson (:karlt)]

Comment on attachment 305930 [details] [diff] [review]
include lspace and rspace [checked-in]

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1204109195&maxdate=1204109280&who=karlt%2B%25karlt.net

Comment 61

[Karl Tomlinson (:karlt)]

Still to do:

* mroot, msqrt, mfenced, bevelled mfrac, and stretchy and largeop mo elements
  require an upper bound on the stretchy character (nsMathMLChar) width.
  (Currently, calculated widths for these elements are too small.)

* A GetPrefTightInkHBounds method (with an implementation for text frames within
  inline blocks), or some other way to determine the amount that characters are
  expected to overflow their frames.  Depending on the fonts, there may not be
  many cases where this is important.

Comment 62

[SamyDagan]

Unexpectedly (to me) bug 407407 is dependent on this one (363240), and as consequence 407407 is now fixed. Cheers, Samy 

Comment 64

[Karl Tomlinson (:karlt)]

Attachment: nsMathMLChar::GetMaxWidth and stretchy <mo> GetIntrinsicWidth implementations

Also includes renaming of MAXSIZE_IS_EXPLICIT to MAXSIZE_IS_ABSOLUTE for
clarity, and rearrangement of nsOperatorFlags inspired by some unsigned/signed
compiler warnings related to 1<<31.

Comment 65

[Karl Tomlinson (:karlt)]

Comment on attachment 308802 [details] [diff] [review]
nsMathMLChar::GetMaxWidth and stretchy <mo> GetIntrinsicWidth implementations

+  class StretchEnumContext;

Looks like I should add 
+  friend class StretchEnumContext;

http://developer.mozilla.org/en/docs/C%2B%2B_Portability_Guide#Be_careful_with_inner_.28nested.29_classes

Comment 66

[Karl Tomlinson (:karlt)]

Attachment: remove some unused nsGlyphTable methods [checked-in]

Comment 67

[Karl Tomlinson (:karlt)]

Attachment: GetIntrinsicWidth for mroot, mfenced, and mfrac

Comment 68

[Karl Tomlinson (:karlt)]

Attachment: GetIntrinsicWidth for msqrt v0.1

(This is separate because I'll see if this can be done in a way that reuses code better.)

Comment 69

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

Comment on attachment 308802 [details] [diff] [review]
nsMathMLChar::GetMaxWidth and stretchy <mo> GetIntrinsicWidth implementations

+  PRBool         mTryVariants;
+  PRBool         mTryParts;

PRPackedBool

+  PRBool largeop = NS_STRETCH_LARGEOP & mStretchHint;
+  PRBool largeopOnly = largeop && !(NS_STRETCH_VARIABLE_MASK & mStretchHint);
+  PRBool maxWidth = NS_STRETCH_MAXWIDTH & mStretchHint;

Assigning non-0/1 values to PRBool is bad form, the existing code was right.

+  PRBool maxWidth = NS_STRETCH_MAXWIDTH & mStretchHint;
+  PRBool maxWidth = (NS_STRETCH_MAXWIDTH & aStretchHint);

Ditto

+  if (!maxWidth &&
+      ((targetSize <= 0) || 
+       (!largeop && 
+        ((isVertical && charSize >= targetSize) ||
+         IsSizeOK(aPresContext, charSize, targetSize, aStretchHint))))) {

This makes my head hurt. Can you break it up a bit with comments or locals to help?

Some documentation for what your overall strategy is would be helpful too.

It basically looks fine, although I only vaugely understand it, so this is partly a rubber stamp review, although I'm sure it's the best we can do.

Comment 70

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

+  if (0 < aMathMLChar->Length()) {
+

Bogus blank line

+nsMathMLmfracFrame::GetIntrinsicWidth(nsIRenderingContext* aRenderingContext)
...
+  // default rendering
+  return nsMathMLContainerFrame::GetIntrinsicWidth(aRenderingContext);  

Why this? Shouldn't vertical fractions take the max of the width of the numerator and denominator or something like that?

Can GetMaxCharWidth not share code with nsMathMLmfencedFrame::ReflowChar?

Comment 71

[Karl Tomlinson (:karlt)]

Comment on attachment 308811 [details] [diff] [review]
remove some unused nsGlyphTable methods [checked-in]

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205383800&maxdate=1205383824&who=karlt%2B%25karlt.net

Comment 72

[Karl Tomlinson (:karlt)]

Attachment: nsMathMLChar::GetMaxWidth and stretchy <mo> GetIntrinsicWidth implementations v1.1 [checked-in]

Addressed issues in comment 65 and comment 69

(In reply to comment #69)
> +  if (!maxWidth &&
> +      ((targetSize <= 0) || 
> +       (!largeop && 
> +        ((isVertical && charSize >= targetSize) ||
> +         IsSizeOK(aPresContext, charSize, targetSize, aStretchHint))))) {
> 
> This makes my head hurt. Can you break it up a bit with comments or locals to
> help?

In tidying this up I noticed the calculation of charSize and targetSize for horizontal stretches was using advance width rather than tight bounds, so I changed that to left and right bearing for consistency with nsMathMLContainerFrame::GetPreferredStretchSize and TryVariants and TryParts.

Comment 73

[Karl Tomlinson (:karlt)]

Attachment: GetIntrinsicWidth for mroot, mfenced, and mfrac v1.1 [checked-in]

(In reply to comment #70)
> +  if (0 < aMathMLChar->Length()) {
> +
> 
> Bogus blank line

Gone.

> +nsMathMLmfracFrame::GetIntrinsicWidth(nsIRenderingContext* aRenderingContext)
> ...
> +  // default rendering
> +  return nsMathMLContainerFrame::GetIntrinsicWidth(aRenderingContext);  
> 
> Why this? Shouldn't vertical fractions take the max of the width of the
> numerator and denominator or something like that?

It's similiar to nsMathMLmfracFrame::Reflow doing nsMathMLContainerFrame::Reflow.

nsMathMLContainerFrame::GetIntrinsicWidth uses this->Place() virtual in nsMathMLmfracFrame with parameter aPlaceOrigin = PR_FALSE to measure.

> Can GetMaxCharWidth not share code with nsMathMLmfencedFrame::ReflowChar?

Added GetCharSpacing.

Comment 74

[Karl Tomlinson (:karlt)]

Comment on attachment 309051 [details] [diff] [review]
nsMathMLChar::GetMaxWidth and stretchy <mo> GetIntrinsicWidth implementations v1.1 [checked-in]

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205467483&maxdate=1205467615&who=karlt%2B%25karlt.net

Comment 75

[Karl Tomlinson (:karlt)]

Comment on attachment 309223 [details] [diff] [review]
GetIntrinsicWidth for mroot, mfenced, and mfrac v1.1 [checked-in]

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205470920&maxdate=1205470964&who=karlt%2B%25karlt.net

Comment 77

[Karl Tomlinson (:karlt)]

Attachment: move Place() from nsIMathMLFrame to nsMathMLContainerFrame [checked-in]

I moved the Place method from nsIMathMLFrame to nsMathMLContainerFrame so
I could change it from an NS_IMETHOD to a virtual nsresult function, with the
intention of then being able to use Place in a pointer to member function.

Now I see that C++ pointers to member functions don't take bound member
functions (but virtual functions are always resolved according to the object
type), so I won't be able to use that approach.

However, these changes are probably still worth taking.  The definition of
nsMathMLmoFrame didn't have the correct NS_IMETHODIMP prefix, so this makes
things consistent.  Place is only called on the (parent) nsMathMLContainer
frames not on other nsIMathMLFrames, and this change makes that clearer.

It also updates the documentation for Place, makes a couple of virtual methods
(that don't need to be virtual) non-virtual, and makes some public methods
(that don't need to be public) protected.  (Some nsMathMLContainer methods are
still public just because they are currently used by derived classes on
objects of other derived classes across the class hierarchy.)

Comment 78

[Karl Tomlinson (:karlt)]

Attachment: GetIntrinsicWidth for msqrt using nsMathMLContainerFrame code with a functionoid

This uses a functionoid to behave like the kind of function pointer that I
want.  Compared to attachment 308814 [details] [diff] [review], more code is shared between
nsMathMLContainerFrame and nsMathMLmsqrtFrame, but 3 extra (small) functions
are added (and that doesn't include the constructors that I hope would be
inlined).

I guess an alternative might be to make nsMathMLContainerFrame::Place public
again and use a static function.  That wouldn't have any fewer functions but
could be fewer lines of code.

What do you think, Rob?

Comment 79

[Karl Tomlinson (:karlt)]

Attachment: GetIntrinsicWidth for msqrt using nsMathMLContainerFrame code with a virtual function [checked-in]

On Rob's advice, this uses a virtual function on the frame to indicate which Place method to use in nsMathMLContainerFrame::GetIntrinsicWidth.
It feels a little quirky but is less complicated than the previous patch, using  fewer lines of codes and removing one of the extra functions.

Comment 80

[Karl Tomlinson (:karlt)]

Comment on attachment 309929 [details] [diff] [review]
move Place() from nsIMathMLFrame to nsMathMLContainerFrame [checked-in]

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205815800&maxdate=1205815985&who=karlt%2B%25karlt.net

Comment 81

[Karl Tomlinson (:karlt)]

Comment on attachment 309929 [details] [diff] [review]
move Place() from nsIMathMLFrame to nsMathMLContainerFrame [checked-in]

Forgot to include the nsMathMLFrame.h changes in the patch.
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205817960&maxdate=1205817972&who=karlt%2B%25karlt.net

Comment 82

[Karl Tomlinson (:karlt)]

Comment on attachment 310153 [details] [diff] [review]
GetIntrinsicWidth for msqrt using nsMathMLContainerFrame code with a virtual function [checked-in]

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205816706&maxdate=1205816867&who=karlt%2B%25karlt.net

Comment 83

[Karl Tomlinson (:karlt)]

(In reply to comment #61)
> Still to do:
> * A GetPrefTightInkHBounds method (with an implementation for text frames
> within
>   inline blocks), or some other way to determine the amount that characters are
>   expected to overflow their frames.  Depending on the fonts, there may not be
>   many cases where this is important.

This hasn't been done yet, but the test case in this bug appears to render correctly.

There are test cases in bug 415413 that show problems with using the advance width only for intrinsic width calculation but considering glyph extents for frame placement, so I fix that issue there, possibly in a different way.
(There are enough patches in this bug.)

Marking this bug fixed.

I'll look at creating some reftests for this after the code freeze tomorrow.

Comment 84

[Karl Tomlinson (:karlt)]

Comment on attachment 309929 [details] [diff] [review]
move Place() from nsIMathMLFrame to nsMathMLContainerFrame [checked-in]

Missed nsMathMLmsupFrame.cpp too:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205819702&maxdate=1205819822&who=karlt%2B%25karlt.net

Comment 85

[Karl Tomlinson (:karlt)]

Attachment: correction for mroot width calculation [checked-in]

Comment 86

[Karl Tomlinson (:karlt)]

Checked in some reftests:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1206502682&maxdate=1206502911&who=karlt%2B%25karlt.net
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1206506760&maxdate=1206507121&who=karlt%2B%25karlt.net

The mroot test is currently commented out pending checkin of the above patch.

Comment 87

[Karl Tomlinson (:karlt)]

Comment on attachment 311725 [details] [diff] [review]
correction for mroot width calculation [checked-in]

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1206658020&maxdate=1206658021&who=karlt%2B%25karlt.net

Comment 88

[Karl Tomlinson (:karlt)]

(The above checkin for mroot won't make Beta 5.)

Added mroot and bevelled mfrac reftests:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1206658320&maxdate=1206658377&who=karlt%2B%25karlt.net