Bug 687807 - Base of munderover not stretched
 Summary: Base of munderover not stretched
 Status: RESOLVED FIXED Core Components MathML (show other bugs) Trunk x86_64 Linux P3 normal (vote) mozilla35 James Kitchener (:jkitch) Anthony Jones (:kentuckyfriedtakahe, :k17e) (view as bug list) mathml-in-mathjax 854339 Show dependency tree / graph

Reported: 2011-09-20 03:26 PDT by Frédéric Wang (:fredw)
Modified: 2014-10-12 02:48 PDT (History)
6 users (show)
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Attachments
testcase (626 bytes, application/xhtml+xml)
2011-09-20 03:26 PDT, Frédéric Wang (:fredw)
no flags Details
Patch (5.87 KB, patch)
2012-11-29 12:46 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V2 (2.12 KB, patch)
2013-04-06 07:36 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Reftest (2.54 KB, patch)
2013-04-06 07:37 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
testcase with two nested mrows (670 bytes, application/xhtml+xml)
2013-05-01 01:57 PDT, Karl Tomlinson (:karlt)
no flags Details
testcase causing too much horizontal stretching (733 bytes, application/xhtml+xml)
2013-05-01 02:04 PDT, Karl Tomlinson (:karlt)
no flags Details
stretchy-embellished-op.html (2.73 KB, text/html)
2013-10-30 08:41 PDT, Frédéric Wang (:fredw)
no flags Details
Part 1: stretch changes (26.56 KB, patch)
2014-05-01 23:58 PDT, James Kitchener (:jkitch)
fred.wang: feedback+
Details | Diff | Splinter Review
Part 2: tests (32.85 KB, patch)
2014-05-01 23:59 PDT, James Kitchener (:jkitch)
fred.wang: review+
fred.wang: feedback+
Details | Diff | Splinter Review
Part 2: tests (30.68 KB, patch)
2014-05-12 04:32 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 1: stretch changes (33.49 KB, patch)
2014-05-12 05:49 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 1: stretch changes (15.22 KB, patch)
2014-08-11 04:58 PDT, James Kitchener (:jkitch)
karlt: review-
Details | Diff | Splinter Review
Part 2: tests (32.00 KB, patch)
2014-08-11 05:00 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
nested horizontal stretching container examples (1.54 KB, text/html)
2014-08-17 20:57 PDT, Karl Tomlinson (:karlt)
no flags Details
Part 1: stretch changes (8.62 KB, patch)
2014-09-01 06:46 PDT, James Kitchener (:jkitch)
karlt: review-
karlt: feedback+
Details | Diff | Splinter Review
Part 2: tests (32.01 KB, patch)
2014-09-01 06:49 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 1: stretch changes (9.77 KB, patch)
2014-09-13 05:44 PDT, James Kitchener (:jkitch)
karlt: review+
Details | Diff | Splinter Review
Part 1: stretch changes (9.87 KB, patch)
2014-09-19 20:13 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review

 Frédéric Wang (:fredw) 2011-09-20 03:26:52 PDT Created attachment 561157 [details] testcase This was reported by MathJax's author when writing the mhchem extension. See the testcase. The arrow is stretched if the is removed or if another child is added to the . The problem was is present in Iceweasel (Firefox 3.5). I'm wondering if it is a regression of bug 21479. Frédéric Wang (:fredw) 2012-11-27 08:37:54 PST 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 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. Frédéric Wang (:fredw) 2012-11-29 12:46:53 PST Created attachment 686753 [details] [diff] [review] Patch Frédéric Wang (:fredw) 2012-11-30 08:25:35 PST Mass change: setting priority to 3 for bugs preventing Gecko's Native MathML to be enabled by default in MathJax. Frédéric Wang (:fredw) 2013-02-14 07:49:04 PST 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. Frédéric Wang (:fredw) 2013-04-06 07:36:49 PDT Created attachment 734244 [details] [diff] [review] Patch V2 Frédéric Wang (:fredw) 2013-04-06 07:37:53 PDT Created attachment 734245 [details] [diff] [review] Reftest Frédéric Wang (:fredw) 2013-04-06 07:40:23 PDT 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. Karl Tomlinson (:karlt) 2013-05-01 01:23:09 PDT 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. Karl Tomlinson (:karlt) 2013-05-01 01:54:14 PDT 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? Karl Tomlinson (:karlt) 2013-05-01 01:57:23 PDT Created attachment 744057 [details] testcase with two nested mrows Here the arrow stretches as expected, even though the top-level embellished operator is a child of [itex]. Karl Tomlinson (:karlt) 2013-05-01 02:04:16 PDT Created attachment 744059 [details] testcase causing too much horizontal stretching 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. Frédéric Wang (:fredw) 2013-10-30 08:41:08 PDT Created attachment 824680 [details] stretchy-embellished-op.html 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 James Kitchener (:jkitch) 2014-04-27 07:30:56 PDT Sealing assign. I have a mostly working patch which needs a little more testing. James Kitchener (:jkitch) 2014-05-01 23:58:47 PDT Created attachment 8416349 [details] [diff] [review] Part 1: stretch changes 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]. James Kitchener (:jkitch) 2014-05-01 23:59:53 PDT Created attachment 8416350 [details] [diff] [review] Part 2: tests Suggestions on additional tests are welcome. Frédéric Wang (:fredw) 2014-05-02 03:12:15 PDT How does this patch affect the results of the existing tests (especially the embellished-op tests) and of the MathML Acid3 tests? James Kitchener (:jkitch) 2014-05-02 04:23:57 PDT 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. Frédéric Wang (:fredw) 2014-05-02 05:35:30 PDT (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. Bill Gianopoulos [:WG9s] 2014-05-04 05:53:56 PDT (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? Bill Gianopoulos [:WG9s] 2014-05-04 05:55:05 PDT (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! Frédéric Wang (:fredw) 2014-05-04 09:15:49 PDT 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. James Kitchener (:jkitch) 2014-05-12 04:32:10 PDT Created attachment 8420913 [details] [diff] [review] Part 2: tests 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. James Kitchener (:jkitch) 2014-05-12 05:49:57 PDT Created attachment 8420952 [details] [diff] [review] Part 1: stretch changes 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. Karl Tomlinson (:karlt) 2014-05-19 21:12:18 PDT 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]. James Kitchener (:jkitch) 2014-08-11 04:58:44 PDT 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. 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). James Kitchener (:jkitch) 2014-08-11 05:00:30 PDT Created attachment 8470796 [details] [diff] [review] Part 2: tests Some of the tests have been tidied up and there are a few new ones. Nothing exciting. James Kitchener (:jkitch) 2014-08-11 05:09:59 PDT Comment hidden (obsolete) 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. While most of the time the stretch routine will return the character unaltered, there is the potential for rounding error to occur on platforms where nscoord is an integer. Bill Gianopoulos [:WG9s] 2014-08-11 10:13:20 PDT The great news is that the new patch does NOT interfere with the fix to bug 969906 the way the old patch did. Karl Tomlinson (:karlt) 2014-08-17 20:57:01 PDT Created attachment 8474344 [details] nested horizontal stretching container examples (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. Karl Tomlinson (:karlt) 2014-08-17 21:08:37 PDT 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().) Karl Tomlinson (:karlt) 2014-08-17 21:10:03 PDT 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 James Kitchener (:jkitch) 2014-09-01 06:46:13 PDT Created attachment 8482284 [details] [diff] [review] Part 1: stretch changes 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. James Kitchener (:jkitch) 2014-09-01 06:49:54 PDT Created attachment 8482286 [details] [diff] [review] Part 2: tests mover-2a has been marked as fails as a consequence of reverting the baseFrame/coreFrame change. Bill Gianopoulos [:WG9s] 2014-09-01 07:01:34 PDT (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! ;-) Karl Tomlinson (:karlt) 2014-09-03 01:14:08 PDT 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 ... itself */ >- || stretchAll) { /* or ......, or friends */ >+ nsStretchDirection stretchDir; >+ if (mEmbellishData.coreFrame == this || /* case of a bare ... itself */ >+ (mEmbellishData.direction == NS_STRETCH_DIRECTION_HORIZONTAL && >+ stretchAll) || /* or ......, 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 ...... 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 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()? James Kitchener (:jkitch) 2014-09-13 05:44:46 PDT 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. 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. Bill Gianopoulos [:WG9s] 2014-09-14 06:55:49 PDT (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. Karl Tomlinson (:karlt) 2014-09-16 21:00:13 PDT 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. James Kitchener (:jkitch) 2014-09-19 20:13:10 PDT Created attachment 8492545 [details] [diff] [review] Part 1: stretch changes Review comment addressed James Kitchener (:jkitch) 2014-09-19 20:17:24 PDT https://tbpl.mozilla.org/?tree=Try&rev=8170e558db3f https://tbpl.mozilla.org/?tree=Try&rev=b61f03daa157 Carsten Book [:Tomcat] 2014-09-21 23:54:02 PDT https://hg.mozilla.org/integration/mozilla-inbound/rev/784dab51b0dc https://hg.mozilla.org/integration/mozilla-inbound/rev/5a4a31220aca Carsten Book [:Tomcat] 2014-09-22 04:00:24 PDT https://hg.mozilla.org/mozilla-central/rev/784dab51b0dc https://hg.mozilla.org/mozilla-central/rev/5a4a31220aca Loic 2014-10-12 02:48:18 PDT *** Bug 1081641 has been marked as a duplicate of this bug. ***

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