Bug 133567 - Use OpenType MATH constant AxisHeight for math tables
Summary: Use OpenType MATH constant AxisHeight for math tables
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL: http://mpeg.chiariglione.org/standard...
Keywords:
Depends on: 153987 158884
Blocks: 122297 155638
  Show dependency treegraph
 
Reported: 2014-06-05 22:47 PDT by Frédéric Wang (:fredw)
Modified: 2016-07-08 12:42 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.50 KB, patch)
2016-03-18 08:09 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (9.48 KB, patch)
2016-04-26 09:03 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (117.93 KB, patch)
2016-06-16 23:13 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (117.96 KB, patch)
2016-06-25 00:39 PDT, Frédéric Wang (:fredw)
bfulgham: review+
Details | Formatted Diff | Diff
Patch (118.26 KB, patch)
2016-07-08 00:52 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 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.
Comment 1 Frédéric Wang (:fredw) 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)
Comment 2 Frédéric Wang (:fredw) 2016-02-05 06:27:16 PST
This is done in Alex's MathMLLayout branch.
Comment 3 Frédéric Wang (:fredw) 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.
Comment 4 Frédéric Wang (:fredw) 2016-04-26 09:03:10 PDT
Created attachment 277381 [details]
Patch
Comment 5 Frédéric Wang (:fredw) 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.
Comment 6 Frédéric Wang (:fredw) 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.
Comment 7 Frédéric Wang (:fredw) 2016-06-25 00:39:27 PDT
Created attachment 282055 [details]
Patch
Comment 8 Brent Fulgham 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());
Comment 9 Frédéric Wang (:fredw) 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.
Comment 10 Frédéric Wang (:fredw) 2016-07-08 00:52:57 PDT
Created attachment 283117 [details]
Patch
Comment 11 Build Bot 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
Comment 12 Build Bot 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
Comment 13 Build Bot 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
Comment 14 Build Bot 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
Comment 15 Frédéric Wang (:fredw) 2016-07-08 01:49:05 PDT
Created attachment 283126 [details]
Patch
Comment 16 Frédéric Wang (:fredw) 2016-07-08 02:38:42 PDT
Committed r202973: <http://trac.webkit.org/changeset/202973>