Closed Bug 975681 Opened 6 years ago Closed 6 years ago

Add tests for MathML mfrac

Categories

(Core :: MathML, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: anujagarwal464, Assigned: anujagarwal464)

References

Details

Attachments

(2 files, 4 obsolete files)

No description provided.
Blocks: 897065
Priority: -- → P5
Assignee: nobody → anujagarwal464
Attached patch bug975681.diff (obsolete) — Splinter Review
Attachment #8395354 - Flags: feedback?(fred.wang)
Comment on attachment 8395354 [details] [diff] [review]
bug975681.diff

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

Thanks Anuj. Tests look good, but as I told you for menclose, using == tests is often more accurate than !=. Could you try to see if you could import and adapt some tests from https://github.com/mathjax/MathJax-test/tree/master/testsuite/MathMLToDisplay/Presentation/GeneralLayout/mfrac? I think they could replace many of the mfrac-A* tests.

For dynamic tests, I guess you also want to test settting/adding attributes and adding/removing children, not just attribute removal.

Could you also try to write Mochitests (see layout/mathml/tests) based on the MathML Acid3 (search mfrac in https://github.com/fred-wang/AcidTestsMathML/blob/master/acid3/index.html)?
Attachment #8395354 - Flags: feedback?(fred.wang) → feedback+
Attached patch bug975681-reftests.diff (obsolete) — Splinter Review
I will add Mochitests in another patch.
Attachment #8395354 - Attachment is obsolete: true
Attachment #8397008 - Flags: feedback?(fred.wang)
Comment on attachment 8397008 [details] [diff] [review]
bug975681-reftests.diff

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

Thank you, that looks very good. 

r=me with mfrac-A-6.html removed.

> I will add Mochitests in another patch.

Yes, please upload a separate patch with only Mochitests, so that will be more convenient for review.

::: layout/reftests/mathml/mfrac-A-6.html
@@ +5,5 @@
> +  </head>
> +  <body>
> +    <div style="position: absolute; left: 20px; top: 20px; width: 200px; height: 34px; background: red;"></div>
> +  </body>
> +</html>

I'm not sure this "34px" value is really reliable. I'd prefer a mochitest for that one.
Attachment #8397008 - Flags: review+
Attachment #8397008 - Flags: feedback?(fred.wang)
Attachment #8397008 - Flags: feedback+
Summary: Add tests for mfrac → Add tests for MathML mfrac
Attached patch reftests for MathML mfrac (obsolete) — Splinter Review
updated the patch.
Attachment #8397008 - Attachment is obsolete: true
Attachment #8397823 - Flags: review?(fred.wang)
Attached patch mochitests for MathML mfrac (obsolete) — Splinter Review
Attachment #8397824 - Flags: review?(fred.wang)
Attachment #8397823 - Flags: review?(fred.wang) → review+
Comment on attachment 8397824 [details] [diff] [review]
mochitests for MathML mfrac

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

Thank you!

r=me with the comments below addressed.

::: layout/mathml/tests/test_bug975681.html
@@ +24,5 @@
> +  </p>
> +  
> +  <p>
> +    <math>
> +      <mfrac linethickness="2em" id="mfrac_linethickness">

I think this should be "px" rather than em so that it won't depend on the font-size. You should probably try a large enough thickness like "30px" so that it is larger than possible measurement errors.

@@ +100,5 @@
> +    var mfrac = document.getElementById("mfrac_linethickness").getBoundingClientRect();
> +    var num = document.getElementById("mfrac_linethickness").firstElementChild.getBoundingClientRect();
> +    var denom = document.getElementById("mfrac_linethickness").lastElementChild.getBoundingClientRect();
> +    
> +    ok(almostLessThanRel(num.height + 2 + denom.height, mfrac.height) && almostLessThanAbs(num.bottom + 2, denom.top), "numerator and denominator should be separated by linethickness")

I think you can just do

almostLessThan(num.height + 30 + denom.height, mfrac.height) and 
almostLessThan(num.bottom + 30 , denom.top)

The additional space between the bar and num/denum will even more help to pass the condition.

Then the *Rel functions can be removed.
Attachment #8397824 - Flags: review?(fred.wang)
Attachment #8397824 - Flags: review+
Attachment #8397824 - Flags: feedback+
updated tests. Thanks for help! :D
Attachment #8397824 - Attachment is obsolete: true
Attachment #8397864 - Flags: review?(fred.wang)
Attachment #8397864 - Flags: review?(fred.wang) → review+
mfrac-C-1 and mfrac-D-1 are failing on OS X [max difference: 1, number of differing pixels: 1]. I think it would be OK to mark them fuzzy.

mfrac-B-1 is failing on windows 7, don't know what to do with this test.
Flags: needinfo?(fred.wang)
(In reply to Anuj Agarwal [:anujagarwal464] from comment #10)
> mfrac-C-1 and mfrac-D-1 are failing on OS X [max difference: 1, number of
> differing pixels: 1]. I think it would be OK to mark them fuzzy.

OK.

> 
> mfrac-B-1 is failing on windows 7, don't know what to do with this test.

when I open reftest analyzer, the "B" denominator seems to be slightly shifted. I think using <mspace/> elements for num/denum might help.
Flags: needinfo?(fred.wang)
updated the mfrac-B-1 test.
Attachment #8397823 - Attachment is obsolete: true
Attachment #8398532 - Flags: review?(fred.wang)
Attachment #8398532 - Flags: review?(fred.wang) → review+
(In reply to Frédéric Wang (:fredw) from comment #13)
> https://tbpl.mozilla.org/?tree=Try&rev=44222f065450

All test passed! :D
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/742f05b9917b
https://hg.mozilla.org/mozilla-central/rev/9d0ede143f7c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
The following changesets are now in Firefox Nightly:

> 742f05b9917b Bug 975681 - Mochitest for MathML mfrac. r=fredw
> 9d0ede143f7c Bug 975681 - Reftests for MathML mfrac. r=fredw

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central
You need to log in before you can comment on or make changes to this bug.