670334 - Empty mfenced elements should display normal size of fences

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Florian Scholz (Open Web Docs)]

Attachment: Test case

Some follow-up information from bug 668969 and bug 553918:

> The height of the fences should be considered in containerSize and I don't
> see
> that happening.  This might be a regression from bug 414277, because, before
> then, characters were always stretched to at least their base size.

> I think nsMathMLmfencedFrame::Reflow needs to get
> the normal size of the fences and consider them when setting containerSize.

See also the sample rendering in the MathML3 testsuite:
http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/mfenced/mfenced1-full.xhtml

Also the attached test case could be used as a reftest.

Comment 1

[Frédéric Wang (:fredw)]

@Florian: maybe the simplest way to solve this bug would be to directly return NS_OK when the base size is fine here:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLChar.cpp#1553

---
I also see issues with small roots badly displayed on Linux (probably due to the scale transform) and I think this change could fix the problem too:

http://www.maths-informatique-jeux.com/international/hsp/web/efficient_quantum_algorithms_and_HSP.php#id3.1.

Comment 2

[Florian Scholz (Open Web Docs)]

Attachment: Patch V1

(In reply to Frédéric Wang from comment #1)
> @Florian: maybe the simplest way to solve this bug would be to directly
> return NS_OK when the base size is fine here:
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLChar.
> cpp#1553

Thanks Frédéric, I've put that in a patch. Looks good to me too.

Comment 3

[Frédéric Wang (:fredw)]

(In reply to Florian Scholz [:fs] from comment #2)
> Created attachment 612902 [details] [diff] [review]
> Patch V1
> 
> (In reply to Frédéric Wang from comment #1)
> > @Florian: maybe the simplest way to solve this bug would be to directly
> > return NS_OK when the base size is fine here:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLChar.
> > cpp#1553
> 
> Thanks Frédéric, I've put that in a patch. Looks good to me too.

Thanks but my idea was just to replace "done = true;" by "return NS_OK;". If the size does not fit, we want to continue the stretching.

Comment 4

[Florian Scholz (Open Web Docs)]

Attachment: Patch V1.1

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 613274 [details] [diff] [review]
Patch V1.1

(In reply to Frédéric Wang from comment #1)
> I also see issues with small roots badly displayed on Linux (probably due to
> the scale transform)

I only see issues with small roots when building with --enable-system-cairo, which doesn't have the fix for bug 518172.  I like the smaller roots.  With some fonts I've seen the roots looking too large.

I also see this approach as not giving the optimal size in some other situations.  e.g. when slightly larger than the base size is requested.

Is it possible to get the normal sizes of the fences by calling Stretch() on the nsMathMLChars with NS_STRETCH_NONE?

Comment 6

[Frédéric Wang (:fredw)]

> Is it possible to get the normal sizes of the fences by calling Stretch() on
> the nsMathMLChars with NS_STRETCH_NONE?

So you mean calling Stretch() twice, a first call with NS_STRETCH_NONE to get the normal size (which will be stored in charSize) before the current Stretch call:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmfencedFrame.cpp#511

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Frédéric Wang from comment #6)
> > Is it possible to get the normal sizes of the fences by calling Stretch() on
> > the nsMathMLChars with NS_STRETCH_NONE?
> 
> So you mean calling Stretch() twice,

Yes, but ...

> a first call with NS_STRETCH_NONE to
> get the normal size (which will be stored in charSize) before the current
> Stretch call:
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/mathml/
> nsMathMLmfencedFrame.cpp#511

... if mfenced were to behave like the spec'd nesting of mrows, and our mrow implementation, the children would be stretched as high as the separators.  This would require getting the normal sizes in nsMathMLmfencedFrame::Reflow() rather than immediately before the stretch in ReflowChar().

For mfenced with one child, or where the children make their implied mrow an
embellished operator, the open and close characters would be considered in the
stretch applied to the child.

However, our mrow implementation doesn't quite follow the spec.  According to
the spec, stretchy children should not be considered in the determining the
stretch size for vertical stretches unless there are no non-stretchy children.
(I personally don't see why stretchy and non-stretchy children should be
treated differently, nor why vertical stretching should be different from
horizontal.)

I don't think it is so important to follow the letter of the spec, but perhaps
we should think about what we really want to fix here.

Is the bug just that mfenced behaves differently from mrow, or do you think
that the current mfenced rendering is undesirable?

I remember now Frédéric's post including a comment about minsize.
http://lists.w3.org/Archives/Public/www-math/2012Jan/0003.html

Perhaps the best thing to do here is to set up a default minsize of 1?
That would resolve the issue for this testcase (even if it doesn't make
mfenced and mrow always behave similarly).

However, I don't think we have an effective minsize implementation.
Perhaps as a start, nsMathMLChar could be modified to never scale smaller than the base size.

I don't really mind if that ends up meaning that roots are always at least base size.  If the smaller roots are preferred, we could add a stretch flag to behave differently for roots.

Comment 8

[David Carlisle]

I was going to submit an mfenced bug but I suspect the issue is the same as this one, so adding a comment here,  <mfenced><mo>&#183;</mo></mfenced> shrinks the brackets to the size of the dot making it unreadable. I tried adding minsize but didn't seem to help. An online test case is

http://monet.nag.co.uk/~dpc/cdotbracket.html

(I'm on windows with stix fonts installed, using nightly 19.0a1 (2012-10-28)

In our production code I switch to <mrow><mo>(</mo> instead of mfenced, so a workaround is possible but it would be nice for the brackets not to shrink.

Comment 9

[Frédéric Wang (:fredw)]

Neil, Bruce and other MathML WG members just think <mfenced> has a bad design and that <mrow>+<mo> should be preferred but they keep it in MathML because of some compromises with other community demands (it seems to be still necessary for Opera's implementation for instance). In Gecko, <mfenced> and <mrow>+<mo> are really implemented in disjoint code areas. This leads to inconsistencies and bugs like this one. I agree that since we implement mfenced, this bug should be fixed, but I'm not sure it is a top priority at the moment...

Comment 10

[Frédéric Wang (:fredw)]

Attachment: Testcase with MathJax

Comment 11

[Frédéric Wang (:fredw)]

So this bug seems to be fixed for me(probably by the changes in bug 960115). I wonder if James' patch for bug 687807 also fixes it, even when the scale is applied.

I guess it's still worth extracting the reftest mfenced-11 from Florian's patch and integrate it into our test suite.

Comment 12

[James Kitchener (:jkitch)]

I can still reproduce this bug on Windows with the latest Nightly.

Comment 13

[James Kitchener (:jkitch)]

Attachment: patch

This follows the suggestion from comment 6.

I have considered the suggestion from comment 7 by setting a minimum stretch size within nsMathMLChar::Stretch() on an opt-in basis, but this produced inferior metrics.

The issue is that the metrics for the stretchy character are manipulated before the Stretch() call.
https://hg.mozilla.org/mozilla-central/annotate/0ed32d9a42d6/layout/mathml/nsMathMLmfencedFrame.cpp#l299

If the minimum metrics are applied after the centering, the resulting metrics will probably not be centered appropriately and will look less like the <mrow> equivalent.

The attached testcase is somewhat wonky in that it doesn't work for all platforms.

Comment 14

[Karl Tomlinson (:karlt)]

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

>+ApplyDefaultMetrics(nsPresContext*      aPresContext,

What do you think about calling this "ApplyUnstretchedMetrics"?

Comment 15

[James Kitchener (:jkitch)]

Attachment: patch

Renamed

Comment 16

[James Kitchener (:jkitch)]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27bc3dbcaff5

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27bc3dbcaff5

Comment 19

[Frédéric Wang (:fredw)]

Does this bug improves the situation with bug 727804?

Comment 20

[James Kitchener (:jkitch)]

Yes, separators are now included when considering fence size.  

(Fences are also included when considering separator size, which doesn't quite follow the MathML standard, but bug 121748 decided that a stretch size of "everything" looked nicer).

Unfortunately the implementations of <mfenced> and <mrow>+<mo> are different enough that writing an automated test is problematic.