Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MathML: Test italic correction of sub and super scripts attached to a large operator #9993

Merged
merged 4 commits into from Mar 22, 2018

Conversation

Projects
None yet
4 participants
@fred-wang
Copy link
Contributor

fred-wang commented Mar 13, 2018

This test italic correction for subsup script (tests for underover should be added in the future). The test passes on WebKit and will replace https://trac.webkit.org/browser/webkit/trunk/LayoutTests/mathml/opentype/large-operators-italic-correction.html (which requires Latin Modern Math to be installed on the WebKit bots).

@wpt-pr-bot wpt-pr-bot requested review from fantasai, gsnedders and kojiishi Mar 13, 2018

@fred-wang fred-wang requested review from mrego and removed request for gsnedders, fantasai and kojiishi Mar 13, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 13, 2018

Build PASSED

Started: 2018-03-22 06:12:26
Finished: 2018-03-22 06:19:04

View more information about this build on:

@wpt-pr-bot wpt-pr-bot requested review from fantasai, gsnedders and kojiishi Mar 13, 2018

@fred-wang fred-wang force-pushed the largeop-subsup-italic-correction branch from 10dcca5 to 7583caa Mar 14, 2018

@fred-wang

This comment has been minimized.

Copy link
Contributor Author

fred-wang commented Mar 16, 2018

screenshot-italic-correction

This is how the test is rendered in Gecko (left) and WebKit (right). Note however that the position of script is wrong in Gecko for the nonzero-italic case (second row) and the rendering of the operator is bad in WebKit in the zero-italic case (first row). WebKit still passes the test but not Gecko.

  • largeop-displayoperatorminheight5000 has a large glyph for U+2AFF N-ARY WHITE VERTICAL BAR that has null italic correction (it is the rectangle displayed in the first row of the Gecko screenshot)
  • The change in largeop.py adds a new file largeop-displayoperatorminheight2000-2AFF-italiccorrection3000 has a large glyph for U+2AFF N-ARY WHITE VERTICAL BAR with italic correction of 3000 (it is the inclined parallelogram displayed in the second row of the WebKit screenshot).
  • minheight values are just to ensure web engines will pick the large glyph of U+2AFF and not the base size.

I pushed a commit to better explain the tests.

cc'ing @davidcarlisle @karlt @khaledhosny for review.

}
</style>
<script>
var emToPx = 10 / 1000; // font-size: 10px, font.em = 1000

This comment has been minimized.

Copy link
@mrego

mrego Mar 21, 2018

Member

Can we move this closer to where it's used?

This comment has been minimized.

Copy link
@fred-wang

fred-wang Mar 22, 2018

Author Contributor

Done.

</style>
<script>
var emToPx = 10 / 1000; // font-size: 10px, font.em = 1000
var epsilon = 1;

This comment has been minimized.

Copy link
@mrego

mrego Mar 21, 2018

Member

Ditto.

This comment has been minimized.

Copy link
@fred-wang

fred-wang Mar 22, 2018

Author Contributor

Done

var v = 0;
assert_approx_equals(getBox("base001").right - getBox("sub001").left, v, epsilon, "msub");
assert_approx_equals(getBox("sup002").left, getBox("base002").right, epsilon, "msup");
assert_approx_equals(getBox("sup003").left - getBox("sub003").left, v, epsilon, "msubsup");

This comment has been minimized.

Copy link
@mrego

mrego Mar 21, 2018

Member

If both are wrongly positioned, but with the same left this will pass.
Wouldn't be better to always compare against "base00X"?
Dunno if that's possible when you the italic correction is applied.

This comment has been minimized.

Copy link
@fred-wang

fred-wang Mar 22, 2018

Author Contributor

Yes, however I'm not sure that's a big deal to "wrongly pass" (Chromium and Edge have many MathML tests passing on wpt.fyi ; just because they succeeded to load the test !). I think it's clearer and more natural to test the italic correction by checking the difference of position of the two scripts. I could add extra tests to check the horizontal position but that kind of tests is already (and more completely done) done by http://w3c-test.org/mathml/presentation-markup/scripts/subsup-parameters-1.html

This comment has been minimized.

Copy link
@mrego

mrego Mar 22, 2018

Member

The goal of a test is to fail when something is not supported, so it'd be better if they fail.
Anyway I understand we have other tests to check the vertical position of the scripts and also the size of the operator (otherwise we need them).
So the rest can be good enough, we're at least checking against "base00X" in some cases so we'll should catch some issues with it.

}
</script>
</head>
<body>

This comment has been minimized.

Copy link
@mrego

mrego Mar 21, 2018

Member

Nit: Dunno if this is common or not on MathML test suite, so feel free to ignore these suggestions:

  • I'd add a <div id="log"></div> on the top of the test, so when you open it the testharness output will be on the top.
  • I'd add a title before each section (like <h2>Null Italic Correction</h2> and <h2>NonNull Italic Correction</h2>).

This comment has been minimized.

Copy link
@fred-wang

fred-wang Mar 22, 2018

Author Contributor

Yes, I never liked that the test results don't show at the top. I actually think it's a good idea and should be done for MathML tests too.

@mrego

mrego approved these changes Mar 22, 2018

@fred-wang fred-wang merged commit 74126cb into master Mar 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fred-wang fred-wang deleted the largeop-subsup-italic-correction branch Mar 22, 2018

hubot pushed a commit to WebKit/webkit that referenced this pull request Mar 22, 2018

fred.wang@free.fr
[MathML] Import WPT test to replace mathml/opentype/large-operators-i…
…talic-correction.html

https://bugs.webkit.org/show_bug.cgi?id=183891

Patch by Frederic Wang <fwang@igalia.com> on 2018-03-22
Reviewed by Manuel Rego Casasnovas.

LayoutTests/imported/w3c:

Import WPT test added in web-platform-tests/wpt#9993.

* web-platform-tests/fonts/math/largeop-displayoperatorminheight2000-2AFF-italiccorrection3000.woff: Added.
* web-platform-tests/fonts/math/w3c-import.log:
* web-platform-tests/mathml/presentation-markup/scripts/subsup-parameters-2-expected.txt: Added.
* web-platform-tests/mathml/presentation-markup/scripts/subsup-parameters-2.html: Added.
* web-platform-tests/mathml/presentation-markup/scripts/w3c-import.log:
* web-platform-tests/mathml/tools/largeop.py:

LayoutTests:

Remove large-operators-italic-correction.html. Italic correction is tested more completely by
a new WPT test that does not require Latin Modern Math to be installed on the try bots.

* mathml/opentype/large-operators-italic-correction-expected.txt: Removed.
* mathml/opentype/large-operators-italic-correction.html: Removed.
* platform/ios/TestExpectations: Remove test expectation.
* platform/mac/TestExpectations: Ditto.
* platform/win/TestExpectations: Ditto.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@229847 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.