Bug 37044 – MathML Support for mroot & msqrt

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[François Sausset]

Created attachment 52450 [details]
Based on the Alex Milowski MathML "monster bug". Improved paddings, baseline alignments & root drawing.

First attempt for an implementation of mroot & msqrt elements.

[Alex Milowski]

> Index: WebCore/mathml/RenderMathMLRoot.cpp
> ===================================================================
> --- WebCore/mathml/RenderMathMLRoot.cpp	(revision 0)
> +++ WebCore/mathml/RenderMathMLRoot.cpp	(revision 0)

> +
> +const int radicalBasePad = 3;
> +const float thresholdBaseHeight = 1.5;

This should probable be prefixed with a 'g':

   gRadicalBasePad
   gThresholdBaseHeight

Any comment describing these constants would be good.

> +
> +RenderMathMLRoot::RenderMathMLRoot(Node *expression) 
> +: RenderMathMLBlock(expression) 
> +{
> +}
> +
> +void RenderMathMLRoot::addChild(RenderObject* child, RenderObject* )
> +{
> +    if (isEmpty()) {
> +        // Add a block for the index
> +        RenderBlock* block = new (renderArena()) RenderBlock(node());
> +        RefPtr<RenderStyle> indexStyle = makeBlockStyle();
> +        indexStyle->setDisplay(INLINE_BLOCK);
> +        block->setStyle(indexStyle.release());
> +        RenderBlock::addChild(block);
> +        
> +        // this is the base, so wrap it so we can pad it
> +        block = new (renderArena()) RenderBlock(node());
> +        RefPtr<RenderStyle> baseStyle = makeBlockStyle();
> +        baseStyle->setDisplay(INLINE_BLOCK);
> +        baseStyle->setPaddingLeft(Length(0.5 * 0.75 , Percent));

You should make these numbers constants.



> +void RenderMathMLRoot::paint(PaintInfo& info, int tx, int ty)
> +{
> +    RenderMathMLBlock::paint(info , tx , ty);
> +    
> +    tx += x();
> +    ty += y();
> +    
> +    RenderBoxModelObject* indexBox = toRenderBoxModelObject(lastChild());
> +    
> +    int maxHeight = indexBox->offsetHeight();
> +    if (!maxHeight) {
> +        // default to the font size in pixels if we're empty
> +        maxHeight = style()->fontSize();
> +    }
> +    int width = indexBox->offsetWidth();
> +    
> +    int indexWidth = 0;
> +    RenderObject* current = firstChild();
> +    while (current != lastChild()) {
> +        if (current->isBoxModelObject()) {
> +            RenderBoxModelObject* box = toRenderBoxModelObject(current);
> +            indexWidth += box->offsetWidth();
> +        }
> +        current = current->nextSibling();
> +    }
> +    
> +    int frontWidth = static_cast<int>(style()->fontSize() * 0.75);

Make the number a constant as well.

> +    int topStartShift = 0;
> +    // Base height above which the shape of the root changes
> +    int thresholdHeight = static_cast<int>(thresholdBaseHeight * style()->fontSize());
> +    
> +    if (maxHeight > thresholdHeight && thresholdHeight) {
> +        float shift = (maxHeight - thresholdHeight) / static_cast<float>(thresholdHeight);
> +        if (shift > 1.)
> +            shift = 1.;
> +        topStartShift = static_cast<int>(0.5 * frontWidth * shift);
> +    }

Same here with the 0.5

> +    
> +    width += topStartShift;
> +    
> +    int start = tx + indexWidth + radicalBasePad + style()->paddingLeft().value() - static_cast<int>(0.2 * style()->fontSize());
> +    ty += style()->paddingTop().value() - static_cast<int>(0.2 * style()->fontSize());
> +    
> +    FloatPoint topStart(start - topStartShift, ty);
> +    FloatPoint bottomLeft(start - 0.5 * frontWidth , ty + maxHeight + radicalBasePad);
> +    FloatPoint topLeft(start - 0.8 * frontWidth , ty + 0.625 * maxHeight);
> +    FloatPoint leftEnd(start - frontWidth , topLeft.y() + 0.05 * style()->fontSize());

Similarly, make the numbers named constants.

> +    
> +    info.context->save();
> +    
> +    info.context->setStrokeThickness(0.02 * style()->fontSize());

Same here.

> +    info.context->setStrokeStyle(SolidStroke);
> +    info.context->setStrokeColor(style()->color(), sRGBColorSpace);
> +    info.context->setLineJoin(MiterJoin);
> +    info.context->setMiterLimit(style()->fontSize());
> +    
> +    Path root;
> +    
> +    root.moveTo(FloatPoint(topStart.x() + width , ty));

No space before comma.

> +    // draw top
> +    root.addLineTo(topStart);
> +    // draw from top left corner to bottom point of radical
> +    root.addLineTo(bottomLeft);
> +    // draw from bottom point to top of left part of radical base "pocket"
> +    root.addLineTo(topLeft);
> +    // draw to end
> +    root.addLineTo(leftEnd);
> +    
> +    info.context->beginPath();
> +    info.context->addPath(root);
> +    info.context->strokePath();
> +    
> +    info.context->save();
> +    
> +    // Build a mask to draw the thick part of the root.
> +    Path mask;
> +    
> +    mask.moveTo(topStart);
> +    mask.addLineTo(bottomLeft);
> +    mask.addLineTo(topLeft);
> +    mask.addLineTo(FloatPoint(2 * topLeft.x() - leftEnd.x(), 2 * topLeft.y() - leftEnd.y()));

Make the '2' a constant.

> +    
> +    info.context->beginPath();
> +    info.context->addPath(mask);
> +    info.context->clip(mask);
> +    
> +    // Draw the thick part of the root.
> +    info.context->setStrokeThickness(0.1 * style()->fontSize());

Same here.

> +    info.context->setLineCap(SquareCap);
> +    
> +    Path line;
> +    
> +    line = line.createLine(bottomLeft, topLeft);
> +    
> +    info.context->beginPath();
> +    info.context->addPath(line);
> +    info.context->strokePath();
> +    
> +    info.context->restore();
> +    
> +    info.context->restore();
> +
> +}
> +
> +void RenderMathMLRoot::layout()
> +{
> +    RenderBlock::layout();
> +    
> +    int maxHeight = toRenderBoxModelObject(lastChild())->offsetHeight();
> +    
> +    RenderObject* current = lastChild()->firstChild();
> +    
> +    toRenderMathMLBlock(current)->style()->setVerticalAlign(BASELINE);
> +    
> +    if (!maxHeight)
> +        maxHeight = style()->fontSize();
> +    
> +    // Base height above which the shape of the root changes
> +    int thresholdHeight = static_cast<int>(thresholdBaseHeight * style()->fontSize());
> +    int topStartShift = 0;
> +    
> +    if (maxHeight > thresholdHeight && thresholdHeight) {
> +        float shift = (maxHeight - thresholdHeight) / static_cast<float>(thresholdHeight);
> +        if (shift > 1.)
> +            shift = 1.;
> +        topStartShift = static_cast<int>(0.5 * static_cast<int>(style()->fontSize() * 0.75) * shift);
> +        
> +        style()->setPaddingBottom(Length(static_cast<int>(0.2 * style()->fontSize()), Fixed));

More named constants here.

> +    }
> +    
> +    // Positioning of the index
> +    RenderBoxModelObject* indexBox = toRenderBoxModelObject(firstChild()->firstChild());
> +    
> +    int indexShift = indexBox->offsetWidth() + topStartShift;
> +    int rootMarginTop = static_cast<int>(0.375 * maxHeight) + style()->paddingBottom().value() + indexBox->offsetHeight() - (maxHeight + static_cast<int>(0.2 * style()->fontSize()));
> +    
> +    style()->setPaddingLeft(Length(indexShift, Fixed));
> +    if (rootMarginTop > 0)
> +        style()->setPaddingTop(Length(rootMarginTop + static_cast<int>(0.2 * style()->fontSize()), Fixed));

Same here.

> +    
> +    setNeedsLayoutAndPrefWidthsRecalc();
> +    markContainingBlocksForLayout();
> +    RenderBlock::layout();
> +    
> +    indexBox->style()->setBottom(Length(static_cast<int>(0.375 * (maxHeight)) + style()->paddingBottom().value(), Fixed));

Same here.

> +    
> +    indexBox->layout();
> +}
> +    
> +}
> +
> +#endif // ENABLE(MATHML)
> +
> +
> Index: WebCore/mathml/RenderMathMLSquareRoot.cpp
> ===================================================================
> --- WebCore/mathml/RenderMathMLSquareRoot.cpp	(revision 0)
> +++ WebCore/mathml/RenderMathMLSquareRoot.cpp	(revision 0)

> +
> +const int radicalBasePad = 3;
> +const float thresholdBaseHeight = 1.5;

Change the names to be prefixed with a 'g' as before.

> +    
> +RenderMathMLSquareRoot::RenderMathMLSquareRoot(Node *expression) 
> +    : RenderMathMLBlock(expression) 
> +{
> +}
> +
> +void RenderMathMLSquareRoot::paint(PaintInfo& info, int tx, int ty)
> +{
> +    RenderMathMLBlock::paint(info, tx, ty);
> +   
> +    tx += x();
> +    ty += y();
> +
> +    int maxHeight = 0;
> +    int width = 0;
> +    RenderObject* current = firstChild();
> +    while (current) {
> +        if (current->isBoxModelObject()) {
> +            
> +            RenderBoxModelObject* box = toRenderBoxModelObject(current);
> +            
> +            // Check to see if this box has a larger height
> +            if (box->offsetHeight() > maxHeight)
> +                maxHeight = box->offsetHeight();
> +            width += box->offsetWidth();
> +        }
> +        current = current->nextSibling();
> +    }
> +    // default to the font size in pixels if we're empty
> +    if (!maxHeight)
> +        maxHeight = style()->fontSize();
> +    
> +    int frontWidth = static_cast<int>(style()->fontSize() * 0.75);

use named constants.

> +    int topStartShift = 0;
> +    // Base height above which the shape of the root changes
> +    int thresholdHeight = static_cast<int>(thresholdBaseHeight * style()->fontSize());
> +    
> +    if (maxHeight > thresholdHeight && thresholdHeight) {
> +        float shift = (maxHeight - thresholdHeight) / static_cast<float>(thresholdHeight);
> +        if (shift > 1.)
> +            shift = 1.;
> +        topStartShift = static_cast<int>(0.5 * frontWidth * shift);
> +    }
> +    
> +    width += topStartShift;
> +    
> +    FloatPoint topStart(tx + frontWidth - topStartShift, ty);
> +    FloatPoint bottomLeft(tx + frontWidth * 0.5 , ty + maxHeight + radicalBasePad);
> +    FloatPoint topLeft(tx + frontWidth * 0.2 , ty + 0.5 * maxHeight);
> +    FloatPoint leftEnd(tx , topLeft.y() + 0.05 * style()->fontSize());
> +    
> +    info.context->save();
> +    
> +    info.context->setStrokeThickness(0.02 * style()->fontSize());

Same in all of the above.


> +    mask.addLineTo(FloatPoint(2 * topLeft.x() - leftEnd.x(), 2 * topLeft.y() - leftEnd.y()));

Same above.

> +    
> +    info.context->beginPath();
> +    info.context->addPath(mask);
> +    info.context->clip(mask);
> +    
> +    // Draw the thick part of the root.
> +    info.context->setStrokeThickness(0.1 * style()->fontSize());

Same above.


> +void RenderMathMLSquareRoot::layout()
> +{
> +    int maxHeight = 0;
> +    
> +    RenderObject* current = firstChild();
> +    while (current) {
> +        if (current->isBoxModelObject()) {
> +            RenderBoxModelObject* box = toRenderBoxModelObject(current);
> +            
> +            if (box->offsetHeight() > maxHeight)
> +                maxHeight = box->offsetHeight();
> +            
> +            box->style()->setVerticalAlign(BASELINE);
> +        }
> +        current = current->nextSibling();
> +    }
> +    
> +    if (!maxHeight)
> +        maxHeight = style()->fontSize();
> +
> +    
> +    if (maxHeight > static_cast<int>(thresholdBaseHeight * style()->fontSize()))
> +        style()->setPaddingBottom(Length(static_cast<int>(0.2 * style()->fontSize()), Fixed));

Same above.


In general, you should try to use global constants for all your calculations.  I went
back through that in the rest of the MathML code and tried to make that change.  Try
to use names like "gPaddingBottomFactor" rather than something more specific like
"gDoubleHeight".

Looks good otherwise.

[François Sausset]

Created attachment 53604 [details]
Updated patch

Numeric values have been moved to constants as suggested.
Only the value "2" has not been changed because it is not an adjustable parameter for the root drawing.

[Alex Milowski]

> ===================================================================
> --- WebCore/mathml/RenderMathMLRoot.cpp	(revision 0)
> +++ WebCore/mathml/RenderMathMLRoot.cpp	(revision 0)

[snip] 

> +
> +void RenderMathMLRoot::addChild(RenderObject* child, RenderObject* )
> +{
> +    if (isEmpty()) {
> +        // Add a block for the index
> +        RenderBlock* block = new (renderArena()) RenderBlock(node());
> +        RefPtr<RenderStyle> indexStyle = makeBlockStyle();
> +        indexStyle->setDisplay(INLINE_BLOCK);
> +        block->setStyle(indexStyle.release());
> +        RenderBlock::addChild(block);
> +        
> +        // FIXME: the wrapping does not seem to be needed anymore.
> +        // this is the base, so wrap it so we can pad it
> +        block = new (renderArena()) RenderBlock(node());
> +        RefPtr<RenderStyle> baseStyle = makeBlockStyle();
> +        baseStyle->setDisplay(INLINE_BLOCK);
> +//        baseStyle->setPaddingLeft(Length(5 * gRadicalWidth , Percent));

You shouldn't have commented out code in the patch.

> +        block->setStyle(baseStyle.release());
> +        RenderBlock::addChild(block);
> +        block->addChild(child);
> +    } else {
> +        // always add to the index
> +        firstChild()->addChild(child);
> +    }
> +}
> +    
> +void RenderMathMLRoot::paint(PaintInfo& info, int tx, int ty)
> +{
> +    RenderMathMLBlock::paint(info , tx , ty);
> +    
> +    tx += x();
> +    ty += y();
> +    
> +    RenderBoxModelObject* indexBox = toRenderBoxModelObject(lastChild());
> +    
> +    int maxHeight = indexBox->offsetHeight();
> +    if (!maxHeight) {
> +        // default to the font size in pixels if we're empty
> +        maxHeight = style()->fontSize();
> +    }

You really shouldn't have the braces around this just to have
the commented for the assignment.  

> +    int width = indexBox->offsetWidth();
> +    
> +    int indexWidth = 0;
> +    RenderObject* current = firstChild();
> +    while (current != lastChild()) {
> +        if (current->isBoxModelObject()) {
> +            RenderBoxModelObject* box = toRenderBoxModelObject(current);
> +            indexWidth += box->offsetWidth();
> +        }
> +        current = current->nextSibling();
> +    }
> +    
> +    int frontWidth = static_cast<int>(style()->fontSize() * gRadicalWidth);
> +    int topStartShift = 0;
> +    // Base height above which the shape of the root changes
> +    int thresholdHeight = static_cast<int>(gThresholdBaseHeight * style()->fontSize());
> +    
> +    if (maxHeight > thresholdHeight && thresholdHeight) {
> +        float shift = (maxHeight - thresholdHeight) / static_cast<float>(thresholdHeight);
> +        if (shift > 1.)
> +            shift = 1.;

I think you want 1.0 because you want to force a float comparison.  I don't see
anything in the style guide that says 1. is bad so it is up to you.

[François Sausset]

Created attachment 53668 [details]
Updated patch

I removed the comment, but let the FIXME, as it seems that part of the code is not needed anymore.
I also corrected the small style mistake.
About 1., it is indeed to force a float comparison.

[Kenneth Rohde Christiansen]

Comment on attachment 53668 [details]
Updated patch

Looks good to me, and it seems that Alex is fine with it as well

[WebKit Commit Bot]

Comment on attachment 53668 [details]
Updated patch

Rejecting patch 53668 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', '53668', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
', 'commit', '--all', '-F', '-'], input=message)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/system/executive.py", line 85, in run_command
    return Executive().run_command(*args, **kwargs)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/system/executive.py", line 186, in run_command
    string_to_communicate = str(input)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xe7' in position 16: ordinal not in range(128)

[Eric Seidel (no email)]

Sorry you were bitten by bug 37765.  I hope to have that fixed today!

[Eric Seidel (no email)]

Comment on attachment 53668 [details]
Updated patch

Sorry again for the trouble.

[Eric Seidel (no email)]

Comment on attachment 53668 [details]
Updated patch

The fix for bug 37765 was rolled out.  So this will fail to land.  Hopefully I'll reland the fix tomorrow at which point I'll cq+ this again.

[Eric Seidel (no email)]

Comment on attachment 53668 [details]
Updated patch

Fixed again.

[WebKit Commit Bot]

Comment on attachment 53668 [details]
Updated patch

Rejecting patch 53668 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kenneth Rohde Christiansen', u'--force']" exit_code: 1
Last 500 characters of output:
cpp
patching file WebCore/mathml/RenderMathMLSquareRoot.h
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/mathml/presentation/roots.xhtml
patching file LayoutTests/platform/mac/mathml/presentation/roots-expected.checksum
patching file LayoutTests/platform/mac/mathml/presentation/roots-expected.txt
error: pathspec 'LayoutTests/platform/mac/mathml/presentation/roots-expected.png' did not match any file(s) known to git.
Did you forget to 'git add'?

Full output: http://webkit-commit-queue.appspot.com/results/1831024

[Chris Jerdonek]

(In reply to comment #11)
> (From update of attachment 53668 [details])
> Rejecting patch 53668 from commit-queue.
> 
> Failed to run
> "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply',
> u'--reviewer', u'Kenneth Rohde Christiansen', u'--force']" exit_code: 1
> Last 500 characters of output:
> <snip>
> LayoutTests/platform/mac/mathml/presentation/roots-expected.checksum
> patching file LayoutTests/platform/mac/mathml/presentation/roots-expected.txt
> error: pathspec
> 'LayoutTests/platform/mac/mathml/presentation/roots-expected.png' did not match
> any file(s) known to git.
> Did you forget to 'git add'?
> 
> Full output: http://webkit-commit-queue.appspot.com/results/1831024

The last 500 characters of output is deceptive in that the final "pathspec" error message has nothing to do with the commit-queue failing.  That output is actually normal (but should probably be corrected to be suppressed).

This was the only problem file (perhaps the patch needs to be brought up to date?):

patching file WebCore/WebCore.xcodeproj/project.pbxproj
Hunk #2 succeeded at 5573 (offset 1 line).
Hunk #3 succeeded at 16346 (offset 3 lines).
Hunk #4 FAILED at 18880.
Hunk #5 FAILED at 21118.
2 out of 5 hunks FAILED -- saving rejects to file WebCore/WebCore.xcodeproj/project.pbxproj.rej

That's the file that caused exit code 1.

[Chris Jerdonek]

(In reply to comment #12)
> The last 500 characters of output is deceptive in that the final "pathspec"
> error message has nothing to do with the commit-queue failing.  That output is
> actually normal (but should probably be corrected to be suppressed).

I filed a bug for this issue here:

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

[François Sausset]

Created attachment 54179 [details]
Up-to-date version of the patch

Regenerated patch from trunk r58178.
It resolves the svn-apply problems.

[WebKit Commit Bot]

Comment on attachment 54179 [details]
Up-to-date version of the patch

Rejecting patch 54179 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
ed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe
svnlook: Can't write to stream: Broken pipe

    The following files contain tab characters:

        trunk/LayoutTests/mathml/presentation/roots.xhtml

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/git/libexec/git-core/git-svn line 570


Full output: http://webkit-commit-queue.appspot.com/results/1796067

[François Sausset]

Created attachment 54195 [details]
Small style fix

Tabs remained in the xhtml test file.

[François Sausset]

Created attachment 54424 [details]
Up-to-date version of the patch

Regenerated patch from trunk r58312 to avoid problems with svn-apply on .xcodeproj files.

[WebKit Commit Bot]

Comment on attachment 54424 [details]
Up-to-date version of the patch

Clearing flags on attachment: 54424

Committed r58349: <http://trac.webkit.org/changeset/58349>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.