Bug 158866

Summary: MathOperator: Improve alignment for vertical size variant
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, bfulgham, buildbot, commit-queue, dbarton, esprehn+autocc, glenn, kondapallykalyan, rego, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 124827, 152244    
Bug Blocks: 159689, 159715    
Attachments:
Description Flags
Patch
none
Screenshot of MathML torture test 14, XITS fonts
none
Patch
none
Archive of layout-test-results from ews101 for mac-yosemite
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch bfulgham: review+

Description Frédéric Wang (:fredw) 2016-06-16 23:04:22 PDT
Follow-up of bug 152244 comment 32.

Currently, MathOperator::stretchTo is called with the desired ascent/descent. When m_stretchType == StretchType::GlyphAssembly, this is used to set the desired vertical position of the operator. However, when m_stretchType == StretchType::SizeVariant the actual ascent/descent of the glyph is used for painting, and this may not correspond at all to the original MathOperator::stretchTo request.

One easy way to fix that is to make MathOperator::stretchTo calculate a parameter m_verticalOffset when m_stretchType != StretchType::GlyphAssembly which would measure the difference between targetCenter = (-ascent + descent) / 2 and glyphCenter = (-m_ascent + m_descent) / 2. Then that parameter can be used to apply a shift during MathOperator::paint.

Or maybe MathOperator::stretchTo should only take the target height as the parameter and it will be up to the MathOperator::paint caller to adjust the vertical offset (although that would probably mean that the shift will be stored in the caller's class).

There is a test from the MathML in HTML5 test suite but it does not work well in WebKit yet:
http://tests.mathml-association.org/mathml/presentation-markup/operators/mo-axis-height-1.html
Comment 1 Frédéric Wang (:fredw) 2016-06-24 03:08:13 PDT
Created attachment 281964 [details]
Patch

Here is an experimental patch showing the idea. Not sure it handles all the cases.
Comment 2 Frédéric Wang (:fredw) 2016-06-24 03:09:21 PDT
Created attachment 281965 [details]
Screenshot of MathML torture test 14, XITS fonts

This screenshot shows the improvement of the patch for MathML torture test 14, XITS fonts.
Comment 3 Frédéric Wang (:fredw) 2016-07-13 23:25:19 PDT
Created attachment 283605 [details]
Patch
Comment 4 Frédéric Wang (:fredw) 2016-07-13 23:29:08 PDT
So I'm not sure what happened with mo-axis-height-1.html. It seems that the previous import and test expectation was just wrong :-( Anyway, after bug 158884 on http://tests.mathml-association.org/mathml/presentation-markup/operators/mo-axis-height-1.html the size variant test fails and the glyph assembly test passes. With the uploaded patch, the size variant test should pass too ;-)

I still need to retrieve some pixel results for iOS/Mac, rebaseline and upload a new patch...
Comment 5 Build Bot 2016-07-14 00:13:09 PDT
Comment on attachment 283605 [details]
Patch

Attachment 283605 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1678588

New failing tests:
mathml/opentype/opentype-stretchy.html
mathml/presentation/mo-stretch.html
Comment 6 Build Bot 2016-07-14 00:13:13 PDT
Created attachment 283610 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 7 Build Bot 2016-07-14 00:15:15 PDT
Comment on attachment 283605 [details]
Patch

Attachment 283605 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1678589

New failing tests:
mathml/opentype/opentype-stretchy.html
mathml/presentation/mo-stretch.html
Comment 8 Build Bot 2016-07-14 00:15:19 PDT
Created attachment 283611 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 9 Build Bot 2016-07-14 00:21:52 PDT
Comment on attachment 283605 [details]
Patch

Attachment 283605 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1678593

New failing tests:
mathml/opentype/opentype-stretchy.html
mathml/presentation/mo-stretch.html
Comment 10 Build Bot 2016-07-14 00:21:56 PDT
Created attachment 283614 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 11 Build Bot 2016-07-14 00:22:25 PDT
Comment on attachment 283605 [details]
Patch

Attachment 283605 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1678594

New failing tests:
mathml/opentype/opentype-stretchy.html
mathml/presentation/mo-stretch.html
Comment 12 Build Bot 2016-07-14 00:22:29 PDT
Created attachment 283615 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5
Comment 13 Frédéric Wang (:fredw) 2016-07-14 00:30:42 PDT
Created attachment 283617 [details]
Patch
Comment 14 Frédéric Wang (:fredw) 2016-07-15 11:28:42 PDT
Can someone please review this patch?
Comment 15 Brent Fulgham 2016-07-15 11:40:58 PDT
Comment on attachment 283617 [details]
Patch

This change looks fine. r=me.
Comment 16 Frédéric Wang (:fredw) 2016-07-15 11:48:12 PDT
Committed r203289: <http://trac.webkit.org/changeset/203289>
Comment 17 Frédéric Wang (:fredw) 2016-07-17 23:09:05 PDT
Committed r203339: <http://trac.webkit.org/changeset/203339>