Bug 363240 - incorrect MathML <mtable> width and position (nsMathMLContainerFrames have zero preferred width)
 Summary: incorrect MathML width and position (nsMathMLContainerFrames have ze...
 Status: RESOLVED FIXED [dbaron-1.9:Rw] regression, testcase Core Components MathML (show other bugs) Trunk All All P2 normal (vote) mozilla1.9beta5 Karl Tomlinson (ni?:karlt) (view as bug list) 420420 reflow-refactor 324857 364359 400207 407407 412871 415413 416429 Show dependency tree / graph

Reported: 2006-12-08 19:13 PST by Jesse Ruderman
Modified: 2008-03-27 17:24 PDT (History)
23 users (show)
roc: blocking1.9+
karlt: in‑testsuite+
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Attachments
testcase (441 bytes, application/xhtml+xml)
2006-12-08 19:14 PST, Jesse Ruderman
no flags Details
implement GetMinWidth/GetPrefWidth with a Reflow (6.77 KB, patch)
2007-12-13 01:03 PST, Karl Tomlinson (ni?:karlt)
no flags Details | Diff | Review
implement GetMinWidth/GetPrefWidth with zero-available-width/unconstrained Reflow (18.66 KB, patch)
2008-01-16 15:42 PST, Karl Tomlinson (ni?:karlt)
no flags Details | Diff | Review
some TeX examples (1.84 KB, application/x-latex)
2008-01-24 14:46 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
some TeX examples (PDF) (34.12 KB, application/pdf)
2008-01-24 14:48 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
varying widths of stretchy operators in TeX (42.11 KB, application/pdf)
2008-01-25 15:17 PST, Karl Tomlinson (ni?:karlt)
no flags Details
varying widths of stretchy operators in TeX (source) (1.63 KB, text/plain)
2008-01-25 15:18 PST, Karl Tomlinson (ni?:karlt)
no flags Details
don't use frame origin offsets to store ascents (46.69 KB, patch)
2008-02-22 01:47 PST, Karl Tomlinson (ni?:karlt)
no flags Details | Diff | Review
GetPrefHBounds API (4.35 KB, patch)
2008-02-24 22:22 PST, Karl Tomlinson (ni?:karlt)
roc: review+
Details | Diff | Review
don't use frame origin offsets to store ascents v2 [checked-in] (40.21 KB, patch)
2008-02-25 18:28 PST, Karl Tomlinson (ni?:karlt)
roc: review+
roc: superreview+
Details | Diff | Review
Get*Width with GetPrefHBounds for nsMathMLContainerFrame (5.01 KB, patch)
2008-02-25 19:00 PST, Karl Tomlinson (ni?:karlt)
no flags Details | Diff | Review
Get*Width with GetPrefHBounds for nsMathMLContainerFrame (12.88 KB, patch)
2008-02-25 20:55 PST, Karl Tomlinson (ni?:karlt)
no flags Details | Diff | Review
Get*Width for nsMathMLContainerFrame (13.53 KB, patch)
2008-02-26 15:15 PST, Karl Tomlinson (ni?:karlt)
no flags Details | Diff | Review
Get*Width for nsMathMLContainerFrame v1.3 [checked-in] (13.38 KB, patch)
2008-02-26 21:03 PST, Karl Tomlinson (ni?:karlt)
roc: review+
roc: superreview+
Details | Diff | Review
include lspace and rspace [checked-in] (2.75 KB, patch)
2008-02-26 21:14 PST, Karl Tomlinson (ni?:karlt)
roc: review+
roc: superreview+
Details | Diff | Review
nsMathMLChar::GetMaxWidth and stretchy <mo> GetIntrinsicWidth implementations (58.59 KB, patch)
2008-03-11 21:45 PDT, Karl Tomlinson (ni?:karlt)
roc: review+
roc: superreview+
Details | Diff | Review
remove some unused nsGlyphTable methods [checked-in] (4.37 KB, patch)
2008-03-12 00:02 PDT, Karl Tomlinson (ni?:karlt)
roc: review+
roc: superreview+
Details | Diff | Review
GetIntrinsicWidth for mroot, mfenced, and mfrac (16.88 KB, patch)
2008-03-12 00:26 PDT, Karl Tomlinson (ni?:karlt)
no flags Details | Diff | Review
GetIntrinsicWidth for msqrt v0.1 (3.27 KB, patch)
2008-03-12 00:34 PDT, Karl Tomlinson (ni?:karlt)
no flags Details | Diff | Review
nsMathMLChar::GetMaxWidth and stretchy <mo> GetIntrinsicWidth implementations v1.1 [checked-in] (60.18 KB, patch)
2008-03-13 00:48 PDT, Karl Tomlinson (ni?:karlt)
no flags Details | Diff | Review
GetIntrinsicWidth for mroot, mfenced, and mfrac v1.1 [checked-in] (21.84 KB, patch)
2008-03-13 14:17 PDT, Karl Tomlinson (ni?:karlt)
roc: review+
roc: superreview+
Details | Diff | Review
move Place() from nsIMathMLFrame to nsMathMLContainerFrame [checked-in] (33.22 KB, patch)
2008-03-17 04:55 PDT, Karl Tomlinson (ni?:karlt)
roc: review+
roc: superreview+
Details | Diff | Review
GetIntrinsicWidth for msqrt using nsMathMLContainerFrame code with a functionoid (8.26 KB, patch)
2008-03-17 04:59 PDT, Karl Tomlinson (ni?:karlt)
no flags Details | Diff | Review
GetIntrinsicWidth for msqrt using nsMathMLContainerFrame code with a virtual function [checked-in] (7.06 KB, patch)
2008-03-17 20:46 PDT, Karl Tomlinson (ni?:karlt)
roc: review+
roc: superreview+
Details | Diff | Review
correction for mroot width calculation [checked-in] (1.09 KB, patch)
2008-03-25 20:35 PDT, Karl Tomlinson (ni?:karlt)
roc: review+
roc: superreview+
Details | Diff | Review

 Jesse Ruderman 2006-12-08 19:13:46 PST 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.) Jesse Ruderman 2006-12-08 19:14:37 PST Created attachment 248045 [details] testcase Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-10-24 01:18:43 PDT Almost certainly due to the fact that MathML does not implement GetPrefWidth/GetMinWidth. AndrewM 2007-10-24 20:20:11 PDT (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  Karl Tomlinson (ni?:karlt) 2007-12-04 01:24:03 PST 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 or 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. Karl Tomlinson (ni?:karlt) 2007-12-13 01:03:38 PST 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 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? Henri Sivonen (:hsivonen) 2007-12-31 05:05:47 PST 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. Karl Tomlinson (ni?:karlt) 2008-01-01 13:35:49 PST (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).  Bill Gianopoulos [:WG9s] 2008-01-13 07:37:28 PST 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. Karl Tomlinson (ni?:karlt) 2008-01-16 15:42:05 PST 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 exp&ApplyFunctiond where d 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 d 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. Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-01-16 18:12:47 PST + 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? David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-01-16 19:26:26 PST 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?) Karl Tomlinson (ni?:karlt) 2008-01-16 19:52:31 PST (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).  David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-01-16 20:30:56 PST 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? Karl Tomlinson (ni?:karlt) 2008-01-17 14:02:13 PST (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 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. 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. 0 2 n yn The placement of the numerator(/denominator) in the 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 ∑ 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.  David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-01-17 15:01:44 PST (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? distler 2008-01-17 15:06:53 PST >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 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-01-17 15:25:31 PST (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... Karl Tomlinson (ni?:karlt) 2008-01-17 15:44:59 PST (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: { a paragraph of text 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.  Karl Tomlinson (ni?:karlt) 2008-01-21 01:14:30 PST *** Bug 412940 has been marked as a duplicate of this bug. *** Karl Tomlinson (ni?:karlt) 2008-01-21 01:19:48 PST Changed summary from "MathML appears empty" to reflect the current symptoms: incorrect width and position (also bug 412871) David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-01-24 14:45:57 PST 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.) David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-01-24 14:46:41 PST Created attachment 299042 [details] some TeX examples David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-01-24 14:48:04 PST Created attachment 299044 [details] some TeX examples (PDF) PDF of attachment 299042 [details], generated with pdflatex. Karl Tomlinson (ni?:karlt) 2008-01-25 14:58:10 PST (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. Karl Tomlinson (ni?:karlt) 2008-01-25 14:59:11 PST 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). Karl Tomlinson (ni?:karlt) 2008-01-25 15:17:02 PST Created attachment 299309 [details] 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? Karl Tomlinson (ni?:karlt) 2008-01-25 15:18:32 PST Created attachment 299311 [details] varying widths of stretchy operators in TeX (source) distler 2008-01-27 15:05:04 PST Even though they don't involve 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 . 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.) Karl Tomlinson (ni?:karlt) 2008-01-27 16:31:02 PST (In reply to comment #28) Thanks for testing. > Even though they don't involve 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 . 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. distler 2008-01-27 17:44:54 PST >> Even though they don't involve 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.) Karl Tomlinson (ni?:karlt) 2008-01-27 18:43:24 PST (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.) distler 2008-01-27 19:46:54 PST Bug 414289 filed. And, since I was at it, I filed bug 414294 (an unrelated problem with stretchy delimiters) as well. Karl Tomlinson (ni?:karlt) 2008-01-29 19:20:13 PST (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.) Karl Tomlinson (ni?:karlt) 2008-02-01 21:56:26 PST 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"  David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-02-16 15:21:54 PST Is there a reasonable upper bound you could use to approximate for our current intrinsic width methods? Karl Tomlinson (ni?:karlt) 2008-02-17 14:25:46 PST (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)?  Karl Tomlinson (ni?:karlt) 2008-02-22 01:47:20 PST 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. Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-02-24 16:05:12 PST How about doing mChildReflowMetrics just as a property we set on child frames via nsIFrame::SetProperty? Karl Tomlinson (ni?:karlt) 2008-02-24 22:22:56 PST 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. Karl Tomlinson (ni?:karlt) 2008-02-24 22:29:57 PST 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. Karl Tomlinson (ni?:karlt) 2008-02-25 18:28:20 PST 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(). Karl Tomlinson (ni?:karlt) 2008-02-25 19:00:45 PST 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. Karl Tomlinson (ni?:karlt) 2008-02-25 19:02:40 PST Attachment 305661 [details] [diff] depends on attachment 305450 [details] [diff] [review] and attachment 305651 [details] [diff] [review]. Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-02-25 19:55:22 PST 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) Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-02-25 20:06:05 PST 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. Karl Tomlinson (ni?:karlt) 2008-02-25 20:49:38 PST 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 Karl Tomlinson (ni?:karlt) 2008-02-25 20:55:15 PST 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]) David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-02-25 23:01:20 PST 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? Karl Tomlinson (ni?:karlt) 2008-02-26 01:11:07 PST (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. Karl Tomlinson (ni?:karlt) 2008-02-26 03:40:28 PST 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). Karl Tomlinson (ni?:karlt) 2008-02-26 03:44:02 PST 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). Karl Tomlinson (ni?:karlt) 2008-02-26 15:15:17 PST 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]. Bill Gianopoulos [:WG9s] 2008-02-26 16:31:54 PST (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  Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-02-26 20:08:15 PST +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. Karl Tomlinson (ni?:karlt) 2008-02-26 21:03:20 PST Created attachment 305929 [details] [diff] [review] Get*Width for nsMathMLContainerFrame v1.3 [checked-in] Made changes requested in comment 54. Karl Tomlinson (ni?:karlt) 2008-02-26 21:09:09 PST (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. Karl Tomlinson (ni?:karlt) 2008-02-26 21:14:10 PST 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.) Karl Tomlinson (ni?:karlt) 2008-02-26 21:17:22 PST + nscoord width = + nsMathMLTokenFrame::GetIntrinsicWidth(aRenderingContext); That can go on one line now. Karl Tomlinson (ni?:karlt) 2008-02-27 03:05:32 PST 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 Karl Tomlinson (ni?:karlt) 2008-02-27 03:06:43 PST 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 Karl Tomlinson (ni?:karlt) 2008-02-27 03:27:21 PST 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. SamyDagan 2008-02-29 06:13:57 PST Unexpectedly (to me) bug 407407 is dependent on this one (363240), and as consequence 407407 is now fixed. Cheers, Samy  Karl Tomlinson (ni?:karlt) 2008-03-05 15:25:22 PST *** Bug 420011 has been marked as a duplicate of this bug. *** Karl Tomlinson (ni?:karlt) 2008-03-11 21:45:14 PDT Created attachment 308802 [details] [diff] [review] nsMathMLChar::GetMaxWidth and stretchy 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. Karl Tomlinson (ni?:karlt) 2008-03-11 23:59:55 PDT Comment on attachment 308802 [details] [diff] [review] nsMathMLChar::GetMaxWidth and stretchy 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 Karl Tomlinson (ni?:karlt) 2008-03-12 00:02:03 PDT Created attachment 308811 [details] [diff] [review] remove some unused nsGlyphTable methods [checked-in] Karl Tomlinson (ni?:karlt) 2008-03-12 00:26:44 PDT Created attachment 308813 [details] [diff] [review] GetIntrinsicWidth for mroot, mfenced, and mfrac Karl Tomlinson (ni?:karlt) 2008-03-12 00:34:04 PDT 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.) Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-03-12 20:20:26 PDT Comment on attachment 308802 [details] [diff] [review] nsMathMLChar::GetMaxWidth and stretchy 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. Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-03-12 20:32:36 PDT + 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? Karl Tomlinson (ni?:karlt) 2008-03-12 21:54:06 PDT 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 Karl Tomlinson (ni?:karlt) 2008-03-13 00:48:56 PDT Created attachment 309051 [details] [diff] [review] nsMathMLChar::GetMaxWidth and stretchy 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. Karl Tomlinson (ni?:karlt) 2008-03-13 14:17:59 PDT Created attachment 309223 [details] [diff] [review] 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. Karl Tomlinson (ni?:karlt) 2008-03-13 22:06:30 PDT Comment on attachment 309051 [details] [diff] [review] nsMathMLChar::GetMaxWidth and stretchy 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 Karl Tomlinson (ni?:karlt) 2008-03-13 22:35:26 PDT 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 Karl Tomlinson (ni?:karlt) 2008-03-13 23:06:23 PDT *** Bug 421726 has been marked as a duplicate of this bug. *** Karl Tomlinson (ni?:karlt) 2008-03-17 04:55:26 PDT 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.) Karl Tomlinson (ni?:karlt) 2008-03-17 04:59:56 PDT 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? Karl Tomlinson (ni?:karlt) 2008-03-17 20:46:42 PDT 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. Karl Tomlinson (ni?:karlt) 2008-03-17 22:07:58 PDT 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 Karl Tomlinson (ni?:karlt) 2008-03-17 22:29:58 PDT 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 Karl Tomlinson (ni?:karlt) 2008-03-17 22:33:02 PDT 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 Karl Tomlinson (ni?:karlt) 2008-03-17 22:43:09 PDT (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. Karl Tomlinson (ni?:karlt) 2008-03-17 23:08:09 PDT 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 Karl Tomlinson (ni?:karlt) 2008-03-25 20:35:03 PDT Created attachment 311725 [details] [diff] [review] correction for mroot width calculation [checked-in] Karl Tomlinson (ni?:karlt) 2008-03-25 21:54:39 PDT 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. Karl Tomlinson (ni?:karlt) 2008-03-27 15:54:16 PDT 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 Karl Tomlinson (ni?:karlt) 2008-03-27 17:24:08 PDT (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

 Note You need to log in before you can comment on or make changes to this bug.