Add more reftests for mfenced

RESOLVED FIXED in mozilla8

Status

()

Core
MathML
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: fredw, Assigned: fscholz)

Tracking

Trunk
mozilla8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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
(Assignee)

Comment 1

6 years ago
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
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Updated

6 years ago
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.
(Reporter)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
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]) ?
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
(Reporter)

Updated

6 years ago
Keywords: checkin-needed
Need to mark mfenced-10 failing on windows.
Keywords: checkin-needed
http://tbpl.mozilla.org/?tree=Try&rev=5d9433610cac
Depends on: 670592
http://hg.mozilla.org/integration/mozilla-inbound/rev/c8835712cf1b
Whiteboard: [good first bug] → [inbound]
Merged:
http://hg.mozilla.org/mozilla-central/rev/c8835712cf1b
Status: NEW → RESOLVED
Last Resolved: 6 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.