Last Comment Bug 687807 - Base of munderover not stretched
: Base of munderover not stretched
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: x86_64 Linux
: P3 normal (vote)
: mozilla35
Assigned To: James Kitchener (:jkitch)
:
Mentors:
http://lists.w3.org/Archives/Public/w...
: 1081641 (view as bug list)
Depends on:
Blocks: mathml-in-mathjax 854339
  Show dependency treegraph
 
Reported: 2011-09-20 03:26 PDT by Frédéric Wang (:fredw)
Modified: 2014-10-12 02:48 PDT (History)
6 users (show)
See Also:
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 | Review
Patch V2 (2.12 KB, patch)
2013-04-06 07:36 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Review
Reftest (2.54 KB, patch)
2013-04-06 07:37 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Review
testcase with two nested mrows (670 bytes, application/xhtml+xml)
2013-05-01 01:57 PDT, Karl Tomlinson (ni?:karlt)
no flags Details
testcase causing too much horizontal stretching (733 bytes, application/xhtml+xml)
2013-05-01 02:04 PDT, Karl Tomlinson (ni?: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 | 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 | Review
Part 2: tests (30.68 KB, patch)
2014-05-12 04:32 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Review
Part 1: stretch changes (33.49 KB, patch)
2014-05-12 05:49 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Review
Part 1: stretch changes (15.22 KB, patch)
2014-08-11 04:58 PDT, James Kitchener (:jkitch)
karlt: review-
Details | Diff | Review
Part 2: tests (32.00 KB, patch)
2014-08-11 05:00 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Review
nested horizontal stretching container examples (1.54 KB, text/html)
2014-08-17 20:57 PDT, Karl Tomlinson (ni?: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 | Review
Part 2: tests (32.01 KB, patch)
2014-09-01 06:49 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Review
Part 1: stretch changes (9.77 KB, patch)
2014-09-13 05:44 PDT, James Kitchener (:jkitch)
karlt: review+
Details | Diff | Review
Part 1: stretch changes (9.87 KB, patch)
2014-09-19 20:13 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Review

Description 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 <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.
Comment 1 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 <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.
Comment 2 Frédéric Wang (:fredw) 2012-11-29 12:46:53 PST
Created attachment 686753 [details] [diff] [review]
Patch
Comment 3 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.
Comment 4 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.
Comment 5 Frédéric Wang (:fredw) 2013-04-06 07:36:49 PDT
Created attachment 734244 [details] [diff] [review]
Patch V2
Comment 6 Frédéric Wang (:fredw) 2013-04-06 07:37:53 PDT
Created attachment 734245 [details] [diff] [review]
Reftest
Comment 7 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.
Comment 8 Karl Tomlinson (ni?: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.
Comment 9 Karl Tomlinson (ni?: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?
Comment 10 Karl Tomlinson (ni?: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 <math>.
Comment 11 Karl Tomlinson (ni?: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.
Comment 12 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
Comment 13 James Kitchener (:jkitch) 2014-04-27 07:30:56 PDT
Sealing assign.  I have a mostly working patch which needs a little more testing.
Comment 14 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].
Comment 15 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.
Comment 16 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?
Comment 17 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.
Comment 18 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.
Comment 19 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?
Comment 20 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!
Comment 21 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.
Comment 22 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.
Comment 23 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.
Comment 24 Karl Tomlinson (ni?: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].
Comment 25 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).
Comment 26 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.
Comment 27 James Kitchener (:jkitch) 2014-08-11 05:09:59 PDT Comment hidden (obsolete)
Comment 28 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.
Comment 29 Karl Tomlinson (ni?: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.
Comment 30 Karl Tomlinson (ni?: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().)
Comment 31 Karl Tomlinson (ni?: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
Comment 32 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.
Comment 33 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.
Comment 34 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! ;-)
Comment 35 Karl Tomlinson (ni?: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 <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()?
Comment 36 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.
Comment 37 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.
Comment 38 Karl Tomlinson (ni?: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.
Comment 39 James Kitchener (:jkitch) 2014-09-19 20:13:10 PDT
Created attachment 8492545 [details] [diff] [review]
Part 1: stretch changes

Review comment addressed
Comment 43 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.