Bug 85729 (menclose)

Summary:

Add support for menclose element

Product:

WebKit

Reporter:

Frédéric Wang (:fredw) <fred.wang>

Component:

MathML

Assignee:

gur.trio

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aboxhall, apinheiro, buildbot, bunhere, cfleizach, cmarcelo, commit-queue, darin, dbarton, distler, dmazzoni, eflews.bot, esprehn+autocc, fred.wang, glenn, gtk-ews, gur.trio, gyuyoung.kim, jdiggs, kangil.han, kondapallykalyan, macpherson, mario, menard, mrobinson, rakuco, rego+ews, rniwa, xan.lopez

Priority:

Keywords:

WebExposed

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

URL:

http://www.w3.org/TR/MathML/chapter3.html#presm.menclose

Bug Depends on:

Bug Blocks:

84019

Attachments:

Description	Flags
testcase	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion	none
Patch	none
Patch	none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
diagram and equation explaining where \sqrt{2} comes from	none
Patch	none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion	none
Patch	none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion	none
Patch	none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion	none
Patch	none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2	none
Patch	none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion	none
Patch	none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2	none
Patch	none
Patch	none

Frédéric Wang (:fredw)

Reported 2012-05-06 07:35:09 PDT

"The menclose element renders its content inside the enclosing notation specified by its notation attribute. menclose accepts a single argument possibly being an inferred mrow of multiple children" MathJax relies on the menclose element to implement various LaTeX commands like \enclose, \boxed, \cancel, \bcancel, \xcancel or \cancelto. Other examples include "\begin{array}{c}\hline ... \end{array}" or "\begin{array}{c}\hdashline ... \end{array}". Some reftests: http://devel.mathjax.org/testing/testsuite/MathMLToDisplay/Presentation/GeneralLayout/menclose/ A testcase: http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/menclose/menclose-cancellation1-simple.xhtml

Attachments
testcase (741 bytes, application/xhtml+xml) 2012-10-28 21:04 PDT, Jacques Distler	no flags	Details
Patch (81.29 KB, patch) 2013-09-28 02:07 PDT, gur.trio	no flags	Details Formatted Diff Diff
Patch (91.19 KB, patch) 2013-09-28 04:27 PDT, gur.trio	no flags	Details Formatted Diff Diff
Patch (91.19 KB, patch) 2013-09-28 06:13 PDT, gur.trio	no flags	Details Formatted Diff Diff
Patch (91.20 KB, patch) 2013-09-28 06:42 PDT, gur.trio	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (508.09 KB, application/zip) 2013-09-28 08:24 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (531.26 KB, application/zip) 2013-09-28 09:51 PDT, Build Bot	no flags	Details
Patch (91.88 KB, patch) 2013-09-30 08:17 PDT, gur.trio	no flags	Details Formatted Diff Diff
Patch (91.89 KB, patch) 2013-09-30 08:37 PDT, gur.trio	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (518.55 KB, application/zip) 2013-09-30 09:27 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (529.00 KB, application/zip) 2013-09-30 09:48 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (558.60 KB, application/zip) 2013-09-30 10:50 PDT, Build Bot	no flags	Details
Patch (68.23 KB, patch) 2014-01-07 03:18 PST, gur.trio	no flags	Details Formatted Diff Diff
Patch (68.22 KB, patch) 2014-01-07 03:33 PST, gur.trio	no flags	Details Formatted Diff Diff
Patch (68.19 KB, patch) 2014-01-07 03:51 PST, gur.trio	no flags	Details Formatted Diff Diff
Patch (68.93 KB, patch) 2014-01-07 23:51 PST, gur.trio	no flags	Details Formatted Diff Diff
Patch (69.76 KB, patch) 2014-01-08 00:02 PST, gur.trio	no flags	Details Formatted Diff Diff
Patch (70.15 KB, patch) 2014-01-08 03:36 PST, gur.trio	no flags	Details Formatted Diff Diff
Patch (71.76 KB, patch) 2014-01-08 08:17 PST, gur.trio	no flags	Details Formatted Diff Diff
Patch (72.53 KB, patch) 2014-01-09 01:52 PST, gur.trio	no flags	Details Formatted Diff Diff
diagram and equation explaining where \sqrt{2} comes from (2.81 KB, text/html) 2014-01-09 03:17 PST, Frédéric Wang (:fredw)	no flags	Details
Patch (73.81 KB, patch) 2014-01-09 05:53 PST, gur.trio	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (482.25 KB, application/zip) 2014-01-09 06:47 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (513.78 KB, application/zip) 2014-01-09 07:09 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (512.27 KB, application/zip) 2014-01-09 08:10 PST, Build Bot	no flags	Details
Patch (73.82 KB, patch) 2014-01-10 03:43 PST, gur.trio	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (471.03 KB, application/zip) 2014-01-10 04:43 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (502.53 KB, application/zip) 2014-01-10 05:01 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (518.20 KB, application/zip) 2014-01-10 06:01 PST, Build Bot	no flags	Details
Patch (73.75 KB, patch) 2014-01-13 05:40 PST, gur.trio	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (472.25 KB, application/zip) 2014-01-13 06:35 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (502.38 KB, application/zip) 2014-01-13 06:57 PST, Build Bot	no flags	Details
Patch (73.82 KB, patch) 2014-01-13 07:56 PST, gur.trio	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (556.74 KB, application/zip) 2014-01-13 08:32 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (471.09 KB, application/zip) 2014-01-13 08:50 PST, Build Bot	no flags	Details
Patch (73.76 KB, patch) 2014-01-13 22:51 PST, gur.trio	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (462.58 KB, application/zip) 2014-01-13 23:46 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (489.03 KB, application/zip) 2014-01-14 00:06 PST, Build Bot	no flags	Details
Patch (73.76 KB, patch) 2014-01-14 00:26 PST, gur.trio	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (467.31 KB, application/zip) 2014-01-14 01:28 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (490.77 KB, application/zip) 2014-01-14 01:45 PST, Build Bot	no flags	Details
Patch (73.75 KB, patch) 2014-01-14 01:49 PST, gur.trio	no flags	Details Formatted Diff Diff
Patch (84.61 KB, patch) 2014-01-16 01:06 PST, gur.trio	no flags	Details Formatted Diff Diff
Patch (86.66 KB, patch) 2014-01-19 23:14 PST, gur.trio	no flags	Details Formatted Diff Diff
Patch (87.85 KB, patch) 2014-01-20 20:03 PST, gur.trio	no flags	Details Formatted Diff Diff
Patch (86.15 KB, patch) 2014-01-22 02:07 PST, gur.trio	no flags	Details Formatted Diff Diff
Patch (83.29 KB, patch) 2014-01-22 22:23 PST, gur.trio	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (489.53 KB, application/zip) 2014-01-22 23:42 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (454.18 KB, application/zip) 2014-01-23 00:18 PST, Build Bot	no flags	Details
Patch (83.40 KB, patch) 2014-01-23 00:19 PST, gur.trio	no flags	Details Formatted Diff Diff
Patch (83.37 KB, patch) 2014-01-27 23:35 PST, gur.trio	no flags	Details Formatted Diff Diff
Show Obsolete (27) View All Add attachment proposed patch, testcase, etc.

Jacques Distler

Comment 1 2012-10-28 21:04:36 PDT

Created attachment 171152 [details] testcase

Jacques Distler

Comment 2 2012-10-28 21:07:37 PDT

As Frederic notes, several common mathematical notations rely (for their MathML implementations) on menclose.

gur.trio

Comment 3 2013-09-17 20:26:58 PDT

(In reply to comment #0) > "The menclose element renders its content inside the enclosing notation specified by its notation attribute. menclose accepts a single argument possibly being an inferred mrow of multiple children" > > MathJax relies on the menclose element to implement various LaTeX commands like \enclose, \boxed, \cancel, \bcancel, \xcancel or \cancelto. Other examples include "\begin{array}{c}\hline ... \end{array}" or "\begin{array}{c}\hdashline ... \end{array}". > > Some reftests: > http://devel.mathjax.org/testing/testsuite/MathMLToDisplay/Presentation/GeneralLayout/menclose/ > > A testcase: > http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/menclose/menclose-cancellation1-simple.xhtml Hi Frederic. For menclose the main attribute we need to support is notation which has different values such as box, circle etc. So for the implementation what I thought was we can create the element and while parsing the attribute if values are like left, right, top, bottom, acturial, madruwb, rounder box we can add CSS border properties. For radical we can create RenderMathMLSquareRoot as anonymous parent and for circle, updiagonalstrike, downdiagonalstrike, verticalstrike, verticalstrike, longdiv we can take care in paint of menclose. Please suggest?

Frédéric Wang (:fredw)

Comment 4 2013-09-18 09:21:42 PDT

(In reply to comment #3) > Hi Frederic. For menclose the main attribute we need to support is notation which has different values such as box, circle etc. So for the implementation what I thought was we can create the element and while parsing the attribute if values are like left, right, top, bottom, acturial, madruwb, rounder box we can add CSS border properties. For radical we can create RenderMathMLSquareRoot as anonymous parent and for circle, updiagonalstrike, downdiagonalstrike, verticalstrike, verticalstrike, longdiv we can take care in paint of menclose. > > Please suggest? Hi Gurpreet. I think you're correct, there are essentially three cases: 1) left, right, top, bottom, etc that can be drawn with CSS border 2) radical and longdiv that can be drawn with one stretchy char (radical and parenthesis) + a stretchy horizontal bar 3) other notations that are painted in the renderer. Currently msqrt does 3) but I'd like to change that (bug 119038). However, 2) is currently blocked by a work-in-progress to refactor the stretchy code, so that can be for later. I would open a new bug to start with the easiest case 1). For that one, you'll have to define the menclose element and notation attribute in mathtags.in and mathattrs.in. Map menclose to MathMLInlineContainerElement so that it behaves like an mrow. Then implement MathMLElement::collectStyleForPresentationAttribute somewhere (like in MathElement.cpp) so that it handles the case hasLocalName(mencloseTag) and name == notationAttr. This should map to the appropriate CSS property, I guess CSSPropertyBorder* or something.

Frédéric Wang (:fredw)

Comment 5 2013-09-18 09:34:52 PDT

(In reply to comment #4) Perhaps MathMLElement::collectStyleForPresentationAttribute is not really a good idea. Anyway, I'd suggest to open a but to implement the basic notations 1), prepare the implementation and continue the discussion there. One thing to note is that menclose@notation can have several values some may be repeated several times ; some are equivalent like box = top + bottom + left + right. So it would be good to have a parsing function that converts from the string to a bit mask. See http://dxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmencloseFrame.cpp#l86

gur.trio

Comment 6 2013-09-18 20:42:30 PDT

(In reply to comment #5) > (In reply to comment #4) > > Perhaps MathMLElement::collectStyleForPresentationAttribute is not really a good idea. Anyway, I'd suggest to open a but to implement the basic notations 1), prepare the implementation and continue the discussion there. One thing to note is that menclose@notation can have several values some may be repeated several times ; some are equivalent like box = top + bottom + left + right. So it would be good to have a parsing function that converts from the string to a bit mask. See http://dxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmencloseFrame.cpp#l86 Hi Frederic. Thanks for the suggestion. Once you create the bug let me know. Shall start working on that.

Frédéric Wang (:fredw)

Comment 7 2013-09-19 02:16:22 PDT

(In reply to comment #6) > Hi Frederic. Thanks for the suggestion. Once you create the bug let me know. Shall start working on that. Feel free to open the bug yourself and to submit a WIP patch there.

gur.trio

Comment 8 2013-09-28 02:07:16 PDT

Created attachment 212887 [details] Patch

Build Bot

Comment 9 2013-09-28 02:33:59 PDT

Comment on attachment 212887 [details] Patch Attachment 212887 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2655411

Build Bot

Comment 10 2013-09-28 02:48:30 PDT

Comment on attachment 212887 [details] Patch Attachment 212887 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2613444

gur.trio

Comment 11 2013-09-28 04:27:47 PDT

Created attachment 212896 [details] Patch

Build Bot

Comment 12 2013-09-28 04:57:13 PDT

Comment on attachment 212896 [details] Patch Attachment 212896 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2653449

Build Bot

Comment 13 2013-09-28 05:08:48 PDT

Comment on attachment 212896 [details] Patch Attachment 212896 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2692288

gur.trio

Comment 14 2013-09-28 06:13:52 PDT

Created attachment 212897 [details] Patch

Build Bot

Comment 15 2013-09-28 06:39:20 PDT

Comment on attachment 212897 [details] Patch Attachment 212897 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2660514

gur.trio

Comment 16 2013-09-28 06:42:12 PDT

Created attachment 212899 [details] Patch

Build Bot

Comment 17 2013-09-28 08:24:19 PDT

Comment on attachment 212899 [details] Patch Attachment 212899 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2654482 New failing tests: mathml/presentation/menclose-notation-right.html mathml/presentation/menclose-notation-actuarial.html mathml/presentation/menclose-notation-bottom.html mathml/presentation/menclose-notation-box.html mathml/presentation/menclose-notation-top.html mathml/presentation/menclose-notation-roundedbox.html mathml/presentation/menclose-notation-left.html mathml/presentation/menclose-notation-madruwb.html

Build Bot

Comment 18 2013-09-28 08:24:23 PDT

Created attachment 212901 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Build Bot

Comment 19 2013-09-28 09:51:20 PDT

Comment on attachment 212899 [details] Patch Attachment 212899 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2659523 New failing tests: mathml/presentation/menclose-notation-right.html mathml/presentation/menclose-notation-actuarial.html mathml/presentation/menclose-notation-bottom.html mathml/presentation/menclose-notation-box.html mathml/presentation/menclose-notation-top.html mathml/presentation/menclose-notation-roundedbox.html mathml/presentation/menclose-notation-left.html mathml/presentation/menclose-notation-madruwb.html

Build Bot

Comment 20 2013-09-28 09:51:23 PDT

Created attachment 212904 [details] Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Sam Weinig

Comment 21 2013-09-28 12:16:03 PDT

Comment on attachment 212899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212899&action=review > Source/WebCore/mathml/MathMLInlineContainerElement.cpp:87 > + if (hasLocalName(mencloseTag)) > + return new (arena) RenderMathMLMenclose(this); The normal model is override createRenderer in the derived class. You should probably do that here. > Source/WebCore/mathml/MathMLMencloseElement.cpp:47 > +// for notation = roundedbox we set the border radius (px) > +static const String borderRadius() > +{ > + DEFINE_STATIC_LOCAL(const String, radius, (ASCIILiteral("5px"))); > + return radius; > +} I don't think this is necessary. Just use ASCIILiteral("5px")) where you call borderRadius(). > Source/WebCore/mathml/MathMLMencloseElement.cpp:144 > + String padding; > + int fontSize = 0; > + String closingBrace = ")"; > + TextRun run(closingBrace.impl(), closingBrace.length()); > + Node* node = parentNode(); > + if (node && node->renderer() && node->renderer()->style()) { > + const Font& font = node->renderer()->style()->font(); > + fontSize = font.width(run); > + padding.append(String::number(fontSize)); > + padding.append("px"); > + } > + return padding; This should use StringBuilder. > Source/WebCore/mathml/MathMLMencloseElement.h:35 > +class MathMLMencloseElement : public MathMLInlineContainerElement { If no one ever inherits from this class, you should mark it FINAL. > Source/WebCore/mathml/MathMLMencloseElement.h:40 > + virtual bool isPresentationAttribute(const QualifiedName&) const OVERRIDE; > + virtual void collectStyleForPresentationAttribute(const QualifiedName&, const AtomicString&, MutableStylePropertySet*) OVERRIDE; > + virtual void finishParsingChildren() OVERRIDE; These can probably be private. > Source/WebCore/mathml/MathMLMencloseElement.h:42 > + Vector<String> getNotationValue() {return mNotationValues;} > + bool isRadical() {return mIsRadicalValue;} You need spaces after the { and before the }. > Source/WebCore/mathml/MathMLMencloseElement.h:45 > + MathMLMencloseElement(const QualifiedName& , Document&); Extraneous space after const QualifiedName&. > Source/WebCore/mathml/MathMLMencloseElement.h:49 > + Vector<String> mNotationValues; > + bool mIsRadicalValue; The style in WebCore is m_name, not mName. > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:49 > + MathMLMencloseElement* menclose = static_cast<MathMLMencloseElement*>(element()); You should add and use a toMathMLMencloseElement() which asserts the type. > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:66 > + if (element()) { When will there be no Element? If it is possible, you should probably check it for null in RenderMathMLMenclose::addChild as well. > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:67 > + MathMLMencloseElement* menclose = static_cast<MathMLMencloseElement*>(element()); You should add and use a toMathMLMencloseElement() which asserts the type. > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:95 > + if (equalIgnoringCase(notationVal[i], "updiagonalstrike")) > + info.context->drawLine(IntPoint(left, (top+boxHeight)), IntPoint((left+boxWidth), top)); > + else if (equalIgnoringCase(notationVal[i], "downdiagonalstrike")) > + info.context->drawLine(IntPoint(left, top), IntPoint((left+boxWidth), (top+boxHeight))); > + else if (equalIgnoringCase(notationVal[i], "verticalstrike")) > + info.context->drawLine(IntPoint((left+halfboxWidth), top), IntPoint((left+halfboxWidth), (top+boxHeight))); > + else if (equalIgnoringCase(notationVal[i], "horizontalstrike")) > + info.context->drawLine(IntPoint(left, (top+halfboxHeight)), IntPoint((left+boxWidth), (top+halfboxHeight))); You need spaces around your +s and you don't really need all the extra parenthesis. > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:35 > +// Render sqrt(base), using radical notation. Is this comment right? > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:42 > + virtual void paint(PaintInfo&, const LayoutPoint&) OVERRIDE; Does this need to be protected? Can it be private? > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:46 > + bool paintdependentAttribute(Vector<String> attr, int attrSize) const; This should probably take a const Vector<String>& to avoid a copy. The name is also odd and should probably use camelcase.

gur.trio

Comment 22 2013-09-30 08:17:38 PDT

Created attachment 212996 [details] Patch

gur.trio

Comment 23 2013-09-30 08:18:27 PDT

(In reply to comment #21) > (From update of attachment 212899 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212899&action=review > > > Source/WebCore/mathml/MathMLInlineContainerElement.cpp:87 > > + if (hasLocalName(mencloseTag)) > > + return new (arena) RenderMathMLMenclose(this); > > The normal model is override createRenderer in the derived class. You should probably do that here. > > > Source/WebCore/mathml/MathMLMencloseElement.cpp:47 > > +// for notation = roundedbox we set the border radius (px) > > +static const String borderRadius() > > +{ > > + DEFINE_STATIC_LOCAL(const String, radius, (ASCIILiteral("5px"))); > > + return radius; > > +} > > I don't think this is necessary. Just use ASCIILiteral("5px")) where you call borderRadius(). > > > Source/WebCore/mathml/MathMLMencloseElement.cpp:144 > > + String padding; > > + int fontSize = 0; > > + String closingBrace = ")"; > > + TextRun run(closingBrace.impl(), closingBrace.length()); > > + Node* node = parentNode(); > > + if (node && node->renderer() && node->renderer()->style()) { > > + const Font& font = node->renderer()->style()->font(); > > + fontSize = font.width(run); > > + padding.append(String::number(fontSize)); > > + padding.append("px"); > > + } > > + return padding; > > This should use StringBuilder. > > > Source/WebCore/mathml/MathMLMencloseElement.h:35 > > +class MathMLMencloseElement : public MathMLInlineContainerElement { > > If no one ever inherits from this class, you should mark it FINAL. > > > Source/WebCore/mathml/MathMLMencloseElement.h:40 > > + virtual bool isPresentationAttribute(const QualifiedName&) const OVERRIDE; > > + virtual void collectStyleForPresentationAttribute(const QualifiedName&, const AtomicString&, MutableStylePropertySet*) OVERRIDE; > > + virtual void finishParsingChildren() OVERRIDE; > > These can probably be private. > > > Source/WebCore/mathml/MathMLMencloseElement.h:42 > > + Vector<String> getNotationValue() {return mNotationValues;} > > + bool isRadical() {return mIsRadicalValue;} > > You need spaces after the { and before the }. > > > Source/WebCore/mathml/MathMLMencloseElement.h:45 > > + MathMLMencloseElement(const QualifiedName& , Document&); > > Extraneous space after const QualifiedName&. > > > Source/WebCore/mathml/MathMLMencloseElement.h:49 > > + Vector<String> mNotationValues; > > + bool mIsRadicalValue; > > The style in WebCore is m_name, not mName. > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:49 > > + MathMLMencloseElement* menclose = static_cast<MathMLMencloseElement*>(element()); > > You should add and use a toMathMLMencloseElement() which asserts the type. > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:66 > > + if (element()) { > > When will there be no Element? If it is possible, you should probably check it for null in RenderMathMLMenclose::addChild as well. > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:67 > > + MathMLMencloseElement* menclose = static_cast<MathMLMencloseElement*>(element()); > > You should add and use a toMathMLMencloseElement() which asserts the type. > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:95 > > + if (equalIgnoringCase(notationVal[i], "updiagonalstrike")) > > + info.context->drawLine(IntPoint(left, (top+boxHeight)), IntPoint((left+boxWidth), top)); > > + else if (equalIgnoringCase(notationVal[i], "downdiagonalstrike")) > > + info.context->drawLine(IntPoint(left, top), IntPoint((left+boxWidth), (top+boxHeight))); > > + else if (equalIgnoringCase(notationVal[i], "verticalstrike")) > > + info.context->drawLine(IntPoint((left+halfboxWidth), top), IntPoint((left+halfboxWidth), (top+boxHeight))); > > + else if (equalIgnoringCase(notationVal[i], "horizontalstrike")) > > + info.context->drawLine(IntPoint(left, (top+halfboxHeight)), IntPoint((left+boxWidth), (top+halfboxHeight))); > > You need spaces around your +s and you don't really need all the extra parenthesis. > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:35 > > +// Render sqrt(base), using radical notation. > > Is this comment right? > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:42 > > + virtual void paint(PaintInfo&, const LayoutPoint&) OVERRIDE; > > Does this need to be protected? Can it be private? > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:46 > > + bool paintdependentAttribute(Vector<String> attr, int attrSize) const; > > This should probably take a const Vector<String>& to avoid a copy. The name is also odd and should probably use camelcase. Hi Sam. Thanks for the detailed review. Uploaded new patch with the changes.

EFL EWS Bot

Comment 24 2013-09-30 08:23:12 PDT

Comment on attachment 212996 [details] Patch Attachment 212996 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/2774067

EFL EWS Bot

Comment 25 2013-09-30 08:24:14 PDT

Comment on attachment 212996 [details] Patch Attachment 212996 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/2782064

gur.trio

Comment 26 2013-09-30 08:37:55 PDT

Created attachment 212998 [details] Patch

Build Bot

Comment 27 2013-09-30 09:27:05 PDT

Comment on attachment 212998 [details] Patch Attachment 212998 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2640243 New failing tests: mathml/presentation/menclose-notation-right.html mathml/presentation/menclose-notation-actuarial.html mathml/presentation/menclose-notation-bottom.html mathml/presentation/menclose-notation-box.html mathml/presentation/menclose-notation-top.html mathml/presentation/menclose-notation-roundedbox.html mathml/presentation/menclose-notation-left.html mathml/presentation/menclose-notation-madruwb.html

Build Bot

Comment 28 2013-09-30 09:27:09 PDT

Created attachment 213004 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Build Bot

Comment 29 2013-09-30 09:48:16 PDT

Comment on attachment 212998 [details] Patch Attachment 212998 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2695162 New failing tests: mathml/presentation/menclose-notation-right.html mathml/presentation/menclose-notation-actuarial.html mathml/presentation/menclose-notation-bottom.html mathml/presentation/menclose-notation-box.html mathml/presentation/menclose-notation-top.html mathml/presentation/menclose-notation-roundedbox.html mathml/presentation/menclose-notation-left.html mathml/presentation/menclose-notation-madruwb.html

Build Bot

Comment 30 2013-09-30 09:48:19 PDT

Created attachment 213007 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Build Bot

Comment 31 2013-09-30 10:50:18 PDT

Comment on attachment 212998 [details] Patch Attachment 212998 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2722279 New failing tests: mathml/presentation/menclose-notation-right.html mathml/presentation/menclose-notation-actuarial.html mathml/presentation/menclose-notation-bottom.html mathml/presentation/menclose-notation-box.html mathml/presentation/menclose-notation-top.html mathml/presentation/menclose-notation-roundedbox.html mathml/presentation/menclose-notation-left.html mathml/presentation/menclose-notation-madruwb.html

Build Bot

Comment 32 2013-09-30 10:50:21 PDT

Created attachment 213012 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5

gur.trio

Comment 33 2013-10-02 03:41:58 PDT

(In reply to comment #26) > Created an attachment (id=212998) [details] > Patch Some test cases are failing on mac. In the expected.txt file there is "ab" in the same line whereas the mac results show ab one below the other hence the failure. Can someone please help me with this? As in why this difference?

gur.trio

Comment 34 2013-10-04 02:21:11 PDT

(In reply to comment #33) > (In reply to comment #26) > > Created an attachment (id=212998) [details] [details] > > Patch > > Some test cases are failing on mac. > In the expected.txt file there is "ab" in the same line whereas the mac results show ab one below the other hence the failure. > Can someone please help me with this? As in why this difference? Hi Sam. Can you please review? Thanks

gur.trio

Comment 35 2013-10-09 04:03:13 PDT

(In reply to comment #34) > (In reply to comment #33) > > (In reply to comment #26) > > > Created an attachment (id=212998) [details] [details] [details] > > > Patch > > > > Some test cases are failing on mac. > > In the expected.txt file there is "ab" in the same line whereas the mac results show ab one below the other hence the failure. > > Can someone please help me with this? As in why this difference? > > Hi Sam. Can you please review? Thanks Hi. Can someone please review this. Thanks

Frédéric Wang (:fredw)

Comment 36 2013-10-09 04:05:45 PDT

(In reply to comment #35) > > Hi. Can someone please review this. Thanks Hi Gurpreet. I'm really willing to look at your code but don't have much time right now (working on MathJax v2.3 testing this month). I promise I'll give feedback asap. Thanks again for working on this.

Frédéric Wang (:fredw)

Comment 37 2013-10-09 11:44:54 PDT

(In reply to comment #36) Quick remarks: - I'm wondering how relevant it is for the accessibility tools to just know about the fact that something is "menclose" without knowing the notations. Also, <menclose notation="radical"> should probably be treated as <msqrt> in the accessibility code too. - I think the menclose notations, and most MathML attributes, are case sensitive. - When someone uses a list of notation like "roundedbox madruwb" they should overlap around the inner child. I'm not sure that will be possible with your code, since some CSS rules are conflicting. You should probably have tests when multiple notations are used at the same time. - RenderMathMLMenclose::paint should not draw anything when the visibility is hidden, see bug 116600. Perhaps add tests for this and for the colors of the graphics. - The menclose width/height must be at least \sqrt(2) times as large as those of its child if you don't want overlap. Consider for example <math><menclose notation="circle"><mspace width="100px" height="50px" mathbackground="red"/></menclose></math>. Add a javascript test for that? - Perhaps have a test that checks the equivalence <menclose notation="radical"> and <msqrt>.

chris fleizach

Comment 38 2013-10-15 09:41:43 PDT

Comment on attachment 212998 [details] Patch I also don't think it is necessary for AX code to know about a menclose object itself. There's probably more nuanced behavior that would be needed. I'd be fine if you left the Accessibility changes out of this patch r- for Frédéric's comments and failing layout tests. I also don't know exactly why the tests would be failing, but feel free to upload patches to experiment with build bots

gur.trio

Comment 39 2013-11-17 23:50:58 PST

(In reply to comment #38) > (From update of attachment 212998 [details]) > I also don't think it is necessary for AX code to know about a menclose object itself. There's probably more nuanced behavior that would be needed. I'd be fine if you left the Accessibility changes out of this patch > > r- for Frédéric's comments and failing layout tests. > I also don't know exactly why the tests would be failing, but feel free to upload patches to experiment with build bots Thanks Frederic and Chris for the review. Sorry for the delayed reply. Was busy with some other work. Frederic I had some queries I think the menclose notations, and most MathML attributes, are case sensitive. : You mean smaller case it should apply and bigger case it should not? Is yes I observed in FF its applies for both case. When someone uses a list of notation like "roundedbox madruwb" they should overlap around the inner child. I'm not sure that will be possible with your code, since some CSS rules are conflicting. You should probably have tests when multiple notations are used at the same time. : I got your point. Can you suggest how can I achieve this? The menclose width/height must be at least \sqrt(2) times as large as those of its child if you don't want overlap ;Consider for example <math><menclose notation="circle"><mspace width="100px" height="50px" mathbackground="red"/></menclose></math>: You are right. Right now its overlapping. Need suggestion for this also how it can be achived? Through compute min/max pref height and width?

Frédéric Wang (:fredw)

Comment 40 2013-11-18 00:14:46 PST

(In reply to comment #39) > Frederic I had some queries > > I think the menclose notations, and most MathML attributes, are case sensitive. : You mean smaller case it should apply and bigger case it should not? Is yes I observed in FF its applies for both case. I mean "circle" is accepted since it is defined in the spec and in the RelaxNG schema but "CIRCLE" or "CirClE" are supposed to be different notations (and thus ignored). This would just be changing equalIgnoringCase. > When someone uses a list of notation like "roundedbox madruwb" they should overlap around the inner child. I'm not sure that will be possible with your code, since some CSS rules are conflicting. You should probably have tests when multiple notations are used at the same time. : I got your point. Can you suggest how can I achieve this? I suspect a general solution will be to draw all elements with the graphic libraries rather than mixing them with CSS properties. However as I told you, you might just want to start with a smaller bug that implements a partial support. Overlapping notations is probably less frequent in practice so of lower priority. > You are right. Right now its overlapping. Need suggestion for this also how it can be achived? Through compute min/max pref height and width? I think you only need to ensure the preferred min/max width of menclose to be sqrt(2) times as large as the preferred min/max width of its content ; and similarly for the actual width/height.

gur.trio

Comment 41 2013-11-18 00:22:25 PST

(In reply to comment #40) > (In reply to comment #39) > > Frederic I had some queries > > > > I think the menclose notations, and most MathML attributes, are case sensitive. : You mean smaller case it should apply and bigger case it should not? Is yes I observed in FF its applies for both case. > > I mean "circle" is accepted since it is defined in the spec and in the RelaxNG schema but "CIRCLE" or "CirClE" are supposed to be different notations (and thus ignored). This would just be changing equalIgnoringCase. > > > When someone uses a list of notation like "roundedbox madruwb" they should overlap around the inner child. I'm not sure that will be possible with your code, since some CSS rules are conflicting. You should probably have tests when multiple notations are used at the same time. : I got your point. Can you suggest how can I achieve this? > > I suspect a general solution will be to draw all elements with the graphic libraries rather than mixing them with CSS properties. However as I told you, you might just want to start with a smaller bug that implements a partial support. Overlapping notations is probably less frequent in practice so of lower priority. > > > You are right. Right now its overlapping. Need suggestion for this also how it can be achived? Through compute min/max pref height and width? > > I think you only need to ensure the preferred min/max width of menclose to be sqrt(2) times as large as the preferred min/max width of its content ; and similarly for the actual width/height. Thanks Fredric for the quick response. I am confused now :( Should I continue to work on this or not ?

Frédéric Wang (:fredw)

Comment 42 2013-11-18 00:33:51 PST

(In reply to comment #41) > Thanks Fredric for the quick response. I am confused now :( > Should I continue to work on this or not ? I think most of the comments will be easy to address, except perhaps handling mixing several notations and adding better accessibility support. My point is that you don't need to worry too much about these for a first implementation. They can be handled in separate bugs.

gur.trio

Comment 43 2014-01-07 03:18:21 PST

Created attachment 220506 [details] Patch

kov's GTK+ EWS bot

Comment 44 2014-01-07 03:26:20 PST

Comment on attachment 220506 [details] Patch Attachment 220506 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/5469383690289152

EFL EWS Bot

Comment 45 2014-01-07 03:29:54 PST

Comment on attachment 220506 [details] Patch Attachment 220506 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/4658955305353216

gur.trio

Comment 46 2014-01-07 03:33:53 PST

Created attachment 220507 [details] Patch

EFL EWS Bot

Comment 47 2014-01-07 03:39:38 PST

Comment on attachment 220507 [details] Patch Attachment 220507 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/5386102932242432

EFL EWS Bot

Comment 48 2014-01-07 03:43:27 PST

Comment on attachment 220507 [details] Patch Attachment 220507 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/5808315397308416

gur.trio

Comment 49 2014-01-07 03:51:36 PST

Created attachment 220508 [details] Patch

gur.trio

Comment 50 2014-01-07 05:07:50 PST

(In reply to comment #49) > Created an attachment (id=220508) [details] > Patch Hi Frederic and Chris. Sorry about the delay. I have updated new patch with the changes as per what you both had suggested. Few things still needs to be worked on, which I can feel we can handle as separate bugs of menclose. Please review. Thanks.

Frédéric Wang (:fredw)

Comment 51 2014-01-07 06:32:43 PST

Comment on attachment 220508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220508&action=review Thanks, I'll try to look into this asap. Can you update the LayoutTests/mathml/presentation/inferred-mrow-* tests to take into account menclose? > Source/WebCore/mathml/MathMLInlineContainerElement.cpp:89 > + return createRenderer<RenderMathMLUnderOver>(*this, std::move(style)); Not sure why it is RenderMathMLUnderOver but I guess it the menclose renderer is created in the derived class anyway...

Frédéric Wang (:fredw)

Comment 52 2014-01-07 06:40:58 PST

Comment on attachment 220508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220508&action=review > LayoutTests/mathml/presentation/menclose-notation-attribute-set2.html:43 > +</html> Unfortunately, you can not group mismatch tests (one mismatch of an menclose pair is enough to make the test pass) > LayoutTests/mathml/presentation/menclose-notation-no-overlap.html:11 > + isSuccessfullyParsed(); Perhaps you should say that these magic numbers are \sqrt(2) times the inner width/height. > LayoutTests/mathml/presentation/menclose-notation-radical.html:8 > + <math><menclose notation="radical"><msqrt><mspace width="100px" height="50px" mathbackground="red"/></msqrt></menclose></math> Do you really want a mismatch test here or rather that <menclose notation="radical"><mspace width="100px" height="50px" mathbackground="red"/></menclose> renders the same as <msqrt><mspace width="100px" height="50px" mathbackground="red"/></msqrt> ?

chris fleizach

Comment 53 2014-01-07 09:12:27 PST

Comment on attachment 220508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220508&action=review > Source/WebCore/dom/Element.h:473 > +#endif we probably don't want to put this in Element. The other MathML elements are referenced like this in MathMLElement.h > Source/WebCore/mathml/MathMLMencloseElement.cpp:3 > + * Copyright (C) 2013 The MathJax Consortium. if this is a new file why does it have a 2010 copyright? > Source/WebCore/mathml/MathMLMencloseElement.cpp:70 > + if (m_IsRadicalValue && (!firstElementChild())) the parens around firstElementChild() seem unnecessary can you add a comment why you need to add a child when you finish parsing what do the 0's stand for in addChild()? if they're pointers they should be nullptr > Source/WebCore/mathml/MathMLMencloseElement.cpp:80 > + int attrvaluesSize = m_NotationValues.size(); size() returns a size_t > Source/WebCore/mathml/MathMLMencloseElement.cpp:131 > + fontSize = font.width(run); does font.width() return an int? my guess would be that it'd return an unsigned > Source/WebCore/mathml/MathMLMencloseElement.h:2 > + * Copyright (C) 2010 Alex Milowski (alex@milowski.com). All rights reserved. ditto about copyright > Source/WebCore/mathml/MathMLMencloseElement.h:39 > + Vector<String>& getNotationValue() { return m_NotationValues; } i think you can drop the "get" prefix and name it notationValues() const > Source/WebCore/mathml/MathMLMencloseElement.h:40 > + bool isRadical() { return m_IsRadicalValue; } should be a const > Source/WebCore/mathml/MathMLMencloseElement.h:52 > + Vector<String> m_NotationValues; this should be m_notationValues > Source/WebCore/mathml/MathMLMencloseElement.h:53 > + bool m_IsRadicalValue; should be m_isRadicalValue > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:50 > + // For radical notation creating an anonymous RenderMathMLSquareRoot. this comment doesn't explain why you're doing this > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:56 > + toRenderElement(firstChild())->addChild(newChild, beforeChild && beforeChild->parent() == firstChild() ? beforeChild : 0); 0 should be nullptr > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:90 > + Vector<String>& notationVal = menclose->getNotationValue(); var name should be notationValues > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:91 > + int notationvalSize = notationVal.size(); size_t instead of int > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:93 > + isDefaultLongDiv = true; seems like this should be written as bool isDefaultLongDiv = !notatationalValueSize; > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:137 > +bool RenderMathMLMenclose::isValidNotationValue(Vector<String>& attr, int attrSize) const passing in attrSize seems unnecessary since you're already passing in attr, which can easily find the size of itself > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:139 > + for (int i = 0;i < attrSize; i++) { bad spacing > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:140 > + if (attr[i] == "updiagonalstrike" || attr[i] == "downdiagonalstrike" || attr[i] == "horizontalstrike" || attr[i] == "verticalstrike" since you only seem to do work in the above method when the attr name matches one of these, it seems like this check is superfluous. Anything not matching one of these would cause anything to happen > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:43 > + RenderMathMLSquareRoot* newSRoot = new RenderMathMLSquareRoot(toElement(*node), RenderStyle::createAnonymousStyleWithDisplay(&parent->style(), FLEX)); variable name should be something like squareRoot, or root > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:39 > + static RenderMathMLSquareRoot* createAnonymousWithParentRenderer(const RenderObject*); I have a feeling this should return a RenderPtr > LayoutTests/mathml/presentation/menclose-notation-attribute-set1.html:42 > + isSuccessfullyParsed(); bad indentation

gur.trio

Comment 54 2014-01-07 23:51:04 PST

Created attachment 220597 [details] Patch

gur.trio

Comment 55 2014-01-08 00:02:39 PST

Created attachment 220602 [details] Patch

gur.trio

Comment 56 2014-01-08 02:42:15 PST

(In reply to comment #55) > Created an attachment (id=220602) [details] > Patch Hi Frederic and Chris. Thanks for the review.Have made the changes as per the comments and uploaded new patch. But the build bots is not applying the patch :( > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:39 > + static RenderMathMLSquareRoot* createAnonymousWithParentRenderer(const RenderObject*); I have a feeling this should return a RenderPtr : Other files I checked there also its not returning RenderPtr e.g RenderMathMLRow. > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:140 > + if (attr[i] == "updiagonalstrike" || attr[i] == "downdiagonalstrike" || attr[i] == "horizontalstrike" || attr[i] == "verticalstrike" since you only seem to do work in the above method when the attr name matches one of these, it seems like this check is superfluous. Anything not matching one of these would cause anything to happen : Not very clear and cause anything to happen like?

Frédéric Wang (:fredw)

Comment 57 2014-01-08 02:47:46 PST

(In reply to comment #56) > > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:39 > > + static RenderMathMLSquareRoot* createAnonymousWithParentRenderer(const RenderObject*); > > I have a feeling this should return a RenderPtr : Other files I checked there also its not returning RenderPtr e.g RenderMathMLRow. > See https://bugs.webkit.org/show_bug.cgi?id=126631 https://bugs.webkit.org/show_bug.cgi?id=126583 for ongoing changes.

gur.trio

Comment 58 2014-01-08 02:51:00 PST

(In reply to comment #57) > (In reply to comment #56) > > > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:39 > > > + static RenderMathMLSquareRoot* createAnonymousWithParentRenderer(const RenderObject*); > > > > I have a feeling this should return a RenderPtr : Other files I checked there also its not returning RenderPtr e.g RenderMathMLRow. > > > > See > > https://bugs.webkit.org/show_bug.cgi?id=126631 > https://bugs.webkit.org/show_bug.cgi?id=126583 > > for ongoing changes. Ahh Ok :)

gur.trio

Comment 59 2014-01-08 03:36:55 PST

Created attachment 220622 [details] Patch

gur.trio

Comment 60 2014-01-08 08:17:31 PST

Created attachment 220635 [details] Patch

EFL EWS Bot

Comment 61 2014-01-08 08:22:51 PST

Comment on attachment 220635 [details] Patch Attachment 220635 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/6371233004257280

EFL EWS Bot

Comment 62 2014-01-08 08:26:00 PST

Comment on attachment 220635 [details] Patch Attachment 220635 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/6375007340986368

Build Bot

Comment 63 2014-01-08 08:53:45 PST

Comment on attachment 220635 [details] Patch Attachment 220635 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4963858120704000

Build Bot

Comment 64 2014-01-08 09:06:19 PST

Comment on attachment 220635 [details] Patch Attachment 220635 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5921713137123328

chris fleizach

Comment 65 2014-01-08 09:07:32 PST

(In reply to comment #64) > (From update of attachment 220635 [details]) > Attachment 220635 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.appspot.com/results/5921713137123328 /Volumes/Data/EWS/WebKit/Source/WebCore/mathml/MathMLMencloseElement.cpp:83:31: error: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare] for (int i = 0; i < attrvaluesSize; i++) { ~ ^ ~~~~~~~~~~~~~~ 1 error generated.

chris fleizach

Comment 66 2014-01-08 09:15:55 PST

Comment on attachment 220635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220635&action=review > Source/WebCore/mathml/MathMLMencloseElement.cpp:44 > + , m_notationValues(0) i don't think m_notationValues needs to be initialized. > Source/WebCore/mathml/MathMLMencloseElement.h:27 > +#define MATHMLMENCLOSELEMENT_H this should not be upper cased > Source/WebCore/mathml/MathMLMencloseElement.h:37 > + Vector<String>& notationValue() { return m_notationValues; } this method should be const and be called notationValues > Source/WebCore/mathml/MathMLMencloseElement.h:46 > + String longDivLeftPadding(); i think this method can be const > Source/WebCore/mathml/MathMLMencloseElement.h:48 > + virtual bool isMathMLMencloseElement() const { return true; } this should be OVERRIDE FINAL > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:92 > + if ((notationalValueSize && isValidNotationValue(notationVal)) || isDefaultLongDiv) { do you need to call isValidNotationValue() here? if you don't call isValidNotationValue() and you have an invalid attribute, does anything bad happen? it seems like the invalid attribute just won't cause any drawing to happen, but at least the right of the attributes will work > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:105 > + for (int i = 0; i < notationalValueSize; i++) { notationalValueSize is a size_t so i should be as well > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:135 > +bool RenderMathMLMenclose::isValidNotationValue(Vector<String>& attr) const this method name should indicate that it checks all the values in the array, so something like checkNotationalValuesValidity() right now the name seems like it operates on a single value > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:27 > +#define RENDERMATHMLMENCLOSE_H this should not be upper cased > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:46 > + bool isValidNotationValue(Vector<String>& attr) const; argument name is not necessary here

Frédéric Wang (:fredw)

Comment 67 2014-01-08 10:47:33 PST

(In reply to comment #66) > (From update of attachment 220635 [details]) > > Source/WebCore/mathml/MathMLMencloseElement.h:27 > > +#define MATHMLMENCLOSELEMENT_H > > this should not be upper cased > I got the same style error in bug 124838 comment 26. That seems to be a bug in the check-webkit-style script...

gur.trio

Comment 68 2014-01-09 01:51:44 PST

(In reply to comment #52) > (From update of attachment 220508 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=220508&action=review > Do you really want a mismatch test here or rather that > > <menclose notation="radical"><mspace width="100px" height="50px" mathbackground="red"/></menclose> > > renders the same as > > <msqrt><mspace width="100px" height="50px" mathbackground="red"/></msqrt> > > ? : I have reverted back to mismatch since the height of content incase of menclose is more as compared to <msqrt> test case.

gur.trio

Comment 69 2014-01-09 01:52:17 PST

Created attachment 220703 [details] Patch

gur.trio

Comment 70 2014-01-09 01:55:50 PST

(In reply to comment #66) > (From update of attachment 220635 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=220635&action=review > > > Source/WebCore/mathml/MathMLMencloseElement.h:27 > > +#define MATHMLMENCLOSELEMENT_H > > this should not be upper cased > Got style error yesterday but today I updated my webkit code and it doesnot show the style error. So new patch its reverted. > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:92 > > + if ((notationalValueSize && isValidNotationValue(notationVal)) || isDefaultLongDiv) { > > do you need to call isValidNotationValue() here? > if you don't call isValidNotationValue() and you have an invalid attribute, does anything bad happen? > it seems like the invalid attribute just won't cause any drawing to happen, but at least the right of the attributes will work Incase attribute value is invalid then paint wont happen but unnecessary calculations of boxWidth, boxHeight etc would happen. Not required right?

EFL EWS Bot

Comment 71 2014-01-09 01:56:18 PST

Comment on attachment 220703 [details] Patch Attachment 220703 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/6072233084583936

kov's GTK+ EWS bot

Comment 72 2014-01-09 02:00:52 PST

Comment on attachment 220703 [details] Patch Attachment 220703 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/5445130681057280

Frédéric Wang (:fredw)

Comment 73 2014-01-09 02:03:00 PST

(In reply to comment #68) > I have reverted back to mismatch since the height of content incase of menclose is more as compared to <msqrt> test case. Ideally <menclose notation="radical"> and <msqrt>. If that's too hard for this bug, perhaps you can keep the match reftest, mark it as known failure and open a follow-up bug. I don't think that's a too serious MathJax has the same bug (https://github.com/mathjax/MathJax/issues/101) and everybody uses <msqrt> anyway. Note however that in the two cases, you didn't close the <menclose notation="radical"> tag so that might explain your rendering issue.

gur.trio

Comment 74 2014-01-09 02:08:45 PST

(In reply to comment #73) > (In reply to comment #68) > > I have reverted back to mismatch since the height of content incase of menclose is more as compared to <msqrt> test case. > > Ideally <menclose notation="radical"> and <msqrt>. If that's too hard for this bug, perhaps you can keep the match reftest, mark it as known failure and open a follow-up bug. I don't think that's a too serious MathJax has the same bug (https://github.com/mathjax/MathJax/issues/101) and everybody uses <msqrt> anyway. > > Note however that in the two cases, you didn't close the <menclose notation="radical"> tag so that might explain your rendering issue. Actually the difference is because for menclose case we set the height and width to sqrt(2) times its child content which is more as compared to msqrt case. So dont think that will be similar in any case.

Frédéric Wang (:fredw)

Comment 75 2014-01-09 02:15:04 PST

(In reply to comment #74) > Actually the difference is because for menclose case we set the height and width to sqrt(2) times its child content which is more as compared to msqrt case. So dont think that will be similar in any case. This should be done *only* when the circle notation is used. Otherwise this adds too much unnecessary space e.g. if you do only box or strike notations. For <menclose notation="radical"> that should not be necessary, so it could render the same as <msqrt> if menclose adds not padding/margin.

EFL EWS Bot

Comment 76 2014-01-09 02:15:39 PST

Comment on attachment 220703 [details] Patch Attachment 220703 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/5324357945524224

Build Bot

Comment 77 2014-01-09 02:17:31 PST

Comment on attachment 220703 [details] Patch Attachment 220703 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5621356410437632

Build Bot

Comment 78 2014-01-09 02:39:14 PST

Comment on attachment 220703 [details] Patch Attachment 220703 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5058178957967360

gur.trio

Comment 79 2014-01-09 03:06:27 PST

(In reply to comment #75) > (In reply to comment #74) > > Actually the difference is because for menclose case we set the height and width to sqrt(2) times its child content which is more as compared to msqrt case. So dont think that will be similar in any case. > > This should be done *only* when the circle notation is used. Otherwise this adds too much unnecessary space e.g. if you do only box or strike notations. For <menclose notation="radical"> that should not be necessary, so it could render the same as <msqrt> if menclose adds not padding/margin. Ahh Ok. Right now I have done for all. Also the spec suggests "The values "box", "roundedbox", and "circle" should enclose the contents as indicated by the values. The amount of distance between the box, roundedbox, or circle, and the contents are not specified by MathML, and is left to the renderer. In practice, paddings on each side of 0.4em in the horizontal direction and .5ex in the vertical direction seem to work well." so in mathml.css I have added padding which is for all.

Frédéric Wang (:fredw)

Comment 80 2014-01-09 03:11:08 PST

> (In reply to comment #75) > Ahh Ok. Right now I have done for all. Also the spec suggests > "The values "box", "roundedbox", and "circle" should enclose the contents as indicated by the values. The amount of distance between the box, roundedbox, or circle, and the contents are not specified by MathML, and is left to the renderer. In practice, paddings on each side of 0.4em in the horizontal direction and .5ex in the vertical direction seem to work well." > so in mathml.css I have added padding which is for all. Well, I think this is a suggestion from Neil Soiffer as implemented in MathPlayer but not a normative a requirement (and does not work with circle BTW). Gecko adds this kind of padding (with different values) only for borders.

Frédéric Wang (:fredw)

Comment 81 2014-01-09 03:17:25 PST

Created attachment 220708 [details] diagram and equation explaining where \sqrt{2} comes from FYI.

gur.trio

Comment 82 2014-01-09 04:09:43 PST

(In reply to comment #81) > Created an attachment (id=220708) [details] > diagram and equation explaining where \sqrt{2} comes from > > FYI. Thanks Frederic for this. For menclose padding I am keeping the value as mentioned in the spec else the box, roundedbox are very close to the child content. Please suggest?

Frédéric Wang (:fredw)

Comment 83 2014-01-09 04:15:59 PST

(In reply to comment #82) > (In reply to comment #81) > For menclose padding I am keeping the value as mentioned in the spec else the box, roundedbox are very close to the child content. Please suggest? Yes, you need padding for these and other border notations. Ideally, you could do as in Gecko and add padding only for notations that need it but I don't think it's easy to do that if you rely only on mathml.css.

gur.trio

Comment 84 2014-01-09 05:53:25 PST

Created attachment 220718 [details] Patch

EFL EWS Bot

Comment 85 2014-01-09 06:01:58 PST

Comment on attachment 220718 [details] Patch Attachment 220718 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/6644967241940992

Build Bot

Comment 86 2014-01-09 06:47:32 PST

Comment on attachment 220718 [details] Patch Attachment 220718 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4523012342677504 New failing tests: mathml/presentation/menclose-notation-radical.html mathml/presentation/inferred-mrow-baseline.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 87 2014-01-09 06:47:40 PST

Created attachment 220724 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Build Bot

Comment 88 2014-01-09 07:09:16 PST

Comment on attachment 220718 [details] Patch Attachment 220718 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4986078134009856 New failing tests: mathml/presentation/menclose-notation-radical.html mathml/presentation/inferred-mrow-baseline.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 89 2014-01-09 07:09:25 PST

Created attachment 220729 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Build Bot

Comment 90 2014-01-09 08:10:24 PST

Comment on attachment 220718 [details] Patch Attachment 220718 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4600526436040704 New failing tests: mathml/presentation/menclose-notation-radical.html mathml/presentation/inferred-mrow-baseline.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 91 2014-01-09 08:10:33 PST

Created attachment 220736 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5

gur.trio

Comment 92 2014-01-10 03:43:46 PST

Created attachment 220830 [details] Patch

Build Bot

Comment 93 2014-01-10 04:43:17 PST

Comment on attachment 220830 [details] Patch Attachment 220830 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4683762499584000 New failing tests: mathml/presentation/inferred-mrow-baseline.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 94 2014-01-10 04:43:23 PST

Created attachment 220833 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Build Bot

Comment 95 2014-01-10 05:01:42 PST

Comment on attachment 220830 [details] Patch Attachment 220830 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5255957638545408 New failing tests: mathml/presentation/inferred-mrow-baseline.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 96 2014-01-10 05:01:51 PST

Created attachment 220834 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Build Bot

Comment 97 2014-01-10 06:01:31 PST

Comment on attachment 220830 [details] Patch Attachment 220830 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4823773601595392 New failing tests: mathml/presentation/inferred-mrow-baseline.html svg/custom/conditional-processing-2.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 98 2014-01-10 06:01:40 PST

Created attachment 220836 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5

gur.trio

Comment 99 2014-01-10 06:24:05 PST

I am working on the failed test cases.One of the test cases the mo operator is not stretching. And other the mspace is not aligned properly.

Frédéric Wang (:fredw)

Comment 100 2014-01-10 06:46:33 PST

(In reply to comment #99) > I am working on the failed test cases.One of the test cases the mo operator is not stretching. And other the mspace is not aligned properly. See https://bugs.webkit.org/show_bug.cgi?id=124841. menclose should behave like an mrow.

gur.trio

Comment 101 2014-01-13 05:40:14 PST

Created attachment 221031 [details] Patch

Build Bot

Comment 102 2014-01-13 06:35:14 PST

Comment on attachment 221031 [details] Patch Attachment 221031 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5419429596758016 New failing tests: mathml/presentation/inferred-mrow-baseline.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 103 2014-01-13 06:35:21 PST

Created attachment 221035 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Build Bot

Comment 104 2014-01-13 06:57:10 PST

Comment on attachment 221031 [details] Patch Attachment 221031 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4825429579923456 New failing tests: mathml/presentation/inferred-mrow-baseline.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 105 2014-01-13 06:57:19 PST

Created attachment 221037 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5

gur.trio

Comment 106 2014-01-13 07:56:27 PST

Created attachment 221043 [details] Patch

Build Bot

Comment 107 2014-01-13 08:32:18 PST

Comment on attachment 221043 [details] Patch Attachment 221043 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5875198775525376 New failing tests: mathml/presentation/inferred-mrow-baseline.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 108 2014-01-13 08:32:25 PST

Created attachment 221046 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Build Bot

Comment 109 2014-01-13 08:50:30 PST

Comment on attachment 221043 [details] Patch Attachment 221043 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6283281737711616 New failing tests: mathml/presentation/inferred-mrow-baseline.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 110 2014-01-13 08:50:41 PST

Created attachment 221048 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

gur.trio

Comment 111 2014-01-13 22:51:19 PST

Created attachment 221107 [details] Patch

Build Bot

Comment 112 2014-01-13 23:46:35 PST

Comment on attachment 221107 [details] Patch Attachment 221107 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6118968636997632 New failing tests: mathml/presentation/inferred-mrow-baseline.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 113 2014-01-13 23:46:42 PST

Created attachment 221108 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Build Bot

Comment 114 2014-01-14 00:06:12 PST

Comment on attachment 221107 [details] Patch Attachment 221107 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6650917516476416 New failing tests: mathml/presentation/inferred-mrow-baseline.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 115 2014-01-14 00:06:21 PST

Created attachment 221113 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5

gur.trio

Comment 116 2014-01-14 00:26:59 PST

Created attachment 221115 [details] Patch

Build Bot

Comment 117 2014-01-14 01:28:31 PST

Comment on attachment 221115 [details] Patch Attachment 221115 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5427322270253056 New failing tests: mathml/presentation/inferred-mrow-baseline.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 118 2014-01-14 01:28:39 PST

Created attachment 221121 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Build Bot

Comment 119 2014-01-14 01:45:12 PST

Comment on attachment 221115 [details] Patch Attachment 221115 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4517060725964800 New failing tests: mathml/presentation/inferred-mrow-baseline.html mathml/presentation/inferred-mrow-stretchy.html

Build Bot

Comment 120 2014-01-14 01:45:22 PST

Created attachment 221125 [details] Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5

gur.trio

Comment 121 2014-01-14 01:49:58 PST

Created attachment 221127 [details] Patch

gur.trio

Comment 122 2014-01-14 04:38:06 PST

(In reply to comment #121) > Created an attachment (id=221127) [details] > Patch Finally the build is done. :) Please review.

Frédéric Wang (:fredw)

Comment 123 2014-01-15 09:16:21 PST

Comment on attachment 221127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221127&action=review Could you please add tests for dynamic changes: adding/removing children, adding/removing/changing attribute values? > Source/WebCore/mathml/MathMLElement.h:47 > + return hasTagName(MathMLNames::miTag) || hasTagName(MathMLNames::mnTag) || hasTagName(MathMLNames::moTag) || hasTagName(MathMLNames::msTag) || hasTagName(MathMLNames::mtextTag) || hasTagName(MathMLNames::mencloseTag); Why do you need that? menclose is not a MathML token. > Source/WebCore/mathml/MathMLInlineContainerElement.cpp:89 > + return createRenderer<RenderMathMLMenclose>(*this, std::move(style)); Is this necessary. I believe the menclose renderer is created in MathMLMencloseElement::createElementRenderer... > Source/WebCore/mathml/MathMLMencloseElement.cpp:2 > + * Copyright (C) 2014 The MathJax Consortium. All rights reserved. The copyright should contain your name, I think. > Source/WebCore/mathml/MathMLMencloseElement.cpp:68 > + // when notation value is radical and menclose doesnot have any child then s/doesnot/does not/ > Source/WebCore/mathml/MathMLMencloseElement.h:2 > + * Copyright (C) 2014 The MathJax Consortium. All rights reserved. Same here about Copyright. > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:2 > + * Copyright (C) 2014 The MathJax Consortium. All rights reserved. ditto. > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:2 > + * Copyright (C) 2014 The MathJax Consortium. All rights reserved. ditto.

gur.trio

Comment 124 2014-01-16 01:06:47 PST

Created attachment 221345 [details] Patch

gur.trio

Comment 125 2014-01-16 01:07:57 PST

> > Source/WebCore/mathml/MathMLElement.h:47 > > + return hasTagName(MathMLNames::miTag) || hasTagName(MathMLNames::mnTag) || hasTagName(MathMLNames::moTag) || hasTagName(MathMLNames::msTag) || hasTagName(MathMLNames::mtextTag) || hasTagName(MathMLNames::mencloseTag); > > Why do you need that? menclose is not a MathML token. > In MathMLMencloseElement.h its being used in toMathMLMencloseElement. I check if it is menclose and then static_cast.

gur.trio

Comment 126 2014-01-16 01:11:44 PST

(In reply to comment #124) > Created an attachment (id=221345) [details] > Patch Thanks Frederic for the review. Chris and Frederic : There are some known issues of menclose like if no notation attribute then longdiv should be displayed. Its not handled yet. So if basic implementation is fine then can we go ahead with this and for the known issues handle those in different bugs? Just wanted to know your thoughts?

Frédéric Wang (:fredw)

Comment 127 2014-01-16 01:22:55 PST

(In reply to comment #125) > In MathMLMencloseElement.h its being used in toMathMLMencloseElement. I check if it is menclose and then static_cast. I'm not sure what is the best way to cast menclose but: - you definitely don't want to cast a token element like mtext, mn etc to and menclose element. - you don't want to make isTokenElement return true for an menclose element. http://www.w3.org/TR/MathML/chapter3.html#id.3.1.9.1

gur.trio

Comment 128 2014-01-16 01:33:52 PST

(In reply to comment #127) > (In reply to comment #125) > > In MathMLMencloseElement.h its being used in toMathMLMencloseElement. I check if it is menclose and then static_cast. > > I'm not sure what is the best way to cast menclose but: > > - you definitely don't want to cast a token element like mtext, mn etc to and menclose element. > > - you don't want to make isTokenElement return true for an menclose element. > > http://www.w3.org/TR/MathML/chapter3.html#id.3.1.9.1 Actually earlier I had written isMathMLMencloseElement function which was overring the function is Element.h (previous patches) but then removed as per Chris's suggestion. (we probably don't want to put this in Element. The other MathML elements are referenced like this in MathMLElement.h)

gur.trio

Comment 129 2014-01-16 03:31:42 PST

(In reply to comment #128) > (In reply to comment #127) > > (In reply to comment #125) > > > In MathMLMencloseElement.h its being used in toMathMLMencloseElement. I check if it is menclose and then static_cast. > > > > I'm not sure what is the best way to cast menclose but: > > > > - you definitely don't want to cast a token element like mtext, mn etc to and menclose element. > > > > - you don't want to make isTokenElement return true for an menclose element. > > > > http://www.w3.org/TR/MathML/chapter3.html#id.3.1.9.1 > > Actually earlier I had written isMathMLMencloseElement function which was overring the function is Element.h (previous patches) but then removed as per Chris's suggestion. (we probably don't want to put this in Element. The other MathML elements are referenced like this in MathMLElement.h) Else can add another API to check for menclose in MathMLElement.h. Your thoughts?

gur.trio

Comment 130 2014-01-17 05:15:47 PST

(In reply to comment #124) > Created an attachment (id=221345) [details] > Patch Hi Chris. Can you please have a look? Thanks

gur.trio

Comment 131 2014-01-19 23:14:48 PST

Created attachment 221618 [details] Patch

gur.trio

Comment 132 2014-01-20 20:03:19 PST

Created attachment 221716 [details] Patch

gur.trio

Comment 133 2014-01-21 03:16:22 PST

(In reply to comment #132) > Created an attachment (id=221716) [details] > Patch Patch is not being applied. All changes have been done as per the suggestions. Chris and Frederic can you please review. Thanks

Frédéric Wang (:fredw)

Comment 134 2014-01-21 11:34:33 PST

Comment on attachment 221716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221716&action=review > LayoutTests/fast/css/resize-single-axis.html:73 > + //} Why these changes? > LayoutTests/mathml/presentation/menclose-notation-attribute-remove-expected.html:5 > + <math><mspace width="100px" height="50px" mathbackground="red"/></math> You should have an menclose element here, otherwise the test will fail when you make "longdiv" be the default. > LayoutTests/mathml/presentation/menclose-notation-attribute-set1.html:42 > + isSuccessfullyParsed(); I think you could do more testing e.g. check the borders that are absent or the style of corner of roundedbox > LayoutTests/mathml/presentation/menclose-notation-no-overlap.html:7 > + description('Tests that menclose with notation valus as circle is sqrt(2) times the inner width/height.'); s/valus/value/

gur.trio

Comment 135 2014-01-22 02:07:47 PST

Created attachment 221840 [details] Patch

chris fleizach

Comment 136 2014-01-22 09:32:56 PST

Comment on attachment 221840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221840&action=review Thanks for your hard work on this. It's getting close. Maybe you need to update your local sources, which is why the patch doesn't apply > Source/WebCore/ChangeLog:66 > + its values liek top, left CSSBorder properties are applied and for liek misspelled > Source/WebCore/ChangeLog:68 > + For radical value an anonymous RenderMathMLSquareRoot is created as as "a" child of menclose > Source/WebCore/mathml/MathMLMencloseElement.cpp:68 > + // when notation value is radical and menclose does not have any child then when should be capitalized the sentence should say "When notation value is "a" radical and ..." > Source/WebCore/mathml/MathMLMencloseElement.cpp:79 > + if (!val.isEmpty()) { i think this line should be an early return if (val.isEmpty()) return > Source/WebCore/mathml/MathMLMencloseElement.cpp:81 > + size_t attrvaluesSize = m_notationValues.size(); this variable should be named notationValueSize > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:49 > + // For radical notation creating an anonymous RenderMathMLSquareRoot. This comment should probably be worded like "Allow an anonymous RenderMathMLSquareRoot to handle drawing the radical notation, rather than duplicating the code needed to paint a root." > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:70 > + const Vector<String>& notationVal = menclose->notationValues(); I would call this variable notationValues > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:74 > + m_minPreferredLogicalWidth = minPreferredLogicalWidth() * sqrt(float(2)); instead of calling float(2), can you just put a 2.0 > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:85 > + const Vector<String>& notationVal = menclose->notationValues(); ditto about variable name > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:89 > + setLogicalHeight(logicalHeight() * sqrt(float(2))); ditto about float > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:100 > + const Vector<String>& notationVal = menclose->notationValues(); ditto about name > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:117 > + if (notationVal[i] == "updiagonalstrike") are we missing updiagnalarrow? https://developer.mozilla.org/en-US/docs/Web/MathML/Element/menclose > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:137 > + midxPoint= childLeft-left; need spaces around - midxPoint = childLeft - left > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:42 > + virtual void addChild(RenderObject* newChild, RenderObject* beforeChild = 0) override; beforeChild = nullptr

Darin Adler

Comment 137 2014-01-22 10:41:09 PST

Comment on attachment 221840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221840&action=review >> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:74 >> + m_minPreferredLogicalWidth = minPreferredLogicalWidth() * sqrt(float(2)); > > instead of calling float(2), can you just put a 2.0 sqrt(2) is also fine, but I think that float(M_SQRT2) would be even better since it wouldn’t involve a function call

gur.trio

Comment 138 2014-01-22 22:23:54 PST

Created attachment 221948 [details] Patch

gur.trio

Comment 139 2014-01-22 23:00:27 PST

(In reply to comment #136) > (From update of attachment 221840 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221840&action=review > > Thanks for your hard work on this. It's getting close. Maybe you need to update your local sources, which is why the patch doesn't apply > > > Source/WebCore/ChangeLog:66 > > + its values liek top, left CSSBorder properties are applied and for > > liek misspelled > > > Source/WebCore/ChangeLog:68 > > + For radical value an anonymous RenderMathMLSquareRoot is created as > > as "a" child of menclose > > > Source/WebCore/mathml/MathMLMencloseElement.cpp:68 > > + // when notation value is radical and menclose does not have any child then > > when should be capitalized > > the sentence should say "When notation value is "a" radical and ..." > > > Source/WebCore/mathml/MathMLMencloseElement.cpp:79 > > + if (!val.isEmpty()) { > > i think this line should be an early return > if (val.isEmpty()) > return > > > Source/WebCore/mathml/MathMLMencloseElement.cpp:81 > > + size_t attrvaluesSize = m_notationValues.size(); > > this variable should be named > notationValueSize > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:49 > > + // For radical notation creating an anonymous RenderMathMLSquareRoot. > > This comment should probably be worded like > > "Allow an anonymous RenderMathMLSquareRoot to handle drawing the radical notation, rather than duplicating the code needed to paint a root." > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:70 > > + const Vector<String>& notationVal = menclose->notationValues(); > > I would call this variable notationValues > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:74 > > + m_minPreferredLogicalWidth = minPreferredLogicalWidth() * sqrt(float(2)); > > instead of calling float(2), can you just put a 2.0 > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:85 > > + const Vector<String>& notationVal = menclose->notationValues(); > > ditto about variable name > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:89 > > + setLogicalHeight(logicalHeight() * sqrt(float(2))); > > ditto about float > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:100 > > + const Vector<String>& notationVal = menclose->notationValues(); > > ditto about name > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:117 > > + if (notationVal[i] == "updiagonalstrike") > > are we missing updiagnalarrow? > https://developer.mozilla.org/en-US/docs/Web/MathML/Element/menclose > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:137 > > + midxPoint= childLeft-left; > > need spaces around - > midxPoint = childLeft - left > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:42 > > + virtual void addChild(RenderObject* newChild, RenderObject* beforeChild = 0) override; > > beforeChild = nullptr Thanks Chris, Frederic and Darin for the review. So patiently you all helped me with this work.

gur.trio

Comment 140 2014-01-22 23:18:38 PST

(In reply to comment #136) > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:117 > > + if (notationVal[i] == "updiagonalstrike") > > are we missing updiagnalarrow? > https://developer.mozilla.org/en-US/docs/Web/MathML/Element/menclose > This seems to be newly added. Even mozilla right now doesnot support it. So I guess we can take it up later. updiagnalarrow needs some more work.

chris fleizach

Comment 141 2014-01-22 23:24:09 PST

(In reply to comment #140) > (In reply to comment #136) > > > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:117 > > > + if (notationVal[i] == "updiagonalstrike") > > > > are we missing updiagnalarrow? > > https://developer.mozilla.org/en-US/docs/Web/MathML/Element/menclose > > > This seems to be newly added. Even mozilla right now doesnot support it. So I guess we can take it up later. updiagnalarrow needs some more work. Can you adad a TODO in the code and file a bug? Thanks

Build Bot

Comment 142 2014-01-22 23:42:11 PST

Comment on attachment 221948 [details] Patch Attachment 221948 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6385808982409216 New failing tests: mathml/presentation/menclose-notation-no-overlap.html

Build Bot

Comment 143 2014-01-22 23:42:18 PST

Created attachment 221951 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Build Bot

Comment 144 2014-01-23 00:18:35 PST

Comment on attachment 221948 [details] Patch Attachment 221948 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4582663599947776 New failing tests: mathml/presentation/menclose-notation-no-overlap.html

Build Bot

Comment 145 2014-01-23 00:18:46 PST

Created attachment 221954 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

gur.trio

Comment 146 2014-01-23 00:19:51 PST

Created attachment 221955 [details] Patch

gur.trio

Comment 147 2014-01-23 00:20:57 PST

(In reply to comment #141) > (In reply to comment #140) > > (In reply to comment #136) > > > > > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:117 > > > > + if (notationVal[i] == "updiagonalstrike") > > > > > > are we missing updiagnalarrow? > > > https://developer.mozilla.org/en-US/docs/Web/MathML/Element/menclose > > > > > This seems to be newly added. Even mozilla right now doesnot support it. So I guess we can take it up later. updiagnalarrow needs some more work. > > Can you adad a TODO in the code and file a bug? Thanks Done.

chris fleizach

Comment 148 2014-01-24 13:03:22 PST

Comment on attachment 221955 [details] Patch r=me

WebKit Commit Bot

Comment 149 2014-01-27 21:45:58 PST

Comment on attachment 221955 [details] Patch Rejecting attachment 221955 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 221955, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: n-no-overlap.html patching file LayoutTests/mathml/presentation/menclose-notation-radical-expected.html patching file LayoutTests/mathml/presentation/menclose-notation-radical.html patching file LayoutTests/mathml/presentation/menclose-remove-children-expected.html patching file LayoutTests/mathml/presentation/menclose-remove-children.html Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Chris Fleizach']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/4921392906633216

gur.trio

Comment 150 2014-01-27 23:35:01 PST

Created attachment 222409 [details] Patch

gur.trio

Comment 151 2014-01-28 01:00:09 PST

(In reply to comment #150) > Created an attachment (id=222409) [details] > Patch Hi Chris. Could you please set the r+ and cq+. Its the same patch which you reviewed earlier but since patch was a little old cq+ failed :(

WebKit Commit Bot

Comment 152 2014-01-28 08:30:18 PST

Comment on attachment 222409 [details] Patch Clearing flags on attachment: 222409 Committed r162933: <http://trac.webkit.org/changeset/162933>

WebKit Commit Bot

Comment 153 2014-01-28 08:30:31 PST

All reviewed patches have been landed. Closing bug.

Frédéric Wang (:fredw)

Comment 154 2014-03-10 12:25:38 PDT

Mass change: add WebExposed keyword to help MDN documentation.

Note You need to log in before you can comment on or make changes to this bug.