Closed
Bug 1240799
Opened 5 years ago
Closed 5 years ago
mfrac elements without bar do not use axis height
Categories
(Core :: MathML, defect)
Core
MathML
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: fredw, Assigned: jkitch)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
49.19 KB,
patch
|
fredw
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•5 years ago
|
||
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.
Reporter | ||
Comment 2•5 years ago
|
||
(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)
Comment 3•5 years ago
|
||
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 | ||
Updated•5 years ago
|
Assignee: nobody → jkitch.bug
Assignee | ||
Comment 4•5 years ago
|
||
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)
Reporter | ||
Comment 5•5 years ago
|
||
(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)
Assignee | ||
Comment 6•5 years ago
|
||
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)
Reporter | ||
Comment 7•5 years ago
|
||
(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.
Assignee | ||
Comment 8•5 years ago
|
||
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 9•5 years ago
|
||
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)
Reporter | ||
Comment 10•5 years ago
|
||
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+
Assignee | ||
Comment 11•5 years ago
|
||
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)
Reporter | ||
Updated•5 years ago
|
Attachment #8721625 -
Flags: review?(fred.wang) → review+
Assignee | ||
Updated•5 years ago
|
Attachment #8720232 -
Attachment is obsolete: true
Assignee | ||
Comment 12•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1404aa0589c5
Keywords: checkin-needed
Comment 13•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3668019351bb
Keywords: checkin-needed
Comment 14•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3668019351bb
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Reporter | ||
Comment 15•5 years ago
|
||
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)
Assignee | ||
Comment 16•5 years ago
|
||
I'll look into this over the weekend, and create a followup bug for it.
Flags: needinfo?(jkitch.bug)
You need to log in
before you can comment on or make changes to this bug.
Description
•