Last Comment Bug 553918 - Add more reftests for mfenced
: Add more reftests for mfenced
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:
http://www.w3.org/TR/MathML3/chapter3...
Depends on: 670592 538965 668969
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-21 11:24 PDT by Frédéric Wang (:fredw)
Modified: 2011-07-11 07:28 PDT (History)
2 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reftests (6.41 KB, patch)
2011-07-01 13:01 PDT, Florian Scholz [:fscholz] (MDN)
no flags Details | Diff | Splinter Review
Reftests V2 (5.96 KB, patch)
2011-07-06 10:30 PDT, Florian Scholz [:fscholz] (MDN)
karlt: review+
Details | Diff | Splinter Review

Description Frédéric Wang (:fredw) 2010-03-21 11:24:25 PDT
Some bugs have recently been found regarding attributes of <mfenced/>. The purpose of this bug is to add reftests and check that our implementation follows the MathML Spec, in particular the equivalence with the expanded form and limit cases. Below are some ideas of reftests:

1) whitespaces within separators are ignored - see bug 537916 
2) whitespaces in open/close attributes are collapsed - see bug 538965 
3) too many separators: the excess are ignored
4) too few separators: the last separator is repeated
6) defaults values for open, close and separators
5) empty separators
7) stretchy fences used in open/close are stretched like if they were <mo/>'s
Comment 1 Florian Scholz [:fscholz] (MDN) 2011-07-01 13:01:05 PDT
Created attachment 543487 [details] [diff] [review]
Reftests

I think I addressed your ideas and those tests are passing.

However mfenced-9.html is failing:

The MathML3 REC says "In general, an mfenced element can contain zero or more arguments[...]". So, an mfenced element with no arguments should still display fences. (Which is currently not the case in our implementation. See also the MathML testsuite [1] and a comment in our code [2].)

[1] http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/mfenced/mfenced1-full.xhtml
[2] http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmfencedFrame.cpp#354
Comment 2 Frédéric Wang (:fredw) 2011-07-01 14:11:50 PDT
Thanks Florian, that looks great. I don't know if it is usual to take failing reftests without a patch fixing the bug tested. Karl, what do you think about that?

Anyway, we can open a new bug about the fact that empty mfenced should display fences.
Comment 3 Karl Tomlinson (back Dec 13 :karlt) 2011-07-03 15:37:43 PDT
Often enough, we do add failing reftests for bugs in existing features, which is probably the situation here.  (We probably wouldn't add tests for non-existant features.)  It is helpful to link the bug report number (bug 668969) in a comment beside the "fails" line in the reftest.list.
Comment 4 Karl Tomlinson (back Dec 13 :karlt) 2011-07-03 15:55:29 PDT
Comment on attachment 543487 [details] [diff] [review]
Reftests

>diff -r f999ca02a687 layout/reftests/mathml/mfenced-9-ref.html
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/layout/reftests/mathml/mfenced-9-ref.html	Fri Jul 01 20:55:33 2011 +0200
>@@ -0,0 +1,14 @@
>+<!DOCTYPE HTML>
>+<html>
>+  <body>
>+
>+    <!-- No arguments -->
>+
>+    <math>
>+      <mfenced>
>+        <mi></mi> 
>+      </mfenced>

Is it reasonable to expect that an empty mi child takes up no space?
Perhaps it is, but don't we really want to ensure that <mfenced/> is equivalent to this?:

<mrow>
  <mo>(</mo>
  <mo>)</mo>
</mrow>
Comment 5 Karl Tomlinson (back Dec 13 :karlt) 2011-07-03 16:22:58 PDT
If it is reasonable to expect that an empty mi takes up no space, then I'm fine with the test as is, for now at least, because it tests what attachment 543603 [details] [diff] [review] fixes.
Comment 6 Frédéric Wang (:fredw) 2011-07-03 22:25:14 PDT
I think it is reasonable to expect that an empty mi child takes up no space:

"A typical graphical renderer would render an mi element as its content (See Section 3.2.1 MathML characters in token elements), with no extra spacing around it (except spacing associated with neighboring elements)."

except in the case of an editor

"An mi element with no content is allowed; <mi></mi> might, for example, be used by an "expression editor" to represent a location in a MathML expression which requires a "term" (according to conventional syntax for mathematics) but does not yet contain one."

If we want to be sure to have no space, we can use an <mspace/> instead. Or maybe just test

<mrow>
  <mo>(</mo>
  <mo>)</mo>
</mrow>

as you suggested.
Comment 7 Florian Scholz [:fscholz] (MDN) 2011-07-06 10:30:09 PDT
Created attachment 544284 [details] [diff] [review]
Reftests V2

(In reply to comment #3)
> Often enough, we do add failing reftests for bugs in existing features,
> which is probably the situation here.  (We probably wouldn't add tests for
> non-existant features.)  It is helpful to link the bug report number (bug
> 668969) in a comment beside the "fails" line in the reftest.list.

Good to know, but I guess next time I will just open a new bug with an attached reftest to have the discussion there.

(In reply to comment #6)
> If we want to be sure to have no space, we can use an <mspace/> instead. Or
> maybe just test
> 
> <mrow>
>   <mo>(</mo>
>   <mo>)</mo>
> </mrow>
> 
> as you suggested.

I've updated the patch to use an <mpspace /> and no longer marked it as failing, because attachment 543603 [details] [diff] [review] is supposed to fix it. 

However, as said in bug 668969#c4, the height of the fences is not correct and we likely want that to look it like:
<mrow>
  <mo>(</mo>
  <mo>)</mo>
</mrow>

I've looked into nsMathMLChar.cpp then but can't figure out how to fix the sizing.
So, do we want to track the sizing issue in a new bug (with the <mo>(</mo><mo>)</mo> reftest, which currently fails with the fix in attachment 543603 [details] [diff] [review]) ?
Comment 8 Karl Tomlinson (back Dec 13 :karlt) 2011-07-06 20:30:06 PDT
(In reply to comment #7)
> I've looked into nsMathMLChar.cpp then but can't figure out how to fix the
> sizing.
> So, do we want to track the sizing issue in a new bug (with the
> <mo>(</mo><mo>)</mo> reftest, which currently fails with the fix in
> attachment 543603 [details] [diff] [review] [review]) ?

Yes, a new bug, please.  I think nsMathMLmfencedFrame::Reflow needs to get the normal size of the fences and consider them when setting containerSize.
Comment 9 Karl Tomlinson (back Dec 13 :karlt) 2011-07-08 00:20:56 PDT
Need to mark mfenced-10 failing on windows.
Comment 10 Karl Tomlinson (back Dec 13 :karlt) 2011-07-08 00:21:09 PDT
http://tbpl.mozilla.org/?tree=Try&rev=5d9433610cac
Comment 11 Karl Tomlinson (back Dec 13 :karlt) 2011-07-10 21:37:22 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/c8835712cf1b
Comment 12 Mounir Lamouri (:mounir) 2011-07-11 07:28:05 PDT
Merged:
http://hg.mozilla.org/mozilla-central/rev/c8835712cf1b

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