Bug 133567

Summary: Use OpenType MATH constant AxisHeight for math tables
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, buildbot, commit-queue, dbarton, esprehn+autocc, glenn, kondapallykalyan, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://mpeg.chiariglione.org/standards/mpeg-4/open-font-format/text-isoiec-cd-14496-22-3rd-edition
Bug Depends on: 153987, 158884    
Bug Blocks: 122297, 155638    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
bfulgham: review+
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Patch none

Frédéric Wang (:fredw)
Reported 2014-06-05 22:47:28 PDT
Currently, the MathML code seems to use xHeight/2 as an approximation of the math axis (from the MathML spec: the axis of an equation is an alignment line used by typesetters. It is the line on which a minus sign typically lies): fred@debian:~/webkit/WebKit/Source/WebCore/rendering/mathml$ grep "xHeight()" * RenderMathMLBlock.cpp: lengthValue = floatValue * style->fontMetrics().xHeight(); RenderMathMLBlock.cpp: return (logicalHeight() + style().fontMetrics().xHeight()) / 2; RenderMathMLFraction.cpp: return denominatorWrapper->logicalTop() + static_cast<int>(lroundf((m_lineThickness + style().fontMetrics().xHeight()) / 2)); RenderMathMLOperator.cpp: LayoutUnit axis = style().fontMetrics().xHeight() / 2; RenderMathMLScripts.cpp: LayoutUnit axis = style().fontMetrics().xHeight() / 2; This should probably be handled in a separate "GetMathAxis" function to clarify that we use the math axis. Moreover when a MATH table is available, we should use OpenTypeMathData::AxisHeight to be more accurate.
Attachments
Patch (8.50 KB, patch)
2016-03-18 08:09 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (9.48 KB, patch)
2016-04-26 09:03 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (117.93 KB, patch)
2016-06-16 23:13 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (117.96 KB, patch)
2016-06-25 00:39 PDT, Frédéric Wang (:fredw) 		bfulgham: review+
DetailsFormatted DiffDiff
Patch (118.26 KB, patch)
2016-07-08 00:52 PDT, Frédéric Wang (:fredw) 		buildbot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews102 for mac-yosemite (920.72 KB, application/zip)
2016-07-08 01:39 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (788.68 KB, application/zip)
2016-07-08 01:44 PDT, Build Bot 		no flags
Details
Patch (118.12 KB, patch)
2016-07-08 01:49 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Frédéric Wang (:fredw)
Comment 1 2014-07-20 07:44:15 PDT
This, as well as using OpenTypeMathData::FractionRuleThickness as the default thickness in RenderMathMLFraction.cpp should now be relatively easy. See how it is done for OpenTypeMathData::DisplayOperatorMinHeight in RenderMathMLOperator.cpp. (see the desc of the MATH table in http://mpeg.chiariglione.org/standards/mpeg-4/open-font-format/text-isoiec-cd-14496-22-3rd-edition)
Frédéric Wang (:fredw)
Comment 2 2016-02-05 06:27:16 PST
This is done in Alex's MathMLLayout branch.
Frédéric Wang (:fredw)
Comment 3 2016-03-18 08:09:04 PDT
Created attachment 274414 [details] Patch After the refactoring of RenderMathMLFraction and RenderMathMLOperator, the only remaining case to handle is RenderMathMLTable, which is addressed in this patch.
Frédéric Wang (:fredw)
Comment 4 2016-04-26 09:03:10 PDT
Created attachment 277381 [details] Patch
Frédéric Wang (:fredw)
Comment 5 2016-05-17 01:01:23 PDT
The case of RenderMathMLOperator has now removed from phase 1 of the refactoring and should be handled here too. Actually, I realized that symmetric stretching is not quite correct (at least now that we support size variant and not just glyph assembly) see bug 152244 comment 32 for suggestions.
Frédéric Wang (:fredw)
Comment 6 2016-06-16 23:13:45 PDT
Created attachment 281536 [details] Patch Just uploading the patch again, as it seems it won't apply onto trunk for now.
Frédéric Wang (:fredw)
Comment 7 2016-06-25 00:39:27 PDT
Created attachment 282055 [details] Patch
Brent Fulgham
Comment 8 2016-07-07 13:21:51 PDT
Comment on attachment 282055 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282055&action=review This change looks good, but doesn't apply. I'll mark it r+ on the condition that you make sure the tests pass before you land it! :-) > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:317 > + return Optional<int>(logicalHeight() / 2 + axisHeight(style())); It might be simpler to just say: return Optional<int>(logicalHeight() / 2 + mathAxisHeight());
Frédéric Wang (:fredw)
Comment 9 2016-07-08 00:40:11 PDT
(In reply to comment #8) > Comment on attachment 282055 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282055&action=review > > This change looks good, but doesn't apply. I'll mark it r+ on the condition > that you make sure the tests pass before you land it! :-) > > > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:317 > > + return Optional<int>(logicalHeight() / 2 + axisHeight(style())); > > It might be simpler to just say: > > return Optional<int>(logicalHeight() / 2 + mathAxisHeight()); Yes, but again as in bug 133845 that's not possible because RenderMathMLTable does not inherit from RenderMathMLBlock. That's why I introduced this static axisHeight function to share a bit the logic... :-( I will add a comment here too.
Frédéric Wang (:fredw)
Comment 10 2016-07-08 00:52:57 PDT
Created attachment 283117 [details] Patch
Build Bot
Comment 11 2016-07-08 01:39:32 PDT
Comment on attachment 283117 [details] Patch Attachment 283117 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1645611 New failing tests: imported/mathml-in-html5/mathml/presentation-markup/operators/mo-axis-height-1.html
Build Bot
Comment 12 2016-07-08 01:39:36 PDT
Created attachment 283124 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 13 2016-07-08 01:44:00 PDT
Comment on attachment 283117 [details] Patch Attachment 283117 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1645621 New failing tests: imported/mathml-in-html5/mathml/presentation-markup/operators/mo-axis-height-1.html
Build Bot
Comment 14 2016-07-08 01:44:03 PDT
Created attachment 283125 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Frédéric Wang (:fredw)
Comment 15 2016-07-08 01:49:05 PDT
Created attachment 283126 [details] Patch
Frédéric Wang (:fredw)
Comment 16 2016-07-08 02:38:42 PDT
Committed r202973: <http://trac.webkit.org/changeset/202973>
Note You need to log in before you can comment on or make changes to this bug.