Bug 119043 - Large stretch size error for MathML operators
Summary: Large stretch size error for MathML operators
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:
Keywords:
Depends on:
Blocks: 124827 126842
  Show dependency treegraph
 
Reported: 2013-07-24 01:45 PDT by Frédéric Wang (:fredw)
Modified: 2014-02-22 02:04 PST (History)
9 users (show)

See Also:


Attachments
testcase (1.51 KB, text/html)
2013-07-24 01:45 PDT, Frédéric Wang (:fredw)
no flags Details
testcase with different depth and height (1.19 KB, text/html)
2013-11-25 13:35 PST, Frédéric Wang (:fredw)
no flags Details
screenshot of attachment 217827 in Gecko (7.91 KB, image/png)
2013-11-25 13:37 PST, Frédéric Wang (:fredw)
no flags Details
screenshot of attachment 217827 in WebKit (3.60 KB, image/png)
2013-11-25 13:38 PST, Frédéric Wang (:fredw)
no flags Details
WIP Patch (8.51 KB, patch)
2013-11-29 07:01 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
screenshot of attachment 217827 in WebKit + the patch (12.39 KB, image/png)
2013-11-29 07:03 PST, Frédéric Wang (:fredw)
no flags Details
Patch V1 (18.50 KB, patch)
2014-02-19 04:10 PST, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (498.16 KB, application/zip)
2014-02-19 05:12 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (529.83 KB, application/zip)
2014-02-19 07:40 PST, Build Bot
no flags Details
Patch V2 (35.85 KB, patch)
2014-02-19 07:47 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (36.39 KB, patch)
2014-02-20 00:59 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (29.44 KB, patch)
2014-02-21 01:54 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (36.85 KB, patch)
2014-02-22 00:29 PST, Frédéric Wang (:fredw)
cfleizach: review+
Details | Formatted Diff | Diff
Final Patch (36.83 KB, patch)
2014-02-22 01:58 PST, 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) 2013-07-24 01:45:00 PDT
Created attachment 207384 [details]
testcase

When building operators with glyphs you generally can not achieve perfect stretch size and only want the operators to stretch slightly larger than the target sizes. However in the attached testcase, the error is significant, at least 50px. Martin's patch for stretchy operators does not improve the situation.
Comment 1 Frédéric Wang (:fredw) 2013-07-30 08:14:46 PDT
This bug can be seen in test 23 of the Mozilla MathML torture test (and a bit in test 24 and 18 too):
https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/MathML_Torture_Test
Comment 2 Frédéric Wang (:fredw) 2013-11-25 13:35:08 PST
Created attachment 217827 [details]
testcase with different depth and height
Comment 3 Frédéric Wang (:fredw) 2013-11-25 13:37:50 PST
Created attachment 217828 [details]
screenshot of attachment 217827 [details] in Gecko
Comment 4 Frédéric Wang (:fredw) 2013-11-25 13:38:30 PST
Created attachment 217829 [details]
screenshot of attachment 217827 [details] in WebKit
Comment 5 Frédéric Wang (:fredw) 2013-11-25 13:47:52 PST
I just looked at the stretching code and I think:

- The gOperatorExpansion seems to cause the error. It basically adds 20% of the target size, so for a target of height 300px in the first testcase, that's 60px! It seems that the delimiter size should match the target size, or add a small delta that depends only on the font-size, not the target size.

- I'm not sure I understand the alignment in RenderMathMLOperator::firstLineBaseline(). In my opinion, the target size should equal to a height + a depth (using the terminology of the mspace element) and this should be used to determine the alignment.

- Also, we may want to distinguish the symmetric / non symmetric cases (bug 124827).
Comment 6 Frédéric Wang (:fredw) 2013-11-29 07:01:37 PST
Created attachment 218062 [details]
WIP Patch

Here is a patch that removes the gOperatorExpansion factor (and thus the stretch error) and takes into account the children height/depth to align and stretch the operator correctly. I have not tested it much, but that seems to improves the rendering on the torture test and of the testcase.
Comment 7 Frédéric Wang (:fredw) 2013-11-29 07:03:25 PST
Created attachment 218063 [details]
screenshot of attachment 217827 [details] in WebKit + the patch
Comment 8 Frédéric Wang (:fredw) 2014-02-19 04:10:10 PST
Created attachment 224620 [details]
Patch V1
Comment 9 Build Bot 2014-02-19 05:11:58 PST
Comment on attachment 224620 [details]
Patch V1

Attachment 224620 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5106382919434240

New failing tests:
fast/ruby/ruby-base-merge-block-children-crash-2.html
mathml/presentation/mo-stretch.html
Comment 10 Build Bot 2014-02-19 05:12:00 PST
Created attachment 224624 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 11 Frédéric Wang (:fredw) 2014-02-19 06:48:14 PST
Comment on attachment 224620 [details]
Patch V1

I'm asking review for this. The mac failures for mo-stretch.html are expected, as I indicated in the ChangeLog. I'll need to update the reference once the bots give the output but they seem to take time. Actually, I'm not sure why the other ports pass the test. I don't really like this pixel test, so I think I'll convert it to reftest in another bug.
Comment 12 Build Bot 2014-02-19 07:40:08 PST
Comment on attachment 224620 [details]
Patch V1

Attachment 224620 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5155889933713408

New failing tests:
fast/ruby/ruby-base-merge-block-children-crash-2.html
mathml/presentation/mo-stretch.html
Comment 13 Build Bot 2014-02-19 07:40:11 PST
Created attachment 224631 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 14 Frédéric Wang (:fredw) 2014-02-19 07:47:17 PST
Created attachment 224632 [details]
Patch V2
Comment 15 Martin Robinson 2014-02-19 08:59:13 PST
How do you plan to make this a reference test and still test proper rendering of stretchy characters?
Comment 16 Frédéric Wang (:fredw) 2014-02-19 09:06:46 PST
(In reply to comment #15)
> How do you plan to make this a reference test and still test proper rendering of stretchy characters?

I think I'll try to measure getBoundingClientRect() as in stretchy-depth-height.html. There is another test to check gaps IIRC. This won't test that the rendering is "good", though. The problem with that pixel test is that it is going to break each time we modify the rendering of <mo>...
Comment 17 chris fleizach 2014-02-19 11:00:20 PST
Comment on attachment 224632 [details]
Patch V2

View in context: https://bugs.webkit.org/attachment.cgi?id=224632&action=review

some minor comments

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:92
> +    int m_stretchDepth;

what does depth represent here? It looks like the logicalHeight - firstLineBase, but it's not clear from the name what this is

> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:90
> +        if (auto renderMo = child.unembellishedOperator())

auto& renderOperator
Comment 18 Frédéric Wang (:fredw) 2014-02-19 11:08:44 PST
(In reply to comment #17)
> some minor comments
> 
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:92
> > +    int m_stretchDepth;
> 
> what does depth represent here? It looks like the logicalHeight - firstLineBase, but it's not clear from the name what this is

height is the distance above the baseline and depth below the baseline. These are the TeX names I think, as well as those of the <mspace> elements. However, "height" is confusing since it is sometimes used to denote the sum of these two values. Alternatively, perhaps we can use ascent/descent as in font metrics.
Comment 19 Frédéric Wang (:fredw) 2014-02-19 11:17:36 PST
So should I rename them ascent/descent or just add a comment explaining what they represent?
Comment 20 Martin Robinson 2014-02-19 11:34:54 PST
(In reply to comment #19)
> I think I'll try to measure getBoundingClientRect() as in stretchy-depth-height.html. There is another test to check gaps IIRC. This won't test that the rendering is "good", though. The problem with that pixel test is that it is going to break each time we modify the rendering of <mo>...

I think that's fine. There are really only 4 ports to rebase and otherwise we don't have test coverage.
Comment 21 Frédéric Wang (:fredw) 2014-02-19 11:53:33 PST
(In reply to comment #20)
> I think that's fine. There are really only 4 ports to rebase and otherwise we don't have test coverage.

OK, so let's keep that test. However, that does not explain why only the Mac bot failed and why I only needed to update the txt reference. I would have expected that the *.png and other ports failed too.
Comment 22 Frédéric Wang (:fredw) 2014-02-20 00:59:54 PST
Created attachment 224729 [details]
Patch
Comment 23 Frédéric Wang (:fredw) 2014-02-20 01:38:03 PST
Comment on attachment 224729 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224729&action=review

> LayoutTests/mathml/presentation/stretchy-depth-height.html:101
> +          <mo id="s5">{</mo>

I forgot a symmetric="false" here. That won't make the test fails now but will after bug 124827. So I'll update this after the review.
Comment 24 Frédéric Wang (:fredw) 2014-02-21 01:54:54 PST
Created attachment 224844 [details]
Patch
Comment 25 Frédéric Wang (:fredw) 2014-02-21 02:04:51 PST
(In reply to comment #23)
> I forgot a symmetric="false" here. That won't make the test fails now but will after bug 124827. So I'll update this after the review.

Done.

(In reply to comment #19)
> So should I rename them ascent/descent or just add a comment explaining what they represent?

After discussion with Martin, I've renamed them heightAboveBaseline and depthBelowBaseline.

Actually, the spec uses the axis as a reference (center of plus sign, of fraction bar etc), but that won't make a difference for asymmetric stretching here and using the baseline as a reference is bit more convenient to handle with the firstLineBaseline() function. I'll need to update bug 124827 to consider stretch symmetrically with respect to the axis (although we won't really have a reliable way to compute it until bug 122297 is fixed).

See http://www.w3.org/TR/MathML/chapter3.html#id.3.2.5.8.2
Comment 26 chris fleizach 2014-02-21 23:15:49 PST
Comment on attachment 224844 [details]
Patch

I'm not seeing
stretchy-depth-height.html in this patch
otherwise seems ok
Comment 27 Frédéric Wang (:fredw) 2014-02-22 00:23:06 PST
(In reply to comment #26)
> (From update of attachment 224844 [details])
> I'm not seeing
> stretchy-depth-height.html in this patch
> otherwise seems ok

It is in attachment 224729 [details] but was lost in this last patch. I'll upload a new version.
Comment 28 Frédéric Wang (:fredw) 2014-02-22 00:29:06 PST
Created attachment 224953 [details]
Patch
Comment 29 chris fleizach 2014-02-22 01:18:26 PST
Comment on attachment 224953 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224953&action=review

> LayoutTests/mathml/presentation/stretchy-depth-height.html:11
> +        return Math.abs(x - y) < epsilon;

Indentation should be 4 spaces in.
{ should be on line above
Comment 30 Frédéric Wang (:fredw) 2014-02-22 01:45:33 PST
Committed r164534: <http://trac.webkit.org/changeset/164534>
Comment 31 Frédéric Wang (:fredw) 2014-02-22 01:52:21 PST
Reverted r164534 for reason:

missing tests

Committed r164535: <http://trac.webkit.org/changeset/164535>
Comment 32 Frédéric Wang (:fredw) 2014-02-22 01:58:31 PST
Created attachment 224956 [details]
Final Patch
Comment 33 Frédéric Wang (:fredw) 2014-02-22 02:02:49 PST
Committed r164536: <http://trac.webkit.org/changeset/164536>
Comment 34 Frédéric Wang (:fredw) 2014-02-22 02:04:40 PST
OK, the first attempt was correct. For some reason I didn't realize that the new files were in the commit diff...