Last Comment Bug 668969 - Empty mfenced elements should display fences
: Empty mfenced elements should display fences
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Florian Scholz [:fscholz] (MDN)
:
: Anthony Jones (:kentuckyfriedtakahe, :k17e)
Mentors:
Depends on:
Blocks: 557086 553918
  Show dependency treegraph
 
Reported: 2011-07-02 02:23 PDT by Florian Scholz [:fscholz] (MDN)
Modified: 2011-07-11 10:02 PDT (History)
3 users (show)
fred.wang: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix: Removes the if condition (2.26 KB, patch)
2011-07-02 05:27 PDT, Florian Scholz [:fscholz] (MDN)
karlt: review+
Details | Diff | Splinter Review

Description Florian Scholz [:fscholz] (MDN) 2011-07-02 02:23:34 PDT
(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) 2011-07-02 02:31:41 PDT
> 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 [:fscholz] (MDN) 2011-07-02 05:27:51 PDT
Created attachment 543603 [details] [diff] [review]
Fix: Removes the if condition
Comment 3 Frédéric Wang (:fredw) 2011-07-02 05:52:47 PDT
(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 (back Dec 13 :karlt) 2011-07-03 16:21:12 PDT
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 [:fscholz] (MDN) 2011-07-09 03:31:59 PDT
Filed bug 670334 to follow-up the height issue.
Comment 6 Karl Tomlinson (back Dec 13 :karlt) 2011-07-10 21:36:08 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/c262b7cdfd2f
Comment 7 Mounir Lamouri (:mounir) 2011-07-11 07:27:24 PDT
Merged:
http://hg.mozilla.org/mozilla-central/rev/c262b7cdfd2f
Comment 8 Mounir Lamouri (:mounir) 2011-07-11 07:28:21 PDT
Is this tested with tests in bug 553918?
Comment 9 Frédéric Wang (:fredw) 2011-07-11 10:02:36 PDT
(In reply to comment #8)
> Is this tested with tests in bug 553918?

Yes, it is.

Note You need to log in before you can comment on or make changes to this bug.