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

RESOLVED FIXED in mozilla1.9beta5

Core
MathML
P2
normal
RESOLVED FIXED
11 years ago
10 years ago

Tracking

({regression, testcase})

Trunk
mozilla1.9beta5
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
 blocking1.9 + in-testsuite +

Attachments

(16 attachments, 9 obsolete attachments)

 441 bytes, application/xhtml+xml Details 18.66 KB, patch Details | Diff | Splinter Review 1.84 KB, application/x-latex Details 34.12 KB, application/pdf Details 42.11 KB, application/pdf Details 1.63 KB, text/plain Details 4.35 KB, patch roc : review+ Details | Diff | Splinter Review 40.21 KB, patch roc : review+ roc : superreview+ Details | Diff | Splinter Review 13.38 KB, patch roc : review+ roc : superreview+ Details | Diff | Splinter Review 2.75 KB, patch roc : review+ roc : superreview+ Details | Diff | Splinter Review 4.37 KB, patch roc : review+ roc : superreview+ Details | Diff | Splinter Review 60.18 KB, patch Details | Diff | Splinter Review 21.84 KB, patch roc : review+ roc : superreview+ Details | Diff | Splinter Review 33.22 KB, patch roc : review+ roc : superreview+ Details | Diff | Splinter Review 7.06 KB, patch roc : review+ roc : superreview+ Details | Diff | Splinter Review 1.09 KB, patch roc : review+ roc : superreview+ Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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.)
(Reporter)

Comment 1

11 years ago
Created attachment 248045 [details]
testcase
(Reporter)

Updated

11 years ago
Flags: blocking1.9?

Updated

11 years ago
Flags: blocking1.9? → blocking1.9+

Updated

10 years ago
Whiteboard: [dbaron-1.9:Rw]

Comment 2

10 years ago
Almost certainly due to the fact that MathML does not implement GetPrefWidth/GetMinWidth.
Assignee: rbs → mozbugz
(Assignee)

Updated

10 years ago
Depends on: 400207

Comment 3

10 years ago
(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

(Assignee)

Updated

10 years ago
OS: Mac OS X → All
Priority: -- → P2

Updated

10 years ago
Priority: P2 → P1
(Assignee)

10 years ago
Blocks: 324857
(Assignee)

Updated

10 years ago
Blocks: 400207
No longer depends on: 400207
(Assignee)

Comment 4

10 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Target Milestone: --- → mozilla1.9 M11
(Assignee)

Comment 5

10 years ago
Created attachment 292927 [details] [diff] [review]
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

* 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

10 years ago
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.
(Assignee)

Comment 7

10 years ago
(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).


Summary: MathML <mtable> appears empty → MathML <mtable> appears empty (nsMathMLContainerFrames have zero preferred width)

Comment 8

10 years ago
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.
(Assignee)

Comment 9

10 years ago
Created attachment 297447 [details] [diff] [review]
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.
Attachment #297447 - Flags: review?(roc)
(Assignee)

Updated

10 years ago
QA Contact: ian → mathml

Comment 10

10 years ago
+                             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?

Updated

10 years ago
Attachment #297447 - Flags: superreview?(dbaron)

Comment 11

10 years ago
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?)
(Assignee)

Comment 12

10 years ago
(In reply to comment #11)
> (I though there was another bug on implementing intrinsic widths for MathML

bug 400207 comment 3 ?

Or "similar refactoring decisions as for nsLeafFrame, but the inline intrinsic
width API may be more involved here"?

> 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

* 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

10 years ago
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?
(Assignee)

Comment 14

10 years ago
(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 [itex] 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 [itex] direct parent.  But <mrow> and other elements do not do line-breaking.

Currently on 1.9, [itex] 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

10 years ago
(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

10 years ago
>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

10 years ago
(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...
(Assignee)

Comment 18

10 years ago
(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.

(Assignee)

10 years ago
Blocks: 412871
(Assignee)

Updated

10 years ago
Attachment #292927 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Duplicate of this bug: 412940
(Assignee)

Comment 20

10 years ago
Changed summary from "MathML <mtable> appears empty" to reflect the current symptoms: incorrect width and position (also bug 412871)
Summary: MathML <mtable> appears empty (nsMathMLContainerFrames have zero preferred width) → incorrect MathML <mtable> width and position (nsMathMLContainerFrames have zero preferred width)

Comment 21

10 years ago
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

10 years ago
Created attachment 299042 [details]
some TeX examples

Comment 23

10 years ago
Created attachment 299044 [details]
some TeX examples (PDF)

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

Comment 24

10 years ago
(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 &sum;, &int;, /, 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.
(Assignee)

Comment 25

10 years ago
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).
(Assignee)

Comment 26

10 years ago
Created attachment 299309 [details]
varying widths of stretchy operators in TeX

> 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 &sum; 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?
(Assignee)

Comment 27

10 years ago
Created attachment 299311 [details]
varying widths of stretchy operators in TeX (source)

Comment 28

10 years ago
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.)
(Assignee)

Comment 29

10 years ago
(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

10 years ago
>> 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.)
(Assignee)

Comment 31

10 years ago
(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

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

Comment 33

10 years ago
(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.)
Whiteboard: [dbaron-1.9:Rw] → [dbaron-1.9:Rw][swag:40]
Target Milestone: mozilla1.9beta3 → mozilla1.9beta4
(Assignee)

Updated

10 years ago
Whiteboard: [dbaron-1.9:Rw][swag:40] → [dbaron-1.9:Rw][swag:40d]
(Assignee)

Comment 34

10 years ago
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.

Keywords: relnote
(Assignee)

10 years ago
Blocks: 415413
(Assignee)

10 years ago
Blocks: 364359

Updated

10 years ago
Hardware: Macintosh → All
(Assignee)

10 years ago
Blocks: 416429

Comment 35

10 years ago
Is there a reasonable upper bound you could use to approximate for our current intrinsic width methods?
(Assignee)

Comment 36

10 years ago
(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)?

(Assignee)

Comment 37

10 years ago
Created attachment 304932 [details] [diff] [review]
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.
Attachment #304932 - Flags: review?(roc)

Comment 38

10 years ago
How about doing mChildReflowMetrics just as a property we set on child frames via nsIFrame::SetProperty?
(Assignee)

Comment 39

10 years ago
Created attachment 305450 [details] [diff] [review]
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.
Attachment #305450 - Flags: superreview?(dbaron)
Attachment #305450 - Flags: review?(roc)
(Assignee)

Comment 40

10 years ago
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.
(Assignee)

Comment 41

10 years ago
Created attachment 305651 [details] [diff] [review]
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().
Attachment #304932 - Attachment is obsolete: true
Attachment #305651 - Flags: review?(roc)
Attachment #304932 - Flags: review?(roc)
(Assignee)

Comment 42

10 years ago
Created attachment 305661 [details] [diff] [review]
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.
Attachment #305661 - Flags: review?(roc)
(Assignee)

Comment 43

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

Updated

10 years ago
Attachment #297447 - Flags: superreview?(dbaron)
Attachment #297447 - Flags: review?(roc)

Comment 44

10 years ago
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)
Attachment #305651 - Flags: superreview+
Attachment #305651 - Flags: review?(roc)
Attachment #305651 - Flags: review+

Comment 45

10 years ago
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.
Attachment #305450 - Flags: review?(roc) → review+
(Assignee)

Comment 46

10 years ago
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
Attachment #305651 - Attachment description: don't use frame origin offsets to store ascents v2 → don't use frame origin offsets to store ascents v2 [checked-in]
(Assignee)

Comment 47

10 years ago
Created attachment 305681 [details] [diff] [review]
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])
Attachment #305661 - Attachment is obsolete: true
Attachment #305681 - Flags: review?(roc)
Attachment #305661 - Flags: review?(roc)

Comment 48

10 years ago
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?
(Assignee)

Comment 49

10 years ago
(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.
(Assignee)

Comment 50

10 years ago
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
be included when there is a border).
Attachment #305450 - Flags: superreview?(dbaron)
(Assignee)

Comment 51

10 years ago
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).
Attachment #305681 - Flags: review?(roc)
(Assignee)

Comment 52

10 years ago
Created attachment 305858 [details] [diff] [review]
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].
Attachment #305681 - Attachment is obsolete: true
Attachment #305858 - Flags: review?(roc)

Comment 53

10 years ago
(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

10 years ago
+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.
(Assignee)

Comment 55

10 years ago
Created attachment 305929 [details] [diff] [review]
Get*Width for nsMathMLContainerFrame v1.3 [checked-in]

Made changes requested in comment 54.
Attachment #305858 - Attachment is obsolete: true
Attachment #305929 - Flags: review?(roc)
Attachment #305858 - Flags: review?(roc)
(Assignee)

Comment 56

10 years ago
(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.
(Assignee)

Comment 57

10 years ago
Created attachment 305930 [details] [diff] [review]
include lspace and rspace [checked-in]

(This provides an mo element implementation, but it doesn't yet handle stretchy operators.)
Attachment #305930 - Flags: review?(roc)
(Assignee)

Comment 58

10 years ago
+  nscoord width =
+    nsMathMLTokenFrame::GetIntrinsicWidth(aRenderingContext);

That can go on one line now.

Updated

10 years ago
Attachment #305929 - Flags: superreview+
Attachment #305929 - Flags: review?(roc)
Attachment #305929 - Flags: review+

Updated

10 years ago
Attachment #305930 - Flags: superreview+
Attachment #305930 - Flags: review?(roc)
Attachment #305930 - Flags: review+
(Assignee)

Comment 59

10 years ago
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
Attachment #305929 - Attachment description: Get*Width for nsMathMLContainerFrame v1.3 → Get*Width for nsMathMLContainerFrame v1.3 [checked-in]
(Assignee)

Comment 60

10 years ago
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
Attachment #305930 - Attachment description: include lspace and rspace → include lspace and rspace [checked-in]
(Assignee)

Comment 61

10 years ago
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.
Target Milestone: mozilla1.9beta4 → mozilla1.9

Updated

10 years ago
Flags: blocking1.9+
Priority: P1 → P2

Comment 62

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

10 years ago
Blocks: 407407
(Reporter)

Updated

10 years ago
Depends on: 420420
(Assignee)

Updated

10 years ago
Duplicate of this bug: 420011

Updated

10 years ago
Flags: tracking1.9+
(Assignee)

Comment 64

10 years ago
Created attachment 308802 [details] [diff] [review]
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.
Attachment #308802 - Flags: review?(roc)
(Assignee)

Comment 65

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

+  class StretchEnumContext;

+  friend class StretchEnumContext;

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

Comment 66

10 years ago
Created attachment 308811 [details] [diff] [review]
remove some unused nsGlyphTable methods [checked-in]
Attachment #308811 - Flags: review?(roc)
(Assignee)

Comment 67

10 years ago
Created attachment 308813 [details] [diff] [review]
GetIntrinsicWidth for mroot, mfenced, and mfrac
Attachment #308813 - Flags: review?(roc)
(Assignee)

Comment 68

10 years ago
Created attachment 308814 [details] [diff] [review]
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

10 years ago
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.
Attachment #308802 - Flags: superreview+
Attachment #308802 - Flags: review?(roc)
Attachment #308802 - Flags: review+

Comment 70

10 years ago
+  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?

Updated

10 years ago
Attachment #308811 - Flags: superreview+
Attachment #308811 - Flags: review?(roc)
Attachment #308811 - Flags: review+
(Assignee)

Comment 71

10 years ago
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
Attachment #308811 - Attachment description: remove some unused nsGlyphTable methods → remove some unused nsGlyphTable methods [checked-in]
(Assignee)

Comment 72

10 years ago
Created attachment 309051 [details] [diff] [review]
nsMathMLChar::GetMaxWidth and stretchy <mo> GetIntrinsicWidth implementations v1.1 [checked-in]

Addressed issues in comment 65 and 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.
Attachment #308802 - Attachment is obsolete: true
(Assignee)

Comment 73

10 years ago
Created attachment 309223 [details] [diff] [review]
GetIntrinsicWidth for mroot, mfenced, and mfrac v1.1 [checked-in]

> +  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.
Attachment #308813 - Attachment is obsolete: true
Attachment #309223 - Flags: review?(roc)
Attachment #308813 - Flags: review?(roc)

Updated

10 years ago
Attachment #309223 - Flags: superreview+
Attachment #309223 - Flags: review?(roc)
Attachment #309223 - Flags: review+
(Assignee)

Comment 74

10 years ago
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
Attachment #309051 - Attachment description: nsMathMLChar::GetMaxWidth and stretchy <mo> GetIntrinsicWidth implementations v1.1 → nsMathMLChar::GetMaxWidth and stretchy <mo> GetIntrinsicWidth implementations v1.1 [checked-in]
(Assignee)

Comment 75

10 years ago
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
Attachment #309223 - Attachment description: GetIntrinsicWidth for mroot, mfenced, and mfrac v1.1 → GetIntrinsicWidth for mroot, mfenced, and mfrac v1.1 [checked-in]
(Assignee)

Updated

10 years ago
Duplicate of this bug: 421726
(Assignee)

Comment 77

10 years ago
Created attachment 309929 [details] [diff] [review]
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.)
Attachment #309929 - Flags: review?(roc)
(Assignee)

Comment 78

10 years ago
Created attachment 309931 [details] [diff] [review]
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?
Attachment #309931 - Flags: review?(roc)

Updated

10 years ago
Attachment #309929 - Flags: superreview+
Attachment #309929 - Flags: review?(roc)
Attachment #309929 - Flags: review+
(Assignee)

Comment 79

10 years ago
Created attachment 310153 [details] [diff] [review]
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.
Attachment #309931 - Attachment is obsolete: true
Attachment #310153 - Flags: review?(roc)
Attachment #309931 - Flags: review?(roc)

Updated

10 years ago
Attachment #310153 - Flags: superreview+
Attachment #310153 - Flags: review?(roc)
Attachment #310153 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #308814 - Attachment is obsolete: true
(Assignee)

Comment 80

10 years ago
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
Attachment #309929 - Attachment description: mover Place() from nsIMathMLFrame to nsMathMLContainerFrame → move Place() from nsIMathMLFrame to nsMathMLContainerFrame [checked-in]
(Assignee)

Comment 81

10 years ago
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
(Assignee)

Comment 82

10 years ago
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
Attachment #310153 - Attachment description: GetIntrinsicWidth for msqrt using nsMathMLContainerFrame code with a virtual function → GetIntrinsicWidth for msqrt using nsMathMLContainerFrame code with a virtual function [checked-in]
(Assignee)

Comment 83

10 years ago
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Keywords: relnote
Resolution: --- → FIXED
Whiteboard: [dbaron-1.9:Rw][swag:40d] → [dbaron-1.9:Rw]
Target Milestone: mozilla1.9 → mozilla1.9beta5
(Assignee)

Comment 84

10 years ago
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
(Assignee)

Comment 85

10 years ago
Created attachment 311725 [details] [diff] [review]
correction for mroot width calculation [checked-in]
Attachment #311725 - Flags: review?(roc)
(Assignee)

Comment 86

10 years ago
Checked in some reftests:

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

Updated

10 years ago
Attachment #311725 - Flags: superreview+
Attachment #311725 - Flags: review?(roc)
Attachment #311725 - Flags: review+
(Assignee)

Comment 87

10 years ago
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
Attachment #311725 - Attachment description: correction for mroot width calculation → correction for mroot width calculation [checked-in]
(Assignee)

Comment 88

10 years ago
(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
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.