668969 - Empty mfenced elements should display fences

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Florian Scholz (Open Web Docs)]

(Follow-up of bug 553918 comment1)

Testsuite test: http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/mfenced/mfenced1-full.xhtml

I already provided a reftest which currently fails (mfenced-9): https://bug553918.bugzilla.mozilla.org/attachment.cgi?id=543487

Perhaps we can just remove the if statement here?
http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmfencedFrame.cpp#354

Comment 1

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

> Perhaps we can just remove the if statement here?
> http://mxr.mozilla.org/mozilla-central/source/layout/mathml/
> nsMathMLmfencedFrame.cpp#354

Yes, I think I will work. We just have to verified that we won't use null Frame pointer or similar.

Comment 2

[Florian Scholz (Open Web Docs)]

Attachment: Fix: Removes the if condition

Comment 3

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

(In reply to comment #2)
> Created attachment 543603 [details] [diff] [review] [review]
> Fix: Removes the if condition

Did you test it? I think I read it too quickly this morning, and thought this if statement was to leave the function when the mfenced is empty.

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 543603 [details] [diff] [review]
Fix: Removes the if condition

Yes, I agree that the empty mfenced should not be handled specially here.

The centering logic happens to provide additional size so that the fences become visible.  This is still not quite exactly how they should be, but is definitely better.

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.

This patch is at least part of the solution and good enough for this bug.

Comment 5

[Florian Scholz (Open Web Docs)]

Filed bug 670334 to follow-up the height issue.

Comment 6

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c262b7cdfd2f

Comment 7

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/c262b7cdfd2f

Comment 8

[Mounir Lamouri (:mounir)]

Is this tested with tests in bug 553918?

Comment 9

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

(In reply to comment #8)
> Is this tested with tests in bug 553918?

Yes, it is.