Closed Bug 670334 Opened 9 years ago Closed 5 years ago

Empty mfenced elements should display normal size of fences

Categories

(Core :: MathML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: fscholz, Assigned: jkitch)

References

Details

Attachments

(3 files, 3 obsolete files)

Attached file 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.
@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.
Attached patch Patch V1 (obsolete) — Splinter Review
(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.
Assignee: nobody → elchi3
Status: NEW → ASSIGNED
Attachment #612902 - Flags: review?(karlt)
(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.
Attached patch Patch V1.1 (obsolete) — Splinter Review
Attachment #612902 - Attachment is obsolete: true
Attachment #613274 - Flags: review?(karlt)
Attachment #612902 - Flags: review?(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?
Attachment #613274 - Flags: review?(karlt) → review-
> 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
(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.
Assignee: elchi3 → nobody
Status: ASSIGNED → NEW
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.
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...
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.
I can still reproduce this bug on Windows with the latest Nightly.
Assignee: nobody → jkitch.bug
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #613274 - Attachment is obsolete: true
Attachment #8503749 - Flags: review?(karlt)
Comment on attachment 8503749 [details] [diff] [review]
patch

>+ApplyDefaultMetrics(nsPresContext*      aPresContext,

What do you think about calling this "ApplyUnstretchedMetrics"?
Attachment #8503749 - Flags: review?(karlt) → review+
Attached patch patchSplinter Review
Renamed
Attachment #8503749 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/27bc3dbcaff5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Does this bug improves the situation with bug 727804?
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.
Blocks: 727804
You need to log in before you can comment on or make changes to this bug.