"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
Created attachment 171152 [details] testcase
As Frederic notes, several common mathematical notations rely (for their MathML implementations) on menclose.
(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?
(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.
(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
(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.
(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.
Created attachment 212887 [details] Patch
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
Comment on attachment 212887 [details] Patch Attachment 212887 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2613444
Created attachment 212896 [details] Patch
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
Comment on attachment 212896 [details] Patch Attachment 212896 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2692288
Created attachment 212897 [details] Patch
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
Created attachment 212899 [details] Patch
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
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
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
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
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.
Created attachment 212996 [details] Patch
(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.
Comment on attachment 212996 [details] Patch Attachment 212996 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/2774067
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
Created attachment 212998 [details] Patch
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
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
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
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
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
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
(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?
(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
(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
(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.
(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>.
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
(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?
(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.
(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 ?
(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.
Created attachment 220506 [details] Patch
Comment on attachment 220506 [details] Patch Attachment 220506 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/5469383690289152
Comment on attachment 220506 [details] Patch Attachment 220506 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/4658955305353216
Created attachment 220507 [details] Patch
Comment on attachment 220507 [details] Patch Attachment 220507 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/5386102932242432
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
Created attachment 220508 [details] Patch
(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.
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...
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> ?
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
Created attachment 220597 [details] Patch
Created attachment 220602 [details] Patch
(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?
(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.
(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 :)
Created attachment 220622 [details] Patch
Created attachment 220635 [details] Patch
Comment on attachment 220635 [details] Patch Attachment 220635 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/6371233004257280
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
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
Comment on attachment 220635 [details] Patch Attachment 220635 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5921713137123328
(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.
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
(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...
(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.
Created attachment 220703 [details] Patch
(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?
Comment on attachment 220703 [details] Patch Attachment 220703 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/6072233084583936
Comment on attachment 220703 [details] Patch Attachment 220703 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/5445130681057280
(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.
(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.
(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.
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
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
Comment on attachment 220703 [details] Patch Attachment 220703 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5058178957967360
(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.
> (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.
Created attachment 220708 [details] diagram and equation explaining where \sqrt{2} comes from FYI.
(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?
(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.
Created attachment 220718 [details] Patch
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
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
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
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
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
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
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
Created attachment 220830 [details] Patch
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
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
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
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
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
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
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.
(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.
Created attachment 221031 [details] Patch
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
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
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
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
Created attachment 221043 [details] Patch
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
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
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
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
Created attachment 221107 [details] Patch
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
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
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
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
Created attachment 221115 [details] Patch
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
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
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
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
Created attachment 221127 [details] Patch
(In reply to comment #121) > Created an attachment (id=221127) [details] > Patch Finally the build is done. :) Please review.
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.
Created attachment 221345 [details] Patch
> > 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.
(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?
(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
(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)
(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?
(In reply to comment #124) > Created an attachment (id=221345) [details] > Patch Hi Chris. Can you please have a look? Thanks
Created attachment 221618 [details] Patch
Created attachment 221716 [details] Patch
(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
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/
Created attachment 221840 [details] Patch
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
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
Created attachment 221948 [details] Patch
(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.
(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.
(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
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
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
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
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
Created attachment 221955 [details] Patch
(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.
Comment on attachment 221955 [details] Patch r=me
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
Created attachment 222409 [details] Patch
(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 :(
Comment on attachment 222409 [details] Patch Clearing flags on attachment: 222409 Committed r162933: <http://trac.webkit.org/changeset/162933>
All reviewed patches have been landed. Closing bug.
Mass change: add WebExposed keyword to help MDN documentation.