Bug 119038 - Draw radicals with glyphs for better rendering
Draw radicals with glyphs for better rendering
 Status: RESOLVED FIXED None WebKit Unclassified MathML (show other bugs) 528+ (Nightly build) All All P2 Normal Frédéric Wang (:fredw) 130322 133557 133878 133899 mathml-in-mathjax 99623 106866 115789 126516 130353 133603 133604 134018 134020 Show dependency tree / graph

 Reported: 2013-07-24 01:09 PDT by Frédéric Wang (:fredw) 2014-06-18 00:33 PDT (History) 16 users (show) aestes bfulgham bunhere cdumez cfleizach commit-queue dbarton esprehn+autocc glenn gyuyoung.kim kondapallykalyan macpherson menard mrobinson rakuco sergio

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.
 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). Frédéric Wang (:fredw) 2013-07-24 01:09:42 PDT Created attachment 207381 [details] screenshot of testcase in Gecko and WebKit 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%. 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. 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) 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. Frédéric Wang (:fredw) 2014-03-25 11:09:19 PDT Created attachment 227768 [details] Patch V2 Frédéric Wang (:fredw) 2014-03-25 11:09:54 PDT Created attachment 227769 [details] Current Screenshot of roots.xhtml 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 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). 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. 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. 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 Frédéric Wang (:fredw) 2014-06-06 06:51:33 PDT Created attachment 232614 [details] Patch V4 Frédéric Wang (:fredw) 2014-06-06 14:16:09 PDT Created attachment 232627 [details] Patch V5 Frédéric Wang (:fredw) 2014-06-07 01:42:10 PDT Created attachment 232656 [details] Patch 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 Frédéric Wang (:fredw) 2014-06-13 11:08:49 PDT Committed r169939:  WebKit Commit Bot 2014-06-13 13:30:36 PDT Re-opened since this is blocked by bug 133878 Frédéric Wang (:fredw) 2014-06-13 13:36:16 PDT Assertion: http://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK1%20%28Tests%29/builds/5870/steps/layout-test/logs/stdio 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 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. 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() Frédéric Wang (:fredw) 2014-06-13 23:19:23 PDT Committed r169963:  WebKit Commit Bot 2014-06-14 00:59:40 PDT Re-opened since this is blocked by bug 133899 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::LayoutUnit&, double&, double&, WebCore::LayoutUnit&, bool&) 00:57:11.248 87464 4 0x115edec0e WebCore::RenderFlexibleBox::layoutFlexItems(bool, WTF::Vector&) 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) 2014-06-16 03:08:10 PDT Committed r170005:  Frédéric Wang (:fredw) 2014-06-16 04:32:12 PDT Committed r170006: