Bug 127889 – Menclose with no notation attribute does not display anything.

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

Created attachment 222630 [details]
Menclose with no notation attribute.

Menclose with no notation attribute should display same as notation="longdiv". Also when notation does not have a valid value or is empty nothing should be displayed.

Created attachment 222634 [details]
Patch

Created attachment 222635 [details]
Patch

[Build Bot]

Comment on attachment 222635 [details]
Patch

Attachment 222635 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4900508359720960

New failing tests:
mathml/presentation/menclose-notation-default-longdiv.html

[Build Bot]

Created attachment 222650 [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 on attachment 222635 [details]
Patch

Attachment 222635 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6228614521552896

New failing tests:
mathml/presentation/menclose-notation-default-longdiv.html

[Build Bot]

Created attachment 222651 [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 on attachment 222635 [details]
Patch

Attachment 222635 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6327039669829632

New failing tests:
mathml/presentation/menclose-notation-default-longdiv.html

[Build Bot]

Created attachment 222652 [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

[Build Bot]

Comment on attachment 222635 [details]
Patch

Attachment 222635 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6100569735299072

New failing tests:
mathml/presentation/menclose-notation-default-longdiv.html

[Build Bot]

Created attachment 222657 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Hi Can someone help me analyze the failed mathml test case incase of mac. For windows its working fine for me.

[Frédéric Wang (:fredw)]

(In reply to comment #11)
> Hi Can someone help me analyze the failed mathml test case incase of mac. For windows its working fine for me.

I just took a quick look at the test and it seems correct to me.

(In reply to comment #12)
> (In reply to comment #11)
> > Hi Can someone help me analyze the failed mathml test case incase of mac. For windows its working fine for me.
> 
> I just took a quick look at the test and it seems correct to me.

Ya but it failed :-( The expected png and actual have difference in the top padding.

[Frédéric Wang (:fredw)]

> Ya but it failed :-( The expected png and actual have difference in the top padding.

I have not checked the code, but I wonder if you are having the same problem as me for bug 124838... Where the style is not applied correctly (but the tests only fail on Mac...)

(In reply to comment #14)
> > Ya but it failed :-( The expected png and actual have difference in the top padding.
> 
> I have not checked the code, but I wonder if you are having the same problem as me for bug 124838... Where the style is not applied correctly (but the tests only fail on Mac...)

Hi Frederic. Can you review the patch please? Thanks

[chris fleizach]

Comment on attachment 222635 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222635&action=review

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:80
> +    if (!menclose->hasAttribute(notationAttr)) {

I think we should have a method on MathMLMencloseElement for something like isDefaultLongDiv() instead of checking hasAttribute() for each of the following
that will make it clearer why !notationAttr = default long div

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:148
> +            if (!menclose->hasAttribute(notationAttr))

this should be if (isDefaultLongDiv)

Created attachment 222963 [details]
Patch

[Build Bot]

Comment on attachment 222963 [details]
Patch

Attachment 222963 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6209255493337088

New failing tests:
mathml/presentation/menclose-notation-default-longdiv.html

[Build Bot]

Created attachment 222968 [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 on attachment 222963 [details]
Patch

Attachment 222963 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5895950513995776

New failing tests:
mathml/presentation/menclose-notation-default-longdiv.html

[Build Bot]

Created attachment 222972 [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

[Build Bot]

Comment on attachment 222963 [details]
Patch

Attachment 222963 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5548848437002240

New failing tests:
mathml/presentation/menclose-notation-default-longdiv.html

[Build Bot]

Created attachment 222974 [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

[chris fleizach]

(In reply to comment #23)
> Created an attachment (id=222974) [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

Looks like there's a very small difference on Mac while drawing. Might be better to turn this into a different kind of test, or put in mac platform-specific results

(In reply to comment #24)
> (In reply to comment #23)
> > Created an attachment (id=222974) [details] [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
> 
> Looks like there's a very small difference on Mac while drawing. Might be better to turn this into a different kind of test, or put in mac platform-specific results

Ya there is a small difference in the top padding.

Created attachment 223080 [details]
Patch

(In reply to comment #26)
> Created an attachment (id=223080) [details]
> Patch

Hi Chris and Frederic please have a look. Test cases are passing now. Thanks.

[Frédéric Wang (:fredw)]

Comment on attachment 223080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223080&action=review

That looks good to me.

> Source/WebCore/ChangeLog:3
> +        Menclose with no notation attribute doesnot display anything.

does not

[chris fleizach]

Comment on attachment 223080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223080&action=review

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:68
> +    const float paddingTop = 6.f;

.f shouldn't be necessary here.
From style guidelines

"Unless required in order to force floating point math, do not append .0, .f and .0f to floating point literals."

> LayoutTests/mathml/presentation/menclose-notation-invalid-empty-expected.html:10
> +			        <mtr><mtd><mspace width="100px" height="50px" mathbackground="red"/></mtd></mtr>

looks like there are tabs in this file

> LayoutTests/mathml/presentation/menclose-notation-invalid-empty.html:10
> +			        <mtr><mtd><menclose notation=""><mspace width="100px" height="50px" mathbackground="red"/></menclose></mtd></mtr>

ditto about tabs

Created attachment 223300 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 223300 [details]
Patch

Clearing flags on attachment: 223300

Committed r163543: <http://trac.webkit.org/changeset/163543>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Frédéric Wang (:fredw)]

Mass change: add WebExposed keyword to help MDN documentation.