MathML: nested square root symbols have varying descenders

• Component: MathML
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Created attachment 64057 [details]
Test case

* SUMMARY
When loading the attached test case, the bottom of the square root symbols different in how far below the baseline they're drawn.  This seems like a bug.

* STEPS TO REPRODUCE
1. Open the attached test case on a WebKit nightly build.

* RESULTS
Note that the "descender" of the square root symbol varies depending on which one you look at.

* REGRESSION
N/A

[François Sausset]

In the test case, only the first and the last root are problematic.
For the left one, the problem is known and it is due to the fact that you have more than one element inside mroot/msqrt. To fix it, we have to implement inferred mrow that will wrap around the child elements.
The implementation was planned and I hope to have time to do it soon.
For the right root, I'll investigate.

[François Sausset]

The problem for the square root around the "A" (the one on the right) is only appearing with STIX fonts and not with Apple Symbol one.
Strange...

The problem due to the lack of the inferred mrow implementation is font independent, as expected.

[Alex Milowski]

I believe this has been address by the patch from Bug 43588.  Can you please test again and let me know if you accept the rendering?

[François Sausset]

(In reply to comment #3)
> I believe this has been address by the patch from Bug 43588.  Can you please test again and let me know if you accept the rendering?

Indeed, the problem is now solved by this patch when using Apple Symbols font.
With STIX fonts, the different problem occurring around the "A" in the test is still there, but it should be solved by a patch that I have in preparation.

[Dave Barton]

This appears fixed now to me, probably due to the fix for bug 83736. At least the right radical is now shorter, and the left radicals grow by design. Note that TeX also has large radical signs have larger descenders like this, though Firefox does not.

Any objections to closing this bug?

[Dave Barton]

Created attachment 142269 [details]
mroot & msubsup test case

msqrt descenders seem ok in the latest nightlies, but mroot ones are not! Here are some examples from a couple of mathml LayoutTests. It's tricky to catch this, because in DumpRenderTree these examples look fine. This is explained in the ChangeLog for the patch I'm about to upload - RenderInline::offsetHeight() for a text span in the font STIXGeneral 16, for instance, may have an offsetHeight() of 24 in e.g. Safari or Chrome.

[Dave Barton]

Created attachment 142272 [details]
Patch

Comment on attachment 142272 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        This is hard to troubleshoot because for the STIXGeneral font, DumpRenderTree returns a
> +        similar result either way, whereas Safari and Chrome for instance may return much
> +        different values. This can be seen by looking at the existing test files roots.xhtml and

Why does WebKit behavior differ between Safari and DumpRenderTree?

(In reply to comment #8)
> (From update of attachment 142272 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142272&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        This is hard to troubleshoot because for the STIXGeneral font, DumpRenderTree returns a
> > +        similar result either way, whereas Safari and Chrome for instance may return much
> > +        different values. This can be seen by looking at the existing test files roots.xhtml and
> 
> Why does WebKit behavior differ between Safari and DumpRenderTree?

Does STIX need to be added to allowedFontFamilySet() in DumpRenderTree.mm (and the corresponding set in WebKitTestRunner)?

[Dave Barton]

> Why does WebKit behavior differ between Safari and DumpRenderTree?

> Does STIX need to be added to allowedFontFamilySet() in DumpRenderTree.mm (and the corresponding set in WebKitTestRunner)?

Thanks for asking yourself these questions so I didn't have to! :) Seriously, this is why I added you to this bug, and I greatly appreciate your expert help. I will investigate this given your pointers, or any other info you experts think I need ...

[Dave Barton]

Comment on attachment 142272 [details]
Patch

I am reworking the patch using { -webkit-line-box-contain: glyphs replaced } - see <http://dev.w3.org/csswg/css3-linebox/#LineStacking>. This is exciting, and seems clearly to be the right approach for MathML rendering, but it does change several things.

[Dave Barton]

Created attachment 147950 [details]
Patch

[Julien Chaffraix]

Comment on attachment 147950 [details]
Patch

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

Global comments as I don't the line box layout much.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:79
> +    virtual LayoutUnit baselinePosition(FontBaseline, bool firstLine, LineDirectionMode, LinePositionMode = PositionOnContainingLine) const;

OVERRIDE

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:334
> +PassRefPtr<RenderStyle> RenderMathMLOperator::createStackableStyle(int /* lineHeight */, int maxHeightForRenderer, int topRelative)

Nit: Just remove the parameter name |lineHeight| and move the FIXME to the function declaration.

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:170
> -    int baseHeight = roundToInt(getBoxModelObjectHeight(firstChild()));
> +    int baseHeight = contentLogicalHeight();

You still need to round contentLogicalHeight as it returns a LayoutUnit.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:149
> +    LayoutUnit baseWrapperBaseline = toRenderBox(firstChild())->firstLineBoxBaseline();

You can use: baseWrapper->firstLineBoxBaseline()

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:150
> +    LayoutUnit baseBaseline = baseWrapperBaseline - baseWrapper->paddingTop();

For consistency with the rest of the code who is writing-mode aware, it should be paddingBefore. Note that the rest of the code doesn't look really 100% writing mode proof so it shouldn't block the patch but at least that avoids one issue.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:153
> +    LayoutUnit superscriptHeight = superscriptWrapper->logicalHeight() - superscriptWrapper->paddingBottom();

Same comment as above it should be paddingAfter()

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:158
> +        /* fall through */

For one line comment, WebKit uses // not /* */

This is very weird that over uses the under computation. I would have expected the other around for UnderOver.

[Dave Barton]

Comment on attachment 147950 [details]
Patch

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

Thanks as always for the review, Julien!

Sorry this patch looks so big, but hopefully it will be the last one to rebaseline all the MathML tests for a while.

>> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:149
>> +    LayoutUnit baseWrapperBaseline = toRenderBox(firstChild())->firstLineBoxBaseline();
> 
> You can use: baseWrapper->firstLineBoxBaseline()

That doesn't compile:
RenderBlock.h:425: error: 'virtual WebCore::LayoutUnit WebCore::RenderBlock::firstLineBoxBaseline() const' is protected

>> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:158
>> +        /* fall through */
> 
> For one line comment, WebKit uses // not /* */
> 
> This is very weird that over uses the under computation. I would have expected the other around for UnderOver.

To compute the UnderOver baseline, one must use the base's baseline in any case, plus the overscript's height if it exists.

[Dave Barton]

Created attachment 148962 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 148962 [details]
Patch

LGTM.  Thanks.

[Eric Seidel (no email)]

Sorry this got ignore so long.  I've not been watching teh general review queue as closely as I should be.

[WebKit Review Bot]

Comment on attachment 148962 [details]
Patch

Rejecting attachment 148962 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
cted.txt
patching file LayoutTests/platform/mac/mathml/presentation/tables-expected.txt
patching file LayoutTests/platform/mac/mathml/presentation/tokenElements-expected.txt
patching file LayoutTests/platform/mac/mathml/presentation/under-expected.txt
patching file LayoutTests/platform/mac/mathml/presentation/underover-expected.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Seidel']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13415354

[Dave Barton]

Thanks for the r+! I'll merge in the couple test rebaselines from bug 91677 and then try to land the result.

[Dave Barton]

Committed r124512: <http://trac.webkit.org/changeset/124512>