963453 - Add more tests for menclose

Status: RESOLVED FIXED

(Core :: MathML, defect, P5)

Description

[Frédéric Wang (:fredw)]

Try to import tests from

- MathJax: https://github.com/mathjax/MathJax-test/tree/master/testsuite/MathMLToDisplay/Presentation/GeneralLayout/menclose
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=85729

and add more tests.

Comment 1

[Frédéric Wang (:fredw)]

@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.

Comment 2

[Anuj Agarwal [: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.

Comment 3

[Frédéric Wang (:fredw)]

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.

Comment 4

[Anuj Agarwal [:anujagarwal464]]

Attachment: reftest for menclose-2* and menclose-3

Comment 5

[Anuj Agarwal [:anujagarwal464]]

@fredw

need help in writing menclose-4 test.

Comment 6

[Frédéric Wang (:fredw)]

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?

Comment 7

[Anuj Agarwal [:anujagarwal464]]

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.

Comment 8

[Anuj Agarwal [:anujagarwal464]]

Attachment: reftest for menclose-2* menclose-3* menclose-5*

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.

Comment 9

[Anuj Agarwal [:anujagarwal464]]

Attachment: reftest for menclose-2* menclose-3* menclose-4 menclose-5*

Comment 10

[Frédéric Wang (:fredw)]

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.

Comment 11

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug963453_reftest.diff

reftest for "removing a attribute" are named as menclose-6*

Comment 12

[Frédéric Wang (:fredw)]

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.

Comment 13

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug963453_reftest.diff

fixed spaces in reftest.list

Comment 14

[Frédéric Wang (:fredw)]

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.

Comment 15

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug963453_reftest.diff

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!

Comment 16

[Frédéric Wang (:fredw)]

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?

Comment 17

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug963453_reftest.diff

Comment 18

[Frédéric Wang (:fredw)]

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".

Comment 19

[Anuj Agarwal [:anujagarwal464]]

Attachment: 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
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 ?

Comment 20

[Frédéric Wang (:fredw)]

(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.

Comment 21

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug963453_reftest.diff

added shape-rendering: crispEdges; in menclose-2-circle

Comment 22

[Frédéric Wang (:fredw)]

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

Comment 23

[Anuj Agarwal [:anujagarwal464]]

sorry, I will fix that.

Comment 24

[Frédéric Wang (:fredw)]

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)

Comment 25

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug963453_reftest.diff

1. decreased stroke width
2. added fails-if condition for windows 
3. added fuzzy-if condition osx

Comment 26

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug963453_reftest.diff

added random-if for windows

Comment 27

[Frédéric Wang (:fredw)]

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

Comment 28

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #27)
> https://tbpl.mozilla.org/?tree=Try&rev=e7f4b31a99af

wrong patch :-(
https://tbpl.mozilla.org/?tree=Try&rev=72ebf29743ea

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4146284b29b8

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4146284b29b8