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).
Created attachment 207381 [details] screenshot of testcase in Gecko and WebKit
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%.
(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.
(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)
Created attachment 227647 [details] Patch V1 Here is a first patch that applies on top of bug 130322.
Created attachment 227768 [details] Patch V2
Created attachment 227769 [details] Current Screenshot of roots.xhtml
Created attachment 227770 [details] Screenshot of roots.xhtml with the patch and Latin Modern Math
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).
Created attachment 227933 [details] Patch V3 Now with complete tests for add/remove child.
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.
(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
Created attachment 232614 [details] Patch V4
Created attachment 232627 [details] Patch V5
Created attachment 232656 [details] Patch
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
Committed r169939: <http://trac.webkit.org/changeset/169939>
Re-opened since this is blocked by bug 133878
Assertion: http://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK1%20%28Tests%29/builds/5870/steps/layout-test/logs/stdio
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
(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 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()
Committed r169963: <http://trac.webkit.org/changeset/169963>
Re-opened since this is blocked by bug 133899
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()
Committed r170005: <http://trac.webkit.org/changeset/170005>
Committed r170006: <http://trac.webkit.org/changeset/170006>