Closed Bug 553918 Opened 14 years ago Closed 13 years ago

Add more reftests for mfenced

Categories

(Core :: MathML, defect)

Product:
Core
Component:
MathML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla8

People

(Reporter: fredw, Assigned: fs)

References

(
URL
)

Details

Attachments

(1 file, 1 obsolete file)

Reftests V2
5.96 KB, patch
karlt
: review+
Details | Diff | Splinter Review 
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
Depends on: 538965
Attached patch Reftests (obsolete) — — Splinter Review 
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
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.
Assignee: nobody → elchi3
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 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>
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.
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.
Attached patch Reftests V2 — — Splinter Review 
(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]) ?
Attachment #543487 - Attachment is obsolete: true
Attachment #544284 - Flags: review?(karlt)
Attachment #544284 - Flags: review?(karlt) → review+
(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.
Depends on: 668969
Keywords: checkin-needed
Need to mark mfenced-10 failing on windows.
Keywords: checkin-needed
Depends on: 670592
Merged:
http://hg.mozilla.org/mozilla-central/rev/c8835712cf1b
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: