Closed Bug 963453 Opened 6 years ago Closed 6 years ago

Add more tests for menclose

Categories

(Core :: MathML, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: fredw, Assigned: anujagarwal464)

References

()

Details

Attachments

(1 file, 10 obsolete files)

Priority: -- → P5
@Anuj:

The idea is to import some reftests from MathJax:
https://github.com/mathjax/MathJax-test/tree/master/testsuite/MathMLToDisplay/Presentation/GeneralLayout/menclose
or from WebKit:
http://trac.webkit.org/browser/trunk/LayoutTests/mathml/presentation/

The menclose-1*.html tests are already imported: http://hg.mozilla.org/mozilla-central/file/a62bde1d6efe/layout/reftests/mathml

Note that you will need to cleanup or adapt some code (for example remove the header.js or don't make the reftest "reftest-wait" when unnecessary). See how this has been done for menclose-1*.html

menclose-3 compare notation="box" with notation="left top right bottom" and can just be a simple == test. A similar test could be <msqrt> VS <menclose notation="radical">.

menclose-4 tests that the circle does not overlap its content so that the size is increased by a sqrt(2) factor (see https://bug-85729-attachments.webkit.org/attachment.cgi?id=220708). I think this can just be done by measuring metrics. See reftests/canvas/evenodd-fill-sanity.html and test 6 of http://fred-wang.github.io/AcidTestsMathML/acid3/. This is menclose-notation-no-overlap.html in WebKit.

For menclose-2* the idea is to draw an menclose element with a notation and to cover it by drawing some SVG above. The reference is the same but without any menclose notation. If the notation is drawn at the expected place, then it will be covered by the SVG thus invisible, and so the page will render the same as the reference. Note that the SVG is drawn using some Javascript, so it will be like the dynamic reftests you wrote.

Other tests in WebKit is to try various DOM op (adding/removing/setting children or attributes) and to compare with a static page. This is similar to the dynamic reftests you wrote. I think you can write your own tests rather than copying what WebKit does, here.

WebKit also has tests to check the behavior of the default notation (should be longdiv) or invalid/unknown notations. I think you should also try to test multiple notations (perhaps repeated). Again, I think you can write your own tests.
Assignee: nobody → anujagarwal464
Here link for menclose-2* and menclose-3 reftest i wrote
https://github.com/dtr12/mathml-reftest

Please review it and tell me if they are OK.
Thanks Anuj. You must attach patches and follow the Mozilla procedure to get them reviewed:

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

However, if your patch is not finished yet and just want to get some feedback on it, you can attach it to the bug and use the "feedback" flag.
Attachment #8376371 - Flags: feedback?(fred.wang)
@fredw

need help in writing menclose-4 test.
Comment on attachment 8376371 [details] [diff] [review]
reftest for menclose-2* and menclose-3

Review of attachment 8376371 [details] [diff] [review]:
-----------------------------------------------------------------

Can you check that all the <div>'s have coordinates "20px" and can you make the stroke-width as small as possible? Have you tried to check either manually (or better by running the tests) that all the tests match their references?

For menclose-4, you can try to do as in menclose-2-circle:

<menclose id="outer" notation="circle">
<mspace id="inner" width="200px" height="100px"></mspace>
</menclose>

then measure the boxes of the outer and inner frames via getBoundingClientRect(). Check that "outer width" >= Math.sqrt(2) * "inner width" and similarly for the heights. If both are true, document.body.innerHTML = "Pass". The reference can be indicated in the reftest.list. See

http://mxr.mozilla.org/mozilla-central/source/layout/reftests/canvas/evenodd-fill-sanity.html
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/canvas/reftest.list#74

For menclose-5*, do the same as in bug 970977 but with menclose children/attributes.

The tests for the default/invalid/unknown/multiple attributes can be in menclose-3*. For example menclose-3-default can compare <menclose> without attribute against <menclose notation="longdiv">. Or can compare <menclose notation="top top left circle"> against <menclose notation="circle left top">.

::: layout/reftests/mathml/menclose-2-actuarial.html
@@ +32,5 @@
> +      </svg>
> +    </div>
> +    
> +  </body>
> +</html>
\ No newline at end of file

I think this one can be replaced with a menclose-3-actuarial test that compares

"actuarial" against "top left"

::: layout/reftests/mathml/menclose-2-bottom-ref.html
@@ +26,5 @@
> +        </mphantom>
> +      </math>
> +    </div>
> +
> +    <div style="position: absolute; left: 0px; top: 0px;">

20px

::: layout/reftests/mathml/menclose-2-bottom.html
@@ +24,5 @@
> +        </menclose>
> +      </math>
> +    </div>
> +
> +    <div style="position: absolute; left: 0px; top: 0px;">

20px

::: layout/reftests/mathml/menclose-2-box.html
@@ +33,5 @@
> +      </svg>
> +    </div>
> +
> +  </body>
> +</html>
\ No newline at end of file

I think this test can be removed since it is verified by your menclose-3 test.

::: layout/reftests/mathml/menclose-2-circle.html
@@ +25,5 @@
> +        </menclose>
> +      </math>
> +    </div>
> +
> +    <div style="position: absolute; left: 0px; top: 0px;">

should be 20px

::: layout/reftests/mathml/menclose-2-diagonalstrike-ref.html
@@ +26,5 @@
> +       </mphantom> 
> +      </math>
> +    </div>
> +
> +    <div style="position: absolute; left: 0px; top: 0px;">

should be 2Opx

::: layout/reftests/mathml/menclose-2-diagonalstrike.html
@@ +24,5 @@
> +        </menclose>
> +      </math>
> +    </div>
> +
> +    <div style="position: absolute; left: 0px; top: 0px;">

should be 20px

::: layout/reftests/mathml/menclose-2-horizontalstrike.html
@@ +24,5 @@
> +        </menclose>
> +      </math>
> +    </div>
> +
> +    <div style="position: absolute; left: 0px; top: 0px;">

should be 20px

::: layout/reftests/mathml/menclose-2-left.html
@@ +24,5 @@
> +        </menclose>
> +      </math>
> +    </div>
> +
> +    <div style="position: absolute; left: 0px; top: 0px;">

is should be 20px

::: layout/reftests/mathml/menclose-2-longdiv.html
@@ +29,5 @@
> +    <div style="position: absolute; left: 18px; top: 18px;">
> +      <svg width="500px" height="500px">
> +        <path id="path" style="fill: none; stroke-width: 13px; stroke: green; stroke-linecap: round;"></path>
> +      </svg>
> +    </div>

I think the coordinates are not good. The div should have coordinates "20px; top: 20px". I don't think those in doTest() are correct either (as a rule of thumb: if you replace "20px" by "100px", the test should still pass).

::: layout/reftests/mathml/menclose-2-madruwb.html
@@ +9,5 @@
> +        var box = document.getElementById("box").getBoundingClientRect();
> +        document.getElementById("path").setAttribute("d",
> +          "M" + (box.width + ",0") +
> +          "L" + (box.width + "," + box.height) +
> +          "L" + ("0," + box.height) );

I'm not sure the coordinates are correct, it should be

M (box.left + box.width), box.top
l 0, box.height
l -box.width, 0

However, I think you can just remove that menclose-2-madruwb test and replace it with menclose-3-madruwb that compares "madruwb" with "left bottom".

::: layout/reftests/mathml/menclose-2-radical.html
@@ +12,5 @@
> +        <menclose notation="radical">
> +          <mspace width="100px" height="100px"></mspace>
> +        </menclose>
> +      </math>
> +    </div>

I think menclose-2-radical.html can just be dropped: it is just menclose-3-radical.

::: layout/reftests/mathml/menclose-2-roundedbox.html
@@ +28,5 @@
> +    </div>
> +
> +    <div style="position: absolute; left: 0px; top: 0px;">
> +      <svg width="500px" height="500px">
> +          <rect id="rect" style="fill: none; stroke-width: 7px; stroke: green; stroke-linecap: round;"></rect>

I'm wondering if this could be replaced by a normal CSS div with thiner stroke-width but corners rounded via border-radius:
https://developer.mozilla.org/en-US/docs/Web/CSS/border-radius

::: layout/reftests/mathml/menclose-2-updiagonalarrow.html
@@ +8,5 @@
> +      {
> +        var box = document.getElementById("box").getBoundingClientRect();
> +        document.getElementById("path").setAttribute("d",
> +          "M" + ("0," + box.height) +
> +          "L" + (box.width + ",0") );

Shouldn't it be like updiagonalstrike? (with the thicker stroke width, as you used below or anything else to take into account the size of the arrow head)

::: layout/reftests/mathml/menclose-3-radical-ref.html
@@ +7,5 @@
> +
> +  <body>
> +    <!-- menclose: radical -->
> +    <math>
> +      <menclose notation="radical">x</menclose>

This is not valid. "x" must be inside a token element for example <mi>x</mi>. But perhaps you can just use an <mspace> like in your menclose-3 test.

::: layout/reftests/mathml/menclose-3.html
@@ +13,5 @@
> +      </menclose>
> +    </math>
> +
> +  </body>
> +</html>
\ No newline at end of file

Perhaps it should be called menclose-3-box?
Attachment #8376371 - Flags: feedback?(fred.wang) → feedback+
I ran all those tests and they matched their references.
First I kept stroke-width = 3px. When some test failed I increased stroke-width accordingly.
1. I think menclose-2-rounded box test should be written in svg with thicker stroke.I have included css test in this patch.

2. menclose-4 test is not included in patch because it is failing due to precision .It will pass if I take sqrt(2) = 1.41.
Attachment #8376838 - Flags: feedback?(fred.wang)
Attachment #8376838 - Attachment is obsolete: true
Attachment #8376838 - Flags: feedback?(fred.wang)
Attachment #8376898 - Flags: review?(fred.wang)
Comment on attachment 8376898 [details] [diff] [review]
reftest for menclose-2* menclose-3* menclose-4 menclose-5*

Review of attachment 8376898 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks that looks good. For the dynamic reftests, I only see "setting an attribute". I think you could test changing the value of an attribute or removing an attribute. Similarly, you could try adding/removing children from the menclose element. Note that you don't necessarily need to test all the notations, only to have one test for each dynamic operation.

Can you do please add menclose-3* tests for default/invalid/unknown/multiple attributes?

::: layout/reftests/mathml/menclose-2-diagonalstrike.html
@@ +1,1 @@
> +<!doctype html>

Should be renamed "downdiagonalstrike".

::: layout/reftests/mathml/menclose-2-roundedbox-ref.html
@@ +8,5 @@
> +      function doTest()
> +      {
> +        var box = document.getElementById("roundedbox").getBoundingClientRect();
> +        r = document.getElementById("box");
> +        r.style.border = "10px solid #000";

This one can directly be set on the <div>

@@ +29,5 @@
> +        </mphantom> 
> +      </math>
> +    </div>
> +
> +    <div id="box" style="position: absolute; left: 0px; top: 0px; border-radius:10px 10px 10px 10px;" >

No need to set left, top since they are reset on doTest.

::: layout/reftests/mathml/menclose-4.html
@@ +8,5 @@
> +      {
> +        var box1 = document.getElementById("outer").getBoundingClientRect();
> +        var box2 = document.getElementById("inner").getBoundingClientRect();
> +        if (box1.width >= ((Math.sqrt(2) - 0.1)*box2.width)){
> +          if(box1.height >= ((Math.sqrt(2) - 0.1)*box2.height)){

This can be in one if condition (use &&). Also, you can use a local epsilon = 0.1 variable so that it is more convenient to update if necessary.
Attachment #8376898 - Flags: review?(fred.wang) → feedback+
Attached patch bug963453_reftest.diff (obsolete) — Splinter Review
reftest for "removing a attribute" are named as menclose-6*
Attachment #8377174 - Flags: feedback?(fred.wang)
Attachment #8376371 - Attachment is obsolete: true
Attachment #8376898 - Attachment is obsolete: true
Comment on attachment 8377174 [details] [diff] [review]
bug963453_reftest.diff

Review of attachment 8377174 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, that looks great. I've submitted your patch to the test server to see if they pass on all platforms:
https://tbpl.mozilla.org/?tree=Try&rev=9ee991510b6b

::: layout/reftests/mathml/reftest.list
@@ +243,5 @@
> +== menclose-6-roundedbox.html  menclose-6-ref.html
> +== menclose-6-top.html  menclose-6-ref.html
> +== menclose-6-updiagonalarrow.html  menclose-6-ref.html
> +== menclose-6-updiagonalstrike.html  menclose-6-ref.html
> +== menclose-6-verticalstrike.html  menclose-6-ref.html
\ No newline at end of file

I think you only need one space between the page and its reference.
Attachment #8377174 - Flags: feedback?(fred.wang) → feedback+
Attached patch bug963453_reftest.diff (obsolete) — Splinter Review
fixed spaces in reftest.list
Comment on attachment 8377214 [details] [diff] [review]
bug963453_reftest.diff

Review of attachment 8377214 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/mathml/menclose-5-circle.html
@@ +11,5 @@
> +	    </script>
> +	</head>
> +	<body>
> +	    <p>
> +	       <math><menclose id="testMenclose"><mspace width="100px" height="50px"></menclose></math>

Not sure if that's the reason of the test failure, but that <mspace> element is not closed.
Attached patch bug963453_reftest.diff (obsolete) — Splinter Review
1. Increased stroke width by 3px in tests which were failing
2. Fixed reftest conditions

I still don't have commit access to try server. Can you please commit it!
Attachment #8377604 - Flags: feedback?(fred.wang)
https://tbpl.mozilla.org/?tree=Try&rev=7b2411e35c76

I had to fix conflicts in reftest.list. Can you update your mozilla-central branch to ensure that you have the menclose-1* files and then refresh your patch?
Attached patch bug963453_reftest.diff (obsolete) — Splinter Review
Attachment #8377214 - Attachment is obsolete: true
Attachment #8377604 - Attachment is obsolete: true
Attachment #8377604 - Flags: feedback?(fred.wang)
Attachment #8378056 - Flags: review?(fred.wang)
Comment on attachment 8378056 [details] [diff] [review]
bug963453_reftest.diff

Review of attachment 8378056 [details] [diff] [review]:
-----------------------------------------------------------------

I see that you modified the thickness of menclose-2-circle.html. Can you also update reftest.list to move the new menclose-* after the menclose-1 tests (refresh your mozilla-central if necessary).

For some reason, the test failures of menclose-3-radical.html appeared on Windows but I'm not sure why since you marked it "fails".
Attached patch bug963453_reftest.diff (obsolete) — Splinter Review
I increased thickness of menclose-2-circle because it was failing on OSX. If it still fails i will add a fails-if condition in reftest.list
menclose-3-radical passes on reftest opt but fails on reftest unaccelerated opt on windows. Can I some how add an exception for that in reftest.list ?
Attachment #8378056 - Attachment is obsolete: true
Attachment #8378056 - Flags: review?(fred.wang)
Attachment #8378178 - Flags: feedback?(fred.wang)
(In reply to Anuj Agarwal [:anujagarwal464] from comment #19)
> Created attachment 8378178 [details] [diff] [review]
> bug963453_reftest.diff
> 
> I increased thickness of menclose-2-circle because it was failing on OSX. If
> it still fails i will add a fails-if condition in reftest.list

Can you try to use crispEdges to see if that gives better result?
https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/shape-rendering

> menclose-3-radical passes on reftest opt but fails on reftest unaccelerated
> opt on windows. Can I some how add an exception for that in reftest.list ?

I don't see where you see the tests passes. I think you already marked the whole test "fails" so I don't think why it was reported in the last result. Let's try again.
Attached patch bug963453_reftest.diff (obsolete) — Splinter Review
added shape-rendering: crispEdges; in menclose-2-circle
Attachment #8378178 - Attachment is obsolete: true
Attachment #8378178 - Flags: feedback?(fred.wang)
Attachment #8378208 - Flags: feedback?(fred.wang)
Comment on attachment 8378208 [details] [diff] [review]
bug963453_reftest.diff

Review of attachment 8378208 [details] [diff] [review]:
-----------------------------------------------------------------

https://tbpl.mozilla.org/?tree=Try&rev=4ce5d9ae4075

::: layout/reftests/mathml/reftest.list
@@ +189,5 @@
> +== menclose-3-madruwb.html menclose-3-madruwb-ref.html
> +fails == menclose-3-radical.html menclose-3-radical-ref.html # Bug 973917
> +== menclose-3-default.html menclose-3-default-ref.html
> +== menclose-3-invalid.html menclose-3-invalid-ref.html
> +== menclose-3-multiple.html	menclose-3-multiple-ref.html

it seems that you added spaces again
Attachment #8378208 - Flags: feedback?(fred.wang) → feedback+
sorry, I will fix that.
The crispEdges value seems to have solved the problem on Mac. I think we should consider again the result  of Comment 12. Rather than trying to increase the thickness, using crispEdges on the failing tests (and probably everywhere) will solve these small pixel differences. That won't work for the menclose-5-circle.html failure, but we can relax the failure condition by making the test "fuzzy" on Mac (http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.txt#109)
Blocks: 914031
Attached patch bug963453_reftest.diff (obsolete) — Splinter Review
1. decreased stroke width
2. added fails-if condition for windows 
3. added fuzzy-if condition osx
Attachment #8378847 - Flags: feedback?(fred.wang)
added random-if for windows
Attachment #8378847 - Attachment is obsolete: true
Attachment #8378847 - Flags: feedback?(fred.wang)
Attachment #8379018 - Flags: review?(fred.wang)
Attachment #8377174 - Attachment is obsolete: true
Attachment #8378208 - Attachment is obsolete: true
Comment on attachment 8379018 [details] [diff] [review]
bug963453_reftest.diff

Looks good. Please wait the result of the try server before asking checkin-needed. https://tbpl.mozilla.org/?tree=Try&rev=e7f4b31a99af
Attachment #8379018 - Flags: review?(fred.wang) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4146284b29b8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.