Bug 119043 – Large stretch size error for MathML operators

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 217827 [details]
testcase with different depth and height

[Frédéric Wang (:fredw)]

Created attachment 217828 [details]
screenshot of attachment 217827 [details] in Gecko

[Frédéric Wang (:fredw)]

Created attachment 217829 [details]
screenshot of attachment 217827 [details] in WebKit

[Frédéric Wang (:fredw)]

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).

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 218063 [details]
screenshot of attachment 217827 [details] in WebKit + the patch

[Frédéric Wang (:fredw)]

Created attachment 224620 [details]
Patch V1

[Build Bot]

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

[Build Bot]

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

[Frédéric Wang (:fredw)]

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.

[Build Bot]

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

[Build Bot]

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

[Frédéric Wang (:fredw)]

Created attachment 224632 [details]
Patch V2

[Martin Robinson]

How do you plan to make this a reference test and still test proper rendering of stretchy characters?

[Frédéric Wang (:fredw)]

(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>...

[chris fleizach]

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

[Frédéric Wang (:fredw)]

(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.

[Frédéric Wang (:fredw)]

So should I rename them ascent/descent or just add a comment explaining what they represent?

[Martin Robinson]

(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.

[Frédéric Wang (:fredw)]

(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.

[Frédéric Wang (:fredw)]

Created attachment 224729 [details]
Patch

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 224844 [details]
Patch

[Frédéric Wang (:fredw)]

(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

[chris fleizach]

Comment on attachment 224844 [details]
Patch

I'm not seeing
stretchy-depth-height.html in this patch
otherwise seems ok

[Frédéric Wang (:fredw)]

(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.

[Frédéric Wang (:fredw)]

Created attachment 224953 [details]
Patch

[chris fleizach]

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

[Frédéric Wang (:fredw)]

Committed r164534: <http://trac.webkit.org/changeset/164534>

[Frédéric Wang (:fredw)]

Reverted r164534 for reason:

missing tests

Committed r164535: <http://trac.webkit.org/changeset/164535>

[Frédéric Wang (:fredw)]

Created attachment 224956 [details]
Final Patch

[Frédéric Wang (:fredw)]

Committed r164536: <http://trac.webkit.org/changeset/164536>

[Frédéric Wang (:fredw)]

OK, the first attempt was correct. For some reason I didn't realize that the new files were in the commit diff...