1240799 - mfrac elements without bar do not use axis height

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

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

See the AxisHeight test on http://tests.mathml-association.org/mathml/presentation-markup/fractions/frac-parameters-2.html

(and compare with http://tests.mathml-association.org/mathml/presentation-markup/fractions/frac-parameters-1.html)

Comment 1

[James Kitchener (:jkitch)]

https://dxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmfracFrame.cpp#323

In m-c, axis height is considered only when actualRuleThickness != 0.

I've looked at the relevant section in the TeXbook, and there is no mention of axis height within the == 0 rule (15c App G.)

PDF page 3 of 
http://www.tug.org/~vieth/papers/bachotex2008/math-font-paper.pdf
seems to be covering the 15c case and suggests that simply adding the axis height to numShift and demShift is sufficient.  Doing so is enough to get the testcase in the description to pass.

Comment 2

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

(In reply to James Kitchener (:jkitch) from comment #1)
> https://dxr.mozilla.org/mozilla-central/source/layout/mathml/
> nsMathMLmfracFrame.cpp#323
> 
> In m-c, axis height is considered only when actualRuleThickness != 0.
> 
> I've looked at the relevant section in the TeXbook, and there is no mention
> of axis height within the == 0 rule (15c App G.)
> 
> PDF page 3 of 
> http://www.tug.org/~vieth/papers/bachotex2008/math-font-paper.pdf
> seems to be covering the 15c case and suggests that simply adding the axis
> height to numShift and demShift is sufficient.  Doing so is enough to get
> the testcase in the description to pass.

Yes, this is the pdf I've used to create the testcase as well as the implementation note (see http://www.mathml-association.org/MathMLinHTML5/S3.html#SS3.SSS2).

I'm asking confirmation to Khaled but otherwise we can just go and shift the stack by axis height.

Comment 3

[Khaled Hosny]

I have hard time figuring what this issues is about (the test files does not tell much), but if it is about centreing math fractions around math axis, then yes it should be done whether a fraction bar is drawn or not.

Comment 4

[James Kitchener (:jkitch)]

Attachment: patch

The change described in comment 1 is applied.

Testcases, including fonts, are a modified version of the ones listed in the description.  (The test harnesses have been harness scripts have been replaced with small functions sufficient to get the reftest to fail and explain why).

Comment 5

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

(In reply to James Kitchener (:jkitch) from comment #4)
> Created attachment 8719117 [details] [diff] [review]
> patch
> 
> The change described in comment 1 is applied.
> 
> Testcases, including fonts, are a modified version of the ones listed in the
> description.  (The test harnesses have been harness scripts have been
> replaced with small functions sufficient to get the reftest to fail and
> explain why).

Thanks James. I believe other tests in mozilla-central use the testharness files from http://testthewebforward.org/docs/testharness.html so if possible I'd prefer to keep it here too. And more generally to use the testharness for all our MathML "script tests". That will make easier to import tests from http://tests.mathml-association.org or to share with other Web rendering engines. And will avoid us to redefine these small functions each time we want to write a script test. So I'm going to open a separate bug for that. I actually recently did it for WebKit (https://bugs.webkit.org/show_bug.cgi?id=154065).

I'm curious why we need to change the fonts, though.

Comment 6

[James Kitchener (:jkitch)]

Comment on attachment 8719117 [details] [diff] [review]
patch

Other than the change to relative addressing, I was not aware that I made any changes.  I'll look into what changed and if there was a need for it.

Comment 7

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

(In reply to James Kitchener (:jkitch) from comment #6)
> Other than the change to relative addressing, I was not aware that I made
> any changes.  I'll look into what changed and if there was a need for it.

OK, sorry I misunderstood what you said here and I thought the modifications applied to fonts too:

(In reply to James Kitchener (:jkitch) from comment #4)
> Testcases, including fonts, are a modified version of the ones listed in the
> description.

Comment 8

[James Kitchener (:jkitch)]

Attachment: patch

Feedback from Comment 5 applied.

In summary: The change described in comment 1 and test tests linked in the description, marginally modified.

Modifications of test test files are limited to changing the path of fonts and adding a copyright notice.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8720232 [details] [diff] [review]
patch

Would you be able to review this please, Frédéric.
(I'm not familiar with this code.)

Comment 10

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

Comment on attachment 8720232 [details] [diff] [review]
patch

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

Looks good to me, thanks.

Maybe we can merge the license into a single file... But I wonder how we do that for other tests imported from https://github.com/w3c/web-platform-tests ?

::: layout/mathml/tests/fonts/stack-license.txt
@@ +5,5 @@
> +stack-gapmin8000.woff
> +stack-topdisplaystyleshiftup3000.woff
> +stack-topshiftup9000.woff
> +test_stack-parameters.html
> +

../ and only one new line to be consistent with fraction-license.txt

::: layout/mathml/tests/mochitest.ini
@@ +1,3 @@
>  [DEFAULT]
>  
> +

why this new line?

Comment 11

[James Kitchener (:jkitch)]

Attachment: alternative patch

The approach used for the W3C tests is to give them their own directory and use 'wildcarded' expressions to summaries copyright/ownership.  Isolated tests are generally intermingled, but I expect that more MathML-Assoc. tests will be added later.

This patch moves the tests into their own directory (the name of which follows the W3C directory in in m-c/dom/

Reflagging for review to confirm this approach is acceptable.

Comment 12

[James Kitchener (:jkitch)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1404aa0589c5

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3668019351bb

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3668019351bb

Comment 15

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

I just created another test for fraction axis and it seems to fail for stacks:
http://tests.mathml-association.org/mathml/presentation-markup/fractions/frac-1.html

Comment 16

[James Kitchener (:jkitch)]

I'll look into this over the weekend, and create a followup bug for it.