1092053 - The size of displaystyle largeop should be taken into account when determining the stretch size

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Frédéric Wang Nélar]

Attachment: stretch.html

From the spec:

"If a stretchy operator is a direct sub-expression of an mrow element, or is the sole direct sub-expression of an mtd element in some row of a table, then it should stretch to cover the height and depth (above and below the axis) of the non-stretchy direct sub-expressions in the mrow element or table row, unless stretching is constrained by minsize or maxsize attributes. In the case of an embellished stretchy operator, the preceding rule applies to the stretchy operator at its core."

Note that in MathML "stretchy" and displaystyle "largeop" are different notions, even if at the end we may use the same technique to draw them (picking size variants). So largeop display operators should be considered "non-stretchy" and thus their sizes should be taken into account for the stretch size.

In the attached testcase,

- The parenthesis should match the size of the two first integrals (embellished or not) because they are non-stretchy.

- However, they should *not* match the size of the minsize="2em" vertical line (as we do now), because this vertical line is a stretchy operator.

Comment 1

[Frédéric Wang Nélar]

Oh, and for the record, this was reported on the MathML in Wikipedia option: https://www.mediawiki.org/wiki/Extension:Math/Roadmap#Next_steps

Comment 2

[Karl Tomlinson (back June 29 :karlt)]

(In reply to Frédéric Wang (:fredw) from comment #0)
> Created attachment 8514833 [details]
> stretch.html
> 
> From the spec:
> 
> "If a stretchy operator is a direct sub-expression of an mrow element, or is
> the sole direct sub-expression of an mtd element in some row of a table,
> then it should stretch to cover the height and depth (above and below the
> axis) of the non-stretchy direct sub-expressions in the mrow element or
> table row, unless stretching is constrained by minsize or maxsize
> attributes. In the case of an embellished stretchy operator, the preceding
> rule applies to the stretchy operator at its core."
> 
> Note that in MathML "stretchy" and displaystyle "largeop" are different
> notions, even if at the end we may use the same technique to draw them
> (picking size variants). So largeop display operators should be considered
> "non-stretchy" and thus their sizes should be taken into account for the
> stretch size.
> 
> In the attached testcase,
> 
> - The parenthesis should match the size of the two first integrals
> (embellished or not) because they are non-stretchy.

That is the important part here IMO.

> - However, they should *not* match the size of the minsize="2em" vertical
> line (as we do now), because this vertical line is a stretchy operator.

Excluding stretchy sub-expressions, and only for vertical stretched, seems odd to me, and susceptible to subtle changes in the formula.

However, in this testcase the parentheses should be at least as high as the normal unstretched height of the vertical line because

"If a stretchy operator is required to stretch, but all other expressions in the containing element (as described above) are also stretchy, all elements that can stretch should grow to the maximum of the normal unstretched sizes of all elements in the containing object, if they can grow that large."

Comment 3

[Frédéric Wang Nélar]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> > - The parenthesis should match the size of the two first integrals
> > (embellished or not) because they are non-stretchy.
> 
> That is the important part here IMO.

Yes, it is. The second point was only to illustrate the difference.

Comment 4

[James Kitchener (:jkitch)]

Attachment: patch

This code checks for largeops when determining the appropriate stretch size.  The stretch metrics are enlarged by the larger of the largeop metrics or the less-embellished child container.

The alternative to this approach is to perform the "stretching" of largeop and stretchy operators at different points in the reflow process.  This will give different results in the case where the largeop is more heavily embellished (for example multiple nested <msup> elements).

While modifying the method, the opportunity to remove a redundant null check was taken. (Handled by do_QueryFrame())

Comment 5

[Karl Tomlinson (back June 29 :karlt)]

Comment on attachment 8515864 [details] [diff] [review]
patch

If you need commit access for try server, I can vouch for you.

>           if (mathMLchildFrame) {
>             mathMLFrame = mathMLchildFrame;
>           }
>         }
>         mathMLFrame->GetBoundingMetrics(bmChild);
>+        /* Special behaviour for largeops.
>+           In MathML "stretchy" and displaystyle "largeop" are different notions, even if we use
>+           the same technique to draw them (picking size variants).  So largeop display operators
>+           should be considered "non-stretchy" and thus their sizes should be taken into account
>+           for the stretch size.
>+         */
>+        nsMathMLmoFrame* opFrame = do_QueryFrame(embellishData.coreFrame);
>+        if (opFrame) {
>+          nsBoundingMetrics metrics;
>+          bool largeOp =  opFrame->GetLargeOpMetrics(aRenderingContext,
>+                                                     metrics);

If the large op is a stretchy embellished operator, then I expect this would
restretch to a possibly smaller size after the Stretch that happened before
the call to GetPreferredStretchSize() here:

http://hg.mozilla.org/mozilla-central/annotate/7f0d92595432/layout/mathml/nsMathMLContainerFrame.cpp#l377

r- based on that for now, but perhaps you can convince me otherwise.

I wonder whether Reflow() would be a good time to do initial largeop stretches, and they can be restretched later as necessary ...

(In reply to James Kitchener (:jkitch) from comment #4)
> The alternative to this approach is to perform the "stretching" of largeop
> and stretchy operators at different points in the reflow process.  This will
> give different results in the case where the largeop is more heavily
> embellished (for example multiple nested <msup> elements).

... but I'm not clear on the issue here.
Would that address these problems?

>+            if (metrics.ascent > bmChild.ascent) {
>+              bmChild.ascent = metrics.ascent;
>+            }
>+            if (metrics.descent > bmChild.descent) {
>+              bmChild.descent = metrics.descent;
>+            }
>+            if (metrics.leftBearing > bmChild.leftBearing) {
>+              bmChild.leftBearing = metrics.leftBearing;
>+            }
>+            if (metrics.rightBearing > bmChild.rightBearing) {
>+              bmChild.rightBearing = metrics.rightBearing;
>+            }
>+            if (metrics.width > bmChild.width) {
>+              bmChild.width = metrics.width;
>+            }

Looks like some kind of Union function/method for nsBoundingMetrics would be
useful.

>+  nsBoundingMetrics container; // largeops do not care about container size

They do care if they are stretchy.
I expect this works because the default constructor makes the metrics all
zero, ...

>+  nsresult rv = mMathMLChar.Stretch(PresContext(), aRenderingContext,
>+                                    NS_STRETCH_DIRECTION_DEFAULT,
>+                                    container, aMetrics, stretchHint,
>+                                    StyleVisibility()->mDirection);

... but I think it would be clearer, and perhaps more efficient, to pass
NS_STRETCH_LARGEOP instead of stretchHint here.

>+  return NS_SUCCEEDED(rv) ? true : false;

return NS_SUCCEEDED(rv);

>+    function definitelyLessThan(x, y) {
>+      var e = x - y / 3;
>+      return e > epsilon;
>+    }

I'm not understanding the test here.  Some comments below as a parameter to
the ok() test would be helpful.

Why divide y by 3?
Don't you want e < epsilon for x almost less than y?
What is the "definitely" distinction here if there is an epsilon accepting not
quite less than?

>+    ok(almostEqualAbs(mo1.height, largeop1.height));
>+    ok(almostEqualAbs(mo2.height, largeop1.height));
>+    ok(almostEqualAbs(mo3.height, largeop2.height));
>+    ok(almostEqualAbs(mo4.height, largeop2.height));
>+    ok(almostEqualAbs(mo4.height, largeop2.height));
>+    ok(almostEqualAbs(mo4.height, largeop2.height));

Since changes in bug 960115, these are not necessarily close to equal.
I would like them to be but that was not the decision and so is not
implemented.

The code could be tested by checking that operators are taller than the
unstretched size.

>+    ok(definitelyLessThan(minsize1.height, mo5.height));
>+    ok(definitelyLessThan(minsize1.height, mo6.height));

This should be less than or equal to, depending on whether or not
embellishments are considered, right?

But I don't understand how these code changes make the parentheses stretch.

Comment 6

[James Kitchener (:jkitch)]

Attachment: patch

The attached patch follows your suggestion and calculates an approximation of the largeop's displaystyle metrics within nsMathMLmoFrame::Reflow().  (Approximate as it omits further calculations which are performed in Stretch, however the differences shouldn't be significant)

The included test now only checks whether the displaystyle operators are larger than the unstretched versions, the earlier font metric assumptions have been removed.

try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7be4badcd823

Comment 7

[James Kitchener (:jkitch)]

Comment on attachment 8527163 [details] [diff] [review]
patch

See comment 6

Comment 8

[Karl Tomlinson (back June 29 :karlt)]

Comment on attachment 8527163 [details] [diff] [review]
patch

This is much better, but a little out of line with how sizes are usually
calculated.  Usually Reflow() calls FinalizeReflow() with calls Place() and
sizes are calculated in Place().

When !parentWillFireStretch in FinalizeReflow(), this new
mMathMLChar.Stretch() is happening after the nsMathMLmoFrame::Stretch(),
FixInterFrameSpacing(), and GatherAndStoreOverflow() from FinalizeReflow().  I
guess that works out OK because this mMathMLChar::Stretch() is producing the
same size character as the call from nsMathMLmoFrame::Stretch(), but given the
code is quite different, that's non-obvious if always true.

I think an approach more consistent with what usually happens would be to
override Place() in nsMathMLmoFrame, and put this new code there.  If
aPlaceOrigin is false, then the call comes from FinalizeReflow() and there
will be a later Stretch() so largeops can calculate their size with
mMathMLChar.Stretch().  If aPlaceOrigin is true, then the call comes from
nsMathMLmoFrame::Stretch(), which is after the final mMathMLChar.Stretch(),
but that call is just for calling FinishReflowChild() on the children; in that
situation, another mMathMLChar.Stretch() need not and should not be performed,
but nsMathMLmoFrame::Place() can just chain up to nsMathMLTokenFrame.

>+        /* Verify if the size of the element matches the reference, and

"height of the element is greater than that of" or "verify that the element is
taller than" or something similar.

>+      window.addEventListener("MozReftestInvalidate", doTest, false);

I think you can just use the "load" event here.

MozReftestInvalidate ensures that that document has already been drawn once so
as to test that changes to the document are redrawn correctly.

getBoundingClientRect() will trigger reflow synchronously and so should do
enough to ensure that the dimensions are accurate.

>+      <!-- &bigotimes; included in the test as an assertion check-->
>+      <math displaystyle="true">
>+        <mrow>
>+          <mo id="mo2">(</mo>
>+          <mo>&Product;</mo>

This comment appears twice in the reference.  Is this one unintentional?

Is &bigotimes to check that reftest-wait doesn't change the size of big
operators?

Comment 9

[James Kitchener (:jkitch)]

Attachment: patch

Review comments addressed.

The assertion mentioned involves the direction passed to mMathMLChar.Stretch().  ⨂ insists on a vertical direction.

Comment 10

[James Kitchener (:jkitch)]

Attachment: patch

Previous one removed reftest-wait, which may still be needed.

Comment 11

[Karl Tomlinson (back June 29 :karlt)]

Comment on attachment 8532487 [details] [diff] [review]
patch

Yes, I think reftest-wait is good, as is everything here, thank you.

Comment 12

[James Kitchener (:jkitch)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d3e4e2c83b5b

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2633caf667e1

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2633caf667e1

Comment 15

[Frédéric Wang Nélar]

Attachment: Bug 1092053 - Export a crashtest for bigotimes to WPT. r=emilio

This case was included in a reftest in order to exercise an assertion
related to the direction passed for operator stretching but no visual
are JS checks are actually performed on it. So export it in its own
separate crash test instead.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1092053#c9

Comment 16

[Phabricator Automation]

Comment on attachment 9388957 [details]
Bug 1092053 - Export a crashtest for bigotimes to WPT. r=emilio

Revision D203394 was moved to bug 1882817. Setting attachment 9388957 [details] to obsolete.