This was reported by MathJax's author when writing the mhchem extension. See the testcase. The arrow is stretched if the <mrow> is removed or if another child is added to the <mrow>. The problem was is present in Iceweasel (Firefox 3.5). I'm wondering if it is a regression of bug 21479.

I'm trying to think about a way to fix bugs that are blocking the native MathML to be enabled in MathJax again. Probably the most serious one is <mlabeledtr> and the MathJax team plans to add a workaround in MathJax itself. MathJax could also make its Web fonts available to Gecko and solve the font issue (some code needs to written on MathJax side to make this work). I guess we can ignore linebreaking (it is only enabled in MathJax's SVG/HTML-CSS rendering via config options and that could certainly be the same for MathML). The present bug is a problem for the mhchem extension but probably other use cases. I don't think I have time to look at it, so probably the simplest solution would be to remove some of the work done in bug 21479 and change nsMathMLContainerFrame::TransmitAutomaticDataForMrowLikeElement to never consider mrow-like elements to be embellished operators until we find a better fix. I guess that would solve the stretchy problem.

Mass change: setting priority to 3 for bugs preventing Gecko's Native MathML to be enabled by default in MathJax.

One issue here is that, because of bug 236963, the embellished mrow will not stretch. So a better workaround could be to verify whether the parent of the mrow-like element is a math/mtd element and don't mark it as embellished op in that case. Adding a non space-like child to the mrow seems to make the arrow stretch correctly, so I guess this less drastic workaround will still make the testcase work correctly. I don't know whether there are other cases to consider, though.

This new patch uses the workaround suggested in comment 4. It fixes the testcase and passes the corresponding new reftest while still avoiding regressions in the embellished op reftests or MathML Acid3 tests.

Comment on attachment 734245 [details] [diff] [review] Reftest I agree with your conclusion of http://lists.w3.org/Archives/Public/www-math/2012Mar/0047.html and so yes, the arrow should stretch to the width of the "direct sub-expressions" of the munderover.

Comment on attachment 734244 [details] [diff] [review] Patch V2 I don't want to add workarounds like this, at least without better understanding what is going wrong. These kind of special cases scattered around make the code harder to understand and more unpredictable. See the next testcase I'll attach for why I doubt the issue is directly due to bug 236963 and the math parent element. I wonder whether there could be something wrong with the placeOrigin/parentWillFireStretch/stretchAll logic in nsMathMLContainerFrame::FinalizeReflow() perhaps?

Here the arrow stretches as expected, even though the top-level embellished operator is a child of <math>.

Here is testcase where the embellished operator is not a child of math and the stretching is incorrect, but perhaps this is a different issue. It looks like the munderover is doing its horizontal stretching using the size of the parent mrow. The parent mrow dimensions should only be used for vertical stretching. There seem to be general issues with keeping track of the direction of stretch and dimensions for the stretch. I suspect a full solution will involve multiple stretches, rather than saving the stretch for the topmost embellished operator.

I have this test case on my desktop but I don't really remember what I tried. The only thing that I noticed is that there might be distinction between the "core" and the "base": http://dxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLContainerFrame.cpp#l224

Sealing assign. I have a mostly working patch which needs a little more testing.

Stretching is now a two phase operation. Horizontal stretch is considered first, then vertical stretch. Vertical stretch metrics are determined by the core frame, horizontal stretches by the outermost mover etc. element (and failing that the core frame). This solves the problems of attachment 561157 [details] and attachment 744057 [details] (which breaks when using a msup rather than munderover). The rules when to consider the sizes of other elements have been tightened, resolving attachment 744059 [details].

Suggestions on additional tests are welcome.

How does this patch affect the results of the existing tests (especially the embellished-op tests) and of the MathML Acid3 tests?

Try run: https://tbpl.mozilla.org/?tree=Try&rev=9ff7d29355bf Other than some platform specific failures for some of the new tests I have added (stretchy-mfenced-*c), the other tests all pass. MathML Acid3: Score/failed test list of a patched nightly match that of unpatched nightly. The mtd related stretchy tests still fail. I'm looking into a test 63 (operator dictionary ; vertical stretchy operators) failure. Left/right angle brackets apparently don't stretch.

(In reply to James Kitchener (:jkitch) from comment #17) > MathML Acid3: Score/failed test list of a patched nightly match that of > unpatched nightly. The mtd related stretchy tests still fail. This is bug 236963. I'm looking > into a test 63 (operator dictionary ; vertical stretchy operators) failure. > Left/right angle brackets apparently don't stretch. I guess it is due to the change in bug 960115 (removal of the scale correction). I wonder if the torture test should either not include the angle brackets or provide MATH fonts to allow them to stretch to an arbitrary size.

(In reply to James Kitchener (:jkitch) from comment #17) > unpatched nightly. The mtd related stretchy tests still fail. I'm looking > into a test 63 (operator dictionary ; vertical stretchy operators) failure. > Left/right angle brackets apparently don't stretch. I don't see this issue. Perhaps it is related to a specific font?

(In reply to Bill Gianopoulos [:WG9s] from comment #19) > (In reply to James Kitchener (:jkitch) from comment #17) > > > unpatched nightly. The mtd related stretchy tests still fail. I'm looking > > into a test 63 (operator dictionary ; vertical stretchy operators) failure. > > Left/right angle brackets apparently don't stretch. > > I don't see this issue. Perhaps it is related to a specific font? Or, perhaps one of the other patches in Fred's queue "fixes" it!

Comment on attachment 8416349 [details] [diff] [review] Part 1: stretch changes I haven't checked the details, but that looks good to me. I'm deferring to Karl for the review.

I'm deferring some of the mfenced tests for bug 670334, where I will make an attempt at unifying the behaviour of mfenced and its equivalent notation.

GetPreferredStretchSize and Stretch are now called twice, once for horizontal and once for vertical stretches. The horizontal call records the horizontal metrics and returns without applying them. The vertical call copies the stored metrics into its desired containerSize and finishes the stretch. The final MathMLCharacter stretch is performed using NS_STRETCH_DIRECTION_DEFAULT, allowing the character to stretch in its natural direction using the metrics determined earlier. (If the metrics indicate a stretch that isn't the default direction, those metrics are ignored). This should make it easier to implement diagonal stretches in the future. Changes since the comment 14 patch: Stretch direction and the order of calling is now enforced by asserts and nsMathMLContainerFrame::Reflow's stretch procedure has been somewhat streamlined.

Comment on attachment 8420952 [details] [diff] [review] Part 1: stretch changes Currently operators only support one stretch direction. Also, each frame type in an embellished operator subtree should only contribute metrics to and cause stretches in one direction (depending on NS_MATHML_WILL_STRETCH_ALL_CHILDREN_VERTICALLY or _HORIZONTALLY). The bug in attachment 561157 [details], attachment 744059 [details], and attachment 824680 [details] seems to be that sometimes the stretch metrics used are obtained from a subtree that doesn't stretch in the same direction as the operator. Would it be possible to fix this bug more simply by obtaining stretch metrics in embellished frames only if they want a stretch in the same direction as that supported by the operator? That approach is similar to this patch's change to GetPreferredStretchSize(), but I'm guessing it should be possible to do this without obtaining metrics in both directions and to use the depth-recursion of nsMathMLContainerFrame::Stretch() instead of adding another depth-recursion with OutermostHorizontallyStretchedFrame(). I wonder whether the logic at http://hg.mozilla.org/mozilla-central/annotate/41a54c8add09/layout/mathml/nsMathMLContainerFrame.cpp#l330 can be adjusted to call GetPreferredStretchSize() on the first (outermost) frame of the embellished subtree that should contribute metrics to (and in the same direction as) the operator stretch, and use childSize.mBoundingMetrics if no such frame is found. (It already handles the case of Stretch() called with the wrong direction.) Perhaps the Stretch() call on the baseFrame there should not pass mEmbellishData.direction for aStretchDirection until the first such frame is found; mEmbellishData.direction == aStretchDirection could be an indicator that stretch metrics have already been found. Perhaps a change in the supported stretch direction while descending embellishing frames should reset the value passed for aStretchDirection so that new metrics will be found, for the sake of the horizontal case in attachment 824680 [details].

This patch tightens up the requirements for the inclusion of embellishments for metrics calculation by only considering sibling frames if the parent stretch-all-children direction matches the direction of the underlying stretchy operator. It also forces a recalculation of metrics in the event of a horizontal stretchy operator in a vertical direction stretch. The opposite case of a vertical stretchy element in a horizontal direction stretch isn't covered as it cannot occur in the present codebase (horizontal stretches are initiated using the DEFAULT direction).

Some of the tests have been tidied up and there are a few new ones. Nothing exciting.

The great news is that the new patch does NOT interfere with the fix to bug 969906 the way the old patch did.

(In reply to Karl Tomlinson (:karlt) from comment #24) > Perhaps a change in the supported stretch direction while descending > embellishing frames should reset the value passed for aStretchDirection so > that new metrics will be found, for the sake of the horizontal case in > attachment 824680 [details]. Sorry, my suggestion here was not helpful. In the first "Horizontal" testcase in attachment 824680 [details] the arrow is not embellished by the outermost mover because it is not a descendant of the first argument of the outermost mover. Also its stretching looks correct. It is just the placement of the embellishments from the mrow that are incorrect. I wrote this example to prove my suggestion wrong because the arrow should stretch to the maximum size of the embellishments from munder and mover, even with a vertical-stretching mrow in the embellishment hierarchy. I don't know why Firefox (31) is stretching only to the size of the inner embellishment.

Comment on attachment 8470795 [details] [diff] [review] Part 1: stretch changes (In reply to James Kitchener from comment #25) > Created attachment 8470795 [details] [diff] [review] > Part 1: stretch changes > > This patch tightens up the requirements for the inclusion of embellishments > for metrics calculation by only considering sibling frames if the parent > stretch-all-children direction matches the direction of the underlying > stretchy operator. > // compute a size that doesn't include embellishements > bool stretchAll = >- NS_MATHML_WILL_STRETCH_ALL_CHILDREN_VERTICALLY(mPresentationData.flags) || >- NS_MATHML_WILL_STRETCH_ALL_CHILDREN_HORIZONTALLY(mPresentationData.flags); >+ (NS_MATHML_WILL_STRETCH_ALL_CHILDREN_VERTICALLY(mPresentationData.flags) && >+ aStretchDirection == NS_STRETCH_DIRECTION_VERTICAL) || >+ (NS_MATHML_WILL_STRETCH_ALL_CHILDREN_HORIZONTALLY(mPresentationData.flags) && >+ aStretchDirection == NS_STRETCH_DIRECTION_HORIZONTAL); The existing comment here is perhaps misleading, because we *do* want to include child sibling embellishments iff their container frame stretches all children in the direction indicated by the embellished operator. Your change in behavior here is good, I think. Can you write this using :? and assert that aStretchDirection is either horizontal of vertical, please? That would be a little simpler, and I think knowing that there is a direction here would help in understanding the code. Something like: bool stretchAll = aStretchDirection == NS_STRETCH_DIRECTION_VERTICAL ? NS_MATHML_WILL_STRETCH_ALL_CHILDREN_VERTICALLY(mPresentationData.flags) : NS_MATHML_WILL_STRETCH_ALL_CHILDREN_HORIZONTALLY(mPresentationData.flags); > if (NS_MATHML_IS_EMBELLISH_OPERATOR(embellishData.flags) && > embellishData.direction == aStretchDirection && >- presentationData.baseFrame) { >- // embellishements are not included, only consider the inner first child itself >- // XXXkt Does that mean the core descendent frame should be used >- // instead of the base child? >- nsIMathMLFrame* mathMLchildFrame = do_QueryFrame(presentationData.baseFrame); >+ (!NS_MATHML_WILL_STRETCH_ALL_CHILDREN_HORIZONTALLY(presentationData.flags) || >+ aStretchDirection != NS_STRETCH_DIRECTION_HORIZONTAL)) { >+ // embellishments are not included, only consider the inner first child itself >+ nsIMathMLFrame* mathMLchildFrame = do_QueryFrame(embellishData.coreFrame); > if (mathMLchildFrame) { > mathMLFrame = mathMLchildFrame; > } > } It's more complicated than just switching from baseFrame to coreFrame because again we do want to include embellishments in descendant baseFrames iff the embellishing container frame stretches all children in the direction of the underlying operator. Consider attachment 8474344 [details] where the embellishments from the both the inner and outer munder/mover should be considered in the core operator stretch. I think a recursive call to GetPreferredStretchSize() is needed. (That would likely be most simply done here, even if perhaps it could be worked into Stretch().) That need not be done in this patch, but please leave baseFrame for now until we determine how to do this properly. Why is the restriction to vertically stretching container frame and direction parameter added here? Can that be discussed as a subsequent separate patch with examples? > It also forces a recalculation of metrics in the event of a horizontal > stretchy operator in a vertical direction stretch. The opposite case of a > vertical stretchy element in a horizontal direction stretch isn't covered as > it cannot occur in the present codebase (horizontal stretches are initiated > using the DEFAULT direction). Shouldn't the existing "aStretchDirection != mEmbellishData.direction" test in Stretch() handle this case? The stretching code is very hard to understand already as it is. I would like it to be as simple as possible, but no simpler. It feels like adding aHorizStretchRecalc is a bolt-on fix without addressing the core problem, making the code even harder to follow. Is the "aStretchDirection != NS_STRETCH_DIRECTION_DEFAULT" test getting in the way? If so, can NS_STRETCH_DIRECTION_DEFAULT be removed/avoided? > } > else { > GetPreferredStretchSize(aRenderingContext, > stretchAll ? STRETCH_CONSIDER_EMBELLISHMENTS : 0, > mEmbellishData.direction, containerSize); > } > } > >+ if (NS_MATHML_WILL_STRETCH_ALL_CHILDREN_HORIZONTALLY(mPresentationData.flags) && >+ mEmbellishData.direction == NS_STRETCH_DIRECTION_HORIZONTAL && >+ aHorizStretchRecalc) { >+ // under/over frames are special in that horizontal stretches are >+ // always included when determining container size, however the >+ // previous call to GetPreferredStretchSize may not have been aware >+ // of the presence of such a frame. In this event, we retrigger >+ // the size calculations. >+ GetPreferredStretchSize(aRenderingContext, 0, >+ mEmbellishData.direction, containerSize); >+ // Only the outermost under/over frame in an embellished operator >+ // gets special treatment. >+ aHorizStretchRecalc = false; >+ } I don't understand why "horizontal stretches are always included when determining container size". That's referring to one of the changes to GetPreferredStretchSize() IIUC, but I don't know why horizontal is treated differently from vertical there. It looks like there could be two consecutive GetPreferredStretchSize() calls in the quoted code here. These seem to serve similar purposes and I'd like to know why the first is not sufficient. (In reply to James Kitchener from comment #27) > There is one caveat. The attached patch always triggers a stretch of an > embellished operator in its natural direction. If no stretch is intended > (for example a vertical stretching character within a munderover frame), the > stretch is called with the unstretched metrics. Is that a change in behavior? (I thought that all stretchy operators already received a Stretch().)

Currently (without patches here) GetPreferredStretchSize() mostly looks designed to get the stretch size for descendant operators that stretch in the direction indicated by the container frame. In this case, the aStretchDirection parameter matches the direction indicated by the container. However, it is also used to obtain (currently incorrect) sizes for stretching of embellished operators that stretch in the opposite direction to the container frame. In these cases, the first call in nsMathMLContainerFrame::Stretch() and the call in FinalizeReflow(), the aStretchDirection parameter is set to the direction indicated by the operator. Currently GetPreferredStretchSize() is determining how to accumulate child metrics based on the direction of the frame (i.e. mPresentationData.flags). Should it instead use aStretchDirection? http://hg.mozilla.org/mozilla-central/annotate/41a54c8add09/layout/mathml/nsMathMLContainerFrame.cpp#l250

Your suggestions have further simplified that patch. (In reply to Karl Tomlinson (:karlt) from comment #30) > It's more complicated than just switching from baseFrame to coreFrame because > again we do want to include embellishments in descendant baseFrames iff the > embellishing container frame stretches all children in the direction of the > underlying operator. Consider attachment 8474344 [details] where the > embellishments > from the both the inner and outer munder/mover should be considered in the > core operator stretch. I think a recursive call to GetPreferredStretchSize() > is needed. (That would likely be most simply done here, even if perhaps it > could be worked into Stretch().) That need not be done in this patch, but > please leave baseFrame for now until we determine how to do this properly. This change has been reverted. The attached patch passes your testcase, but can fail in certain situations involving mover etc. and a vertical operator. The relevant test is still included, but has been marked as fails. > > Why is the restriction to vertically stretching container frame and direction > parameter added here? Can that be discussed as a subsequent separate patch > with examples? > > > It also forces a recalculation of metrics in the event of a horizontal > > stretchy operator in a vertical direction stretch. The opposite case of a > > vertical stretchy element in a horizontal direction stretch isn't covered as > > it cannot occur in the present codebase (horizontal stretches are initiated > > using the DEFAULT direction). > > Shouldn't the existing "aStretchDirection != mEmbellishData.direction" test > in > Stretch() handle this case? > > The stretching code is very hard to understand already as it is. I would > like > it to be as simple as possible, but no simpler. It feels like adding > aHorizStretchRecalc is a bolt-on fix without addressing the core problem, > making the code even harder to follow. The attached patch prevents (well asserts) a default direction from being passed. This removes both the need for the bool and the second GetPreferredStretchSize call. The consequence of this is that a vertical direction needs to be initially specified if there is the potential for an embedded munderover frame, at which point the code will correct the direction to horizontal as needed. > > (In reply to James Kitchener from comment #27) > > There is one caveat. The attached patch always triggers a stretch of an > > embellished operator in its natural direction. If no stretch is intended > > (for example a vertical stretching character within a munderover frame), the > > stretch is called with the unstretched metrics. > > Is that a change in behavior? > (I thought that all stretchy operators already received a Stretch().) On further reflection this is existing behaviour. (In reply to Karl Tomlinson (:karlt) from comment #31) > > Currently GetPreferredStretchSize() is determining how to accumulate child > metrics based on the direction of the frame (i.e. mPresentationData.flags). > Should it instead use aStretchDirection? This has been changed, but it won't make a difference in behaviour. The ?: expression earlier in the method means that the two ways are equivalent.

mover-2a has been marked as fails as a consequence of reverting the baseFrame/coreFrame change.

(In reply to James Kitchener from comment #32) > Created attachment 8482284 [details] [diff] [review] > Part 1: stretch changes > > Your suggestions have further simplified that patch. Unless it does not work, simpler is ALWAYS better! ;-)

Comment on attachment 8482284 [details] [diff] [review] Part 1: stretch changes This looks close thanks, but I have a request for a changes in Stretch() and probably at least a small change in FinalizeReflow too. >- if (aStretchDirection != NS_STRETCH_DIRECTION_DEFAULT && >- aStretchDirection != mEmbellishData.direction) { >+ if (aStretchDirection != mEmbellishData.direction) { I /think/ this is sensible. This is the only place in nsMathMLContainerFrame's implementation of Stretch() that the aStretchDirection parameter is used, so it seems that its only purpose here is to indicate the direction for which aContainerSize is calculated. That's consistent with the documentation, which implies that aContainerSize is only useful when stretching in aStretchDirection. http://hg.mozilla.org/mozilla-central/annotate/532b5fb77ba1/layout/mathml/nsIMathMLFrame.h#l75 * @param aStretchDirection [in] the direction where to attempt to * stretch. * @param aContainerSize [in] struct that suggests the maximumn size for * the stretched frame. Only member data of the struct that are * relevant to the direction are used (the rest is ignored). So, if aStretchDirection is NS_STRETCH_DIRECTION_DEFAULT, then we don't know the direction for which aContainerSize might be useful. > if (mEmbellishData.direction == NS_STRETCH_DIRECTION_UNSUPPORTED) { > containerSize = childSize.mBoundingMetrics; > } > else { > GetPreferredStretchSize(aRenderingContext, > stretchAll ? STRETCH_CONSIDER_EMBELLISHMENTS : 0, > mEmbellishData.direction, containerSize); >+ if (mEmbellishData.direction == NS_STRETCH_DIRECTION_HORIZONTAL && >+ NS_MATHML_WILL_STRETCH_ALL_CHILDREN_HORIZONTALLY(mPresentationData.flags)) { >+ // Stop subsequent calls from retriggering size calculations >+ aStretchDirection = NS_STRETCH_DIRECTION_HORIZONTAL; >+ } > } > } > > // do the stretching... > mathMLFrame->Stretch(aRenderingContext, >- mEmbellishData.direction, containerSize, childSize); >+ aStretchDirection, containerSize, childSize); I think the change from mEmbellishData.direction to aStretchDirection for the base stretch is sensible. nsMathMLmoFrame::Stretch() uses aStretchDirection to also determine the direction of the stretch (as indicated in the documentation quoted above) and usually won't search for a size variant if the direction doesn't match the indicated direction. I suspect we already often search for size variants in unnecessary situations but we don't need to trigger this here unless there is actually an element inducing a stretch in the supported direction. http://hg.mozilla.org/mozilla-central/annotate/372ce1f36116/layout/mathml/nsMathMLChar.cpp#l1547 if ((aStretchDirection != direction && aStretchDirection != NS_STRETCH_DIRECTION_DEFAULT) || (aStretchHint & ~NS_STRETCH_MAXWIDTH) == NS_STRETCH_NONE) { mDirection = NS_STRETCH_DIRECTION_UNSUPPORTED; return NS_OK; } One thing that worries me there is that largeops without directions (grep largeop layout/mathml/mathfont.properties | grep -v vertical) will only be stretched if aStretchDirection is NS_STRETCH_DIRECTION_DEFAULT. And largeops with vertical intrinsic stretch directions are never large if the stretch direction is horizontal. That issue is probably not directly related to changes here, even if it may be affected by changes here, so I'm happy to deal with this separately if there is a problem. However, if the aStretchDirection is not changed, then it shouldn't be necessary to call GetPreferredStretchSize() again (and it doesn't make sense to call it with mEmbellishData.direction when different from the direction passed to the child stretch). So I think at least the block calling GetPreferredStretchSize() should be conditional on the WILL_STRETCH_ALL test. When mEmbellishData.direction == NS_STRETCH_DIRECTION_UNSUPPORTED, I don't know why there would ever be a need to use the containerSize, so I don't know why it would ever need updating. Perhaps that test was there only to save unnecessary calls to GetPreferredStretchSize(). How can you be sure that only horizontal stretching container need to change the stretch direction? If a horizontally stretching container initiates a stretch for the vertically stretching base with a vertically stretching operator, then how would the operator be stretched? I suspect the WILL_STRETCH_ALL test here should be similar to the ?: test in GetPreferredStretchSize() and aStretchDirection can be set to mEmbellishData.direction if stretching all. > bool stretchAll = > /* NS_MATHML_WILL_STRETCH_ALL_CHILDREN_VERTICALLY(mPresentationData.flags) || */ > NS_MATHML_WILL_STRETCH_ALL_CHILDREN_HORIZONTALLY(mPresentationData.flags); > > nsBoundingMetrics defaultSize; >- if (mEmbellishData.coreFrame == this /* case of a bare <mo>...</mo> itself */ >- || stretchAll) { /* or <mover><mo>...</mo>...</mover>, or friends */ >+ nsStretchDirection stretchDir; >+ if (mEmbellishData.coreFrame == this || /* case of a bare <mo>...</mo> itself */ >+ (mEmbellishData.direction == NS_STRETCH_DIRECTION_HORIZONTAL && >+ stretchAll) || /* or <mover><mo>...</mo>...</mover>, or friends */ >+ mEmbellishData.direction == NS_STRETCH_DIRECTION_UNSUPPORTED) { /* Doesn't stretch */ > // use our current size as computed earlier by Place() > defaultSize = aDesiredSize.mBoundingMetrics; >+ stretchDir = mEmbellishData.direction; > } > else { /* case of <msup><mo>...</mo>...</msup> or friends */ >- // compute a size that doesn't include embellishments >- GetPreferredStretchSize(aRenderingContext, 0, mEmbellishData.direction, >- defaultSize); >+ // Default to a vertical stretch. This will be corrected within >+ // Stretch() where necessary. >+ stretchDir = NS_STRETCH_DIRECTION_VERTICAL; >+ GetPreferredStretchSize(aRenderingContext, 0, stretchDir, defaultSize); > } >- Stretch(aRenderingContext, NS_STRETCH_DIRECTION_DEFAULT, defaultSize, >+ Stretch(aRenderingContext, stretchDir, defaultSize, > aDesiredSize); I would like to suggest just always passing NS_STRETCH_DIRECTION_UNSUPPORTED here, and nsMathMLContainerFrame::Stretch() would determine the appropriate direction and stretch size. However, I suspect that would break largeoponly when this frame is an mo frame. And nsMathMLContainerFrame::Stretch() uses STRETCH_CONSIDER_EMBELLISHMENTS, which doesn't seem quite right to me. I wonder whether passing NS_STRETCH_DIRECTION_DEFAULT would be fine if nsMathMLContainerFrame::Stretch() didn't use STRETCH_CONSIDER_EMBELLISHMENTS. The existing use of aDesiredSize.mBoundingMetrics for the stretch container of <mover> etc also seems odd to me, and inconsistent with nsMathMLContainerFrame::Reflow(), which uses GetPreferredStretchSize(aOptions = 0). However, limiting that to when mEmbellishData.direction == NS_STRETCH_DIRECTION_HORIZONTAL sounds good. Oh, I also notice now that GetPreferredStretchSize() was sometimes called with NS_STRETCH_DIRECTION_UNSUPPORTED, so thanks for sorting that out. (In reply to James Kitchener from comment #32) > The attached patch prevents (well asserts) a default direction from being > passed. > > This removes both the need for the bool and the second > GetPreferredStretchSize call. > > The consequence of this is that a vertical direction needs to be initially > specified if there is the potential for an embedded munderover frame, at > which point the code will correct the direction to horizontal as needed. Would it be possible to always pass mEmbellishData.direction from FinalizeReflow(), even when calling GetPreferredStretchSize()? That would save calculating the stretch size for an unnecessary direction. Would that cause any problems in Stretch()?

> Would it be possible to always pass mEmbellishData.direction from > FinalizeReflow(), even when calling GetPreferredStretchSize()? That would > save calculating the stretch size for an unnecessary direction. Would that > cause any problems in Stretch()? That makes it harder to determine when to retrigger the stretch calculations. My approach was not to not call GetPreferredStretchSize() within FinalizeReflow(), leaving it to the stretch call to recalculate as necessary. * If the embellished op's direction is UNSUPPORTED, DEFAULT or the same as aStretchDirection, use the metrics passed to us. * Otherwise if the container stretches all children in the same direction as the underlying operator, get new metrics via GetPreferredStretchSize, and update the direction to prevent further updates. * If neither of those conditions applies, adopt the baseFrame's metrics. (A GetPreferredStretchSize call would return the metrics of the baseFrame's baseFrame in this situation, but the appropriate metrics should be provided by the time the Stretch call reaches the bottom of its recursive call.) So stretching only occurs if the directions match and is based on the outermost container that stretches all children. Operators that can't stretch do not trigger either recalculations or reassignment of metrics. This also occurs with operators with default direction, but I don't think an overlarge container matters for them.

(In reply to James Kitchener from comment #36) > Created attachment 8489006 [details] [diff] [review] > Part 1: stretch changes I have verified that this patch also does NOT interfere with the pending patch on bug 969906.

Comment on attachment 8489006 [details] [diff] [review] Part 1: stretch changes I like this. Thank you for working through this. (In reply to James Kitchener from comment #36) > Created attachment 8489006 [details] [diff] [review] > Part 1: stretch changes > > > Would it be possible to always pass mEmbellishData.direction from > > FinalizeReflow(), even when calling GetPreferredStretchSize()? That would > > save calculating the stretch size for an unnecessary direction. Would that > > cause any problems in Stretch()? > > That makes it harder to determine when to retrigger the stretch calculations. If the direction is already correct for the first GetPreferredStretchSize(), then I was thinking it should not be necessary to retrigger calculations, except for the bug that GetPreferredStretchSize() does not recursively descend the base frame hierarchy. However, ... > My approach was not to not call GetPreferredStretchSize() within > FinalizeReflow(), leaving it to the stretch call to recalculate as > necessary. ... this seems a sensible approach. > * If the embellished op's direction is UNSUPPORTED, DEFAULT or the same as > aStretchDirection, use the metrics passed to us. >+ if (aStretchDirection != mEmbellishData.direction && >+ mEmbellishData.direction != NS_STRETCH_DIRECTION_UNSUPPORTED && >+ mEmbellishData.direction != NS_STRETCH_DIRECTION_DEFAULT) { nsMathMLOperators::GetStretchyDirection() never returns DEFAULT and so mEmbellishData.direction should never be DEFAULT. Please assert that mEmbellishData.direction != NS_STRETCH_DIRECTION_DEFAULT instead of checking for it in this test. > * Otherwise if the container stretches all children in the same direction as > the underlying operator, get new metrics via GetPreferredStretchSize, and > update the direction to prevent further updates. > * If neither of those conditions applies, adopt the baseFrame's metrics. (A > GetPreferredStretchSize call would return the metrics of the baseFrame's > baseFrame in this situation, but the appropriate metrics should be provided > by the time the Stretch call reaches the bottom of its recursive call.) OK. I gather the third situation here handles incorrect metrics passed to Stretch(NS_STRETCH_DIRECTION_DEFAULT), because NS_STRETCH_DIRECTION_DEFAULT with invoke a Stretch() in nsMathMLmoFrame, and this case is simpler than calling GetPreferredStretchSize(). > So stretching only occurs if the directions match and is based on the > outermost container that stretches all children. > > Operators that can't stretch do not trigger either recalculations or > reassignment of metrics. This also occurs with operators with default > direction, but I don't think an overlarge container matters for them. No need to worry about operators with default direction, because there are none.

Review comment addressed

https://tbpl.mozilla.org/?tree=Try&rev=8170e558db3f https://tbpl.mozilla.org/?tree=Try&rev=b61f03daa157

https://hg.mozilla.org/integration/mozilla-inbound/rev/784dab51b0dc https://hg.mozilla.org/integration/mozilla-inbound/rev/5a4a31220aca

https://hg.mozilla.org/mozilla-central/rev/784dab51b0dc https://hg.mozilla.org/mozilla-central/rev/5a4a31220aca