Bug 119038 – Draw radicals with glyphs for better rendering

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

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

Created attachment 207380 [details]
testcase

Currently, the radicals are drawn with lines and this gives a poor rendering. See the attached testcase and the screenshot comparing Gecko (left) and WebKit (right).

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

Created attachment 207381 [details]
screenshot of testcase in Gecko and WebKit

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

I think this is also the opportunity to refactor the whole RenderMathMLRoot code so that it behaves correctly with dynamic changes (I wonder whether it one the cause of bug 130353) and does not use CSS styles causing bug 126516.

@Martin: you said you wanted to work on this at some point. Did you make any progress?

The relevant MATH constants are:

RadicalVerticalGap

Space between the (ink) top of the expression and the bar over it.
Suggested: 11⁄4 default rule thickness.

RadicalDisplayStyleVertical

Space between the (ink) top of the expression and the bar over it.
Suggested: default rule thickness + 1⁄4 x-height.

RadicalRuleThickness

Thickness of the radical rule. This is the thickness of the rule in designed or constructed radical signs.
Suggested: default rule thickness.

RadicalExtraAscender

Extra white space reserved above the radical.
Suggested: RadicalRuleThickness.

RadicalKernBeforeDegree

Extra horizontal kern before the degree of a radical, if such is present.
Suggested: 5/18 of em.

RadicalKernAfterDegree

Negative kern after the degree of a radical, if such is present.
Suggested: −10/18 of em.

RadicalDegreeBottomRaise

Height of the bottom of the radical Percent degree, if such is present, in proportion to the ascender of the
radical sign.
Suggested: 60%.

[Martin Robinson]

(In reply to comment #2)

> @Martin: you said you wanted to work on this at some point. Did you make any progress?

Please go ahead. I did not get very far.

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

(In reply to comment #3)
> (In reply to comment #2)
> 
> > @Martin: you said you wanted to work on this at some point. Did you make any progress?
> 
> Please go ahead. I did not get very far.

So I tried adding the constuction

{ 0x221A, 0x20d3, 0x20d3, 0x23b7, 0x0    }, // radical

which seems to work with Asana Math and Cambria Math. However, for the old STIX set, the constructions must be done with different characters and fonts. So I'm not sure it's a good idea to do that until we can use the new fonts (bug 130322).

RenderMathMLRoot::addChild has child insertion that depends on the newChild CSS position, which can leads to weird rendering issues with msqrt/mroot if one forces the CSS style. However, I have not been able to produce a clear bug or security issue.

(I tried this:

- append absolute child c2 to an empty msqrt (this inserts the child outside the RenderMathMLRow wrapper)
- append a child c3 (this inserts it inside the RenderMathMLRow wrapper)
- insert c1 before c2 (as I read the code, this should append it to the RenderMathMLRow wrapper)
- remove c2

Then I expected the order of the renderer children to be c3, c1 instead of the DOM order c1, c3 ; but that does not seem to be the case)

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

Created attachment 227647 [details]
Patch V1

Here is a first patch that applies on top of bug 130322.

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

Created attachment 227768 [details]
Patch V2

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

Created attachment 227769 [details]
Current Screenshot of roots.xhtml

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

Created attachment 227770 [details]
Screenshot of roots.xhtml with the patch and Latin Modern Math

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

So I think I'm essentially done with the rewriting of the root code to draw the radical with a stretchy RenderMathMLOperator. The code uses all the radical parameters from the MATH table. I still need to write tests.

Some issues:

- Because of bug 130326, the spacing after the radical sign is sometimes too large.

- The alignment/thickness of the bar and radical sign may not match exactly in some cases. I think it's a limitation with pixel rounding etc, so I don't think that can be solved easily.

- Currently, the patch only works for fonts with a MATH table and breaks the support for radicals with other fonts. Some options: either ensure installation of MATH fonts or use some kind of fallback (scale transform, keeping the graphical drawing etc).

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

Created attachment 227933 [details]
Patch V3

Now with complete tests for add/remove child.

[Brent Fulgham]

Comment on attachment 227933 [details]
Patch V3

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

Did you want this to be reviewed and included?

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:38
> +#include "FontCache.h"

This isn't in proper alphabetical order. Does it need to be at the end of the list for some reason?

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:212
> +    m_degreeBottomRaisePercent = 0.6f;

This section is full of magic numbers. Should they be constants we define elsewhere to make it easier to tweak?

We also compute style().font().size() / 18 twice here.

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

(In reply to comment #11)
> Did you want this to be reviewed and included?
> 

This depends on bug 130322, which requires review, testing on other platforms and Apple's feedback about OpenType MATH fonts. In general, the order of review for my pending MathML patches is given by the status file in https://github.com/fred-wang/WebKitPatches

> > Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:212
> > +    m_degreeBottomRaisePercent = 0.6f;
> 
> This section is full of magic numbers. Should they be constants we define elsewhere to make it easier to tweak?

No, these magic constants are those of LaTeX. They also are the recommended default values in the OpenType MATH table. So the idea here is to use the default values unless we can get different values from the OpenType MATH table. The "tweaking" is left to the font designers.

See section 6.3.6 of http://mpeg.chiariglione.org/standards/mpeg-4/open-font-format/text-isoiec-cd-14496-22-3rd-edition

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

Created attachment 232614 [details]
Patch V4

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

Created attachment 232627 [details]
Patch V5

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

Created attachment 232656 [details]
Patch

[chris fleizach]

Comment on attachment 232656 [details]
Patch

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

looks ok overall

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:342
> +

extra unneeded line

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

Committed r169939: <http://trac.webkit.org/changeset/169939>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 133878

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

Assertion:

http://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK1%20%28Tests%29/builds/5870/steps/layout-test/logs/stdio

[Andy Estes]

r169939 also is causing 21 new layout test failures on the Mac. See http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r169939%20(5194)/results.html

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

(In reply to comment #20)
> r169939 also is causing 21 new layout test failures on the Mac. See http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r169939%20(5194)/results.html

That seems to be "crashes" due to the same assertion.

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

Comment on attachment 232656 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLRoot.h:90
> +    virtual bool isRenderMathMLRoot() const override final { return true; }

So this should have been isRenderMathMLRootWrapper()

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

Committed r169963: <http://trac.webkit.org/changeset/169963>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 133899

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

00:57:11.248 87464 worker/0 mathml/roots-removeChild.html crashed, (stderr lines):
00:57:11.248 87464   ASSERTION FAILED: mainAxisExtent - mainAxisBorderAndPaddingExtentForChild(child) >= 0
00:57:11.248 87464   /Volumes/Data/slave/mavericks-debug/build/Source/WebCore/rendering/RenderFlexibleBox.cpp(678) : WebCore::LayoutUnit WebCore::RenderFlexibleBox::preferredMainAxisContentExtentForChild(WebCore::RenderBox &, bool)
00:57:11.248 87464   1   0x113551fb0 WTFCrash
00:57:11.248 87464   2   0x115ee1953 WebCore::RenderFlexibleBox::preferredMainAxisContentExtentForChild(WebCore::RenderBox&, bool)
00:57:11.248 87464   3   0x115ee1bd1 WebCore::RenderFlexibleBox::computeNextFlexLine(WTF::Vector<WebCore::RenderBox*, 0ul, WTF::CrashOnOverflow>&, WebCore::LayoutUnit&, double&, double&, WebCore::LayoutUnit&, bool&)
00:57:11.248 87464   4   0x115edec0e WebCore::RenderFlexibleBox::layoutFlexItems(bool, WTF::Vector<WebCore::RenderFlexibleBox::LineContext, 0ul, WTF::CrashOnOverflow>&)
00:57:11.248 87464   5   0x115ede741 WebCore::RenderFlexibleBox::layoutBlock(bool, WebCore::LayoutUnit)
00:57:11.248 87464   6   0x115e06a1d WebCore::RenderBlock::layout()
00:57:11.248 87464   7   0x115fce549 WebCore::RenderMathMLRow::layout()
00:57:11.248 87464   8   0x114f5b14c WebCore::RenderElement::layoutIfNeeded()

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

Committed r170005: <http://trac.webkit.org/changeset/170005>

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

Committed r170006: <http://trac.webkit.org/changeset/170006>