Bug 119043

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

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

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

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

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

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.

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

Created attachment 224620 [details] Patch V1

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

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

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

Created attachment 224632 [details] Patch V2

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

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

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

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

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

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

Created attachment 224729 [details] Patch

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.

Created attachment 224844 [details] Patch

(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 on attachment 224844 [details] Patch I'm not seeing stretchy-depth-height.html in this patch otherwise seems ok

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

Created attachment 224953 [details] Patch

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

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

Reverted r164534 for reason: missing tests Committed r164535: <http://trac.webkit.org/changeset/164535>

Created attachment 224956 [details] Final Patch

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

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