Bug 126842 - Only skip stretchy operators when determining the stretch height
Summary: Only skip stretchy operators when determining the stretch height
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: 99620 119043
Blocks: 124838
  Show dependency tree / graph
 
Reported: 2014-01-12 06:37 PST by Frédéric Wang (:fredw)
Modified: 2014-02-22 02:16 PST (History)
8 users (show)

See Also:

Attachments
testcase (233 bytes, text/html)
2014-02-20 02:05 PST, Frédéric Wang (:fredw) 		no flags Details
Patch (4.98 KB, patch)
2014-02-20 02:40 PST, Frédéric Wang (:fredw) 		no flags Details | Formatted Diff | Diff
Patch (5.00 KB, patch)
2014-02-21 02:17 PST, Frédéric Wang (:fredw) 		cfleizach: review+
Details | Formatted Diff | Diff
Patch + 119043 for testing (38.99 KB, patch)
2014-02-22 00:31 PST, Frédéric Wang (:fredw) 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2014-01-12 06:37:34 PST 
> >> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:59
> >> +        // FIXME: Only skip renderMo if it is stretchy.
> > 
> > Bug? :-)
> 
> Fred, I'm not familiar with this issue. If you open a bug for it and write a description I can add a link here.

Sorry, I just realized that I didn't reply to this. I think the idea is that this loop consider the height of children that are not (embellished) operators in order to determine the stretch size. Then a second loop stretch the (embellished) operators to that size. The FIXME comment says that the first loop should only skip *stretchy* operators, so that e.g. <mo>+</mo> is taken into account in the determination of the stretch size. Probably, the second loop should skip these non-stretchy operators too.

I'll open a bug for that later, but it seems to me that this should be fixed after the operator dictionary implementation anyway, so that we know exactly which operator is stretchy.

See also bug 119043 for related improvements to this stretch code (size and alignment).
Comment 1 Frédéric Wang (:fredw) 2014-02-20 02:05:48 PST 
Created attachment 224735 [details]
testcase

In the attached testcase, the brace should stretch to cover the size of the large <mo>p</mo>.
Comment 2 Frédéric Wang (:fredw) 2014-02-20 02:40:26 PST 
Created attachment 224738 [details]
Patch

Here is a simple patch that applies on top of bug 119043.
Comment 3 Frédéric Wang (:fredw) 2014-02-21 02:17:04 PST 
Created attachment 224846 [details]
Patch
Comment 4 Frédéric Wang (:fredw) 2014-02-21 06:09:56 PST 
Comment on attachment 224846 [details]
Patch

Asking review. The test will fail because this depends on bug 119043.
Comment 5 Frédéric Wang (:fredw) 2014-02-22 00:31:37 PST 
Created attachment 224954 [details]
Patch + 119043 for testing
Comment 6 chris fleizach 2014-02-22 01:25:09 PST 
Comment on attachment 224846 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        The MathML code only takes into account non-mo children for the computation of the stretch size. Thus change includes non-stretchy mo in that computation. A new test is added into mathml/presentation/stretchy-depth-height.html.

thus -> this

> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:65
> +            // We exclude stretchy operators from the computation of the stretch size.

I would add the why you're skipping stretchy operators to the comment here as well.
Comment 7 Frédéric Wang (:fredw) 2014-02-22 02:16:03 PST 
Committed r164538: <http://trac.webkit.org/changeset/164538>