Closed Bug 1240799 Opened 5 years ago Closed 5 years ago

mfrac elements without bar do not use axis height

Categories

(Core :: MathML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: fredw, Assigned: jkitch)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
(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.
Flags: needinfo?(khaledhosny)
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.
Flags: needinfo?(khaledhosny)
Assignee: nobody → jkitch.bug
Attached patch patch (obsolete) — Splinter Review
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).
Attachment #8719117 - Flags: review?(karlt)
(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.
Flags: needinfo?(jkitch.bug)
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.
Flags: needinfo?(jkitch.bug)
Attachment #8719117 - Flags: review?(karlt)
(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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8719117 - Attachment is obsolete: true
Attachment #8720232 - Flags: review?(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.)
Attachment #8720232 - Flags: review?(karlt) → review?(fred.wang)
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?
Attachment #8720232 - Flags: review?(fred.wang) → review+
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.
Attachment #8721625 - Flags: review?(fred.wang)
Attachment #8721625 - Flags: review?(fred.wang) → review+
Attachment #8720232 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/3668019351bb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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
Flags: needinfo?(jkitch.bug)
I'll look into this over the weekend, and create a followup bug for it.
Flags: needinfo?(jkitch.bug)
Blocks: 1285807
You need to log in before you can comment on or make changes to this bug.