Bug 119038 - Draw radicals with glyphs for better rendering
: Draw radicals with glyphs for better rendering
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: MathML
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Frédéric Wang (:fredw)
:
Depends on: 130322 133557 133878 133899
Blocks: mathml-in-mathjax 99623 130353 133603 134018 134020 106866 115789 126516 133604
  Show dependency treegraph
 
Reported: 2013-07-24 01:09 PDT by Frédéric Wang (:fredw)
Modified: 2014-06-18 00:33 PDT (History)
16 users (show)

See Also:


Attachments
testcase (516 bytes, text/html)
2013-07-24 01:09 PDT, Frédéric Wang (:fredw)
no flags Details
screenshot of testcase in Gecko and WebKit (4.08 KB, image/png)
2013-07-24 01:09 PDT, Frédéric Wang (:fredw)
no flags Details
Patch V1 (29.85 KB, patch)
2014-03-24 08:33 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch V2 (34.14 KB, patch)
2014-03-25 11:09 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Current Screenshot of roots.xhtml (37.37 KB, image/png)
2014-03-25 11:09 PDT, Frédéric Wang (:fredw)
no flags Details
Screenshot of roots.xhtml with the patch and Latin Modern Math (29.89 KB, image/png)
2014-03-25 11:10 PDT, Frédéric Wang (:fredw)
no flags Details
Patch V3 (58.95 KB, patch)
2014-03-27 01:42 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch V4 (51.07 KB, patch)
2014-06-06 06:51 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch V5 (262.56 KB, patch)
2014-06-06 14:16 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (309.26 KB, patch)
2014-06-07 01:42 PDT, Frédéric Wang (:fredw)
cfleizach: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2013-07-24 01:09:12 PDT
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).
Comment 1 Frédéric Wang (:fredw) 2013-07-24 01:09:42 PDT
Created attachment 207381 [details]
screenshot of testcase in Gecko and WebKit
Comment 2 Frédéric Wang (:fredw) 2014-03-18 03:18:11 PDT
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%.
Comment 3 Martin Robinson 2014-03-18 06:51:43 PDT
(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.
Comment 4 Frédéric Wang (:fredw) 2014-03-20 02:23:37 PDT
(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)
Comment 5 Frédéric Wang (:fredw) 2014-03-24 08:33:15 PDT
Created attachment 227647 [details]
Patch V1

Here is a first patch that applies on top of bug 130322.
Comment 6 Frédéric Wang (:fredw) 2014-03-25 11:09:19 PDT
Created attachment 227768 [details]
Patch V2
Comment 7 Frédéric Wang (:fredw) 2014-03-25 11:09:54 PDT
Created attachment 227769 [details]
Current Screenshot of roots.xhtml
Comment 8 Frédéric Wang (:fredw) 2014-03-25 11:10:29 PDT
Created attachment 227770 [details]
Screenshot of roots.xhtml with the patch and Latin Modern Math
Comment 9 Frédéric Wang (:fredw) 2014-03-25 11:17:21 PDT
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).
Comment 10 Frédéric Wang (:fredw) 2014-03-27 01:42:30 PDT
Created attachment 227933 [details]
Patch V3

Now with complete tests for add/remove child.
Comment 11 Brent Fulgham 2014-04-29 16:58:11 PDT
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.
Comment 12 Frédéric Wang (:fredw) 2014-04-30 00:28:45 PDT
(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
Comment 13 Frédéric Wang (:fredw) 2014-06-06 06:51:33 PDT
Created attachment 232614 [details]
Patch V4
Comment 14 Frédéric Wang (:fredw) 2014-06-06 14:16:09 PDT
Created attachment 232627 [details]
Patch V5
Comment 15 Frédéric Wang (:fredw) 2014-06-07 01:42:10 PDT
Created attachment 232656 [details]
Patch
Comment 16 chris fleizach 2014-06-13 09:59:00 PDT
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
Comment 17 Frédéric Wang (:fredw) 2014-06-13 11:08:49 PDT
Committed r169939: <http://trac.webkit.org/changeset/169939>
Comment 18 WebKit Commit Bot 2014-06-13 13:30:36 PDT
Re-opened since this is blocked by bug 133878
Comment 20 Andy Estes 2014-06-13 13:57:59 PDT
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
Comment 21 Frédéric Wang (:fredw) 2014-06-13 23:03:53 PDT
(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.
Comment 22 Frédéric Wang (:fredw) 2014-06-13 23:19:07 PDT
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()
Comment 23 Frédéric Wang (:fredw) 2014-06-13 23:19:23 PDT
Committed r169963: <http://trac.webkit.org/changeset/169963>
Comment 24 WebKit Commit Bot 2014-06-14 00:59:40 PDT
Re-opened since this is blocked by bug 133899
Comment 25 Frédéric Wang (:fredw) 2014-06-14 01:03:02 PDT
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()
Comment 26 Frédéric Wang (:fredw) 2014-06-16 03:08:10 PDT
Committed r170005: <http://trac.webkit.org/changeset/170005>
Comment 27 Frédéric Wang (:fredw) 2014-06-16 04:32:12 PDT
Committed r170006: <http://trac.webkit.org/changeset/170006>