Bug 127889

Summary: Menclose with no notation attribute does not display anything.
Product: WebKit Reporter: gur.trio
Component: MathMLAssignee: gur.trio
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, bunhere, cdumez, cfleizach, commit-queue, dbarton, esprehn+autocc, fred.wang, glenn, gyuyoung.kim, kondapallykalyan, mrobinson, rniwa, sergio
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 129366    
Attachments:
Description Flags
Menclose with no notation attribute.
none
Patch
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-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
Patch
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Patch
none
Patch none

Description gur.trio 2014-01-29 23:22:26 PST
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.
Comment 1 gur.trio 2014-01-30 00:05:05 PST
Created attachment 222634 [details]
Patch
Comment 2 gur.trio 2014-01-30 00:21:18 PST
Created attachment 222635 [details]
Patch
Comment 3 Build Bot 2014-01-30 02:36:49 PST
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
Comment 4 Build Bot 2014-01-30 02:36:50 PST
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
Comment 5 Build Bot 2014-01-30 02:37:50 PST
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
Comment 6 Build Bot 2014-01-30 02:37:53 PST
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
Comment 7 Build Bot 2014-01-30 02:38:39 PST
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
Comment 8 Build Bot 2014-01-30 02:38:42 PST
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
Comment 9 Build Bot 2014-01-30 03:37:33 PST
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
Comment 10 Build Bot 2014-01-30 03:37:35 PST
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
Comment 11 gur.trio 2014-01-30 04:58:57 PST
Hi Can someone help me analyze the failed mathml test case incase of mac. For windows its working fine for me.
Comment 12 Frédéric Wang (:fredw) 2014-01-30 05:03:06 PST
(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.
Comment 13 gur.trio 2014-01-30 07:50:12 PST
(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.
Comment 14 Frédéric Wang (:fredw) 2014-01-30 07:53:13 PST
> 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...)
Comment 15 gur.trio 2014-01-30 22:13:35 PST
(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
Comment 16 chris fleizach 2014-01-31 09:18:13 PST
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)
Comment 17 gur.trio 2014-02-02 23:46:59 PST
Created attachment 222963 [details]
Patch
Comment 18 Build Bot 2014-02-03 01:03:52 PST
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
Comment 19 Build Bot 2014-02-03 01:03:59 PST
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
Comment 20 Build Bot 2014-02-03 02:05:08 PST
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
Comment 21 Build Bot 2014-02-03 02:05:12 PST
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
Comment 22 Build Bot 2014-02-03 02:36:36 PST
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
Comment 23 Build Bot 2014-02-03 02:36:39 PST
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
Comment 24 chris fleizach 2014-02-03 08:53:23 PST
(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
Comment 25 gur.trio 2014-02-03 08:57:09 PST
(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.
Comment 26 gur.trio 2014-02-04 00:33:28 PST
Created attachment 223080 [details]
Patch
Comment 27 gur.trio 2014-02-05 05:59:28 PST
(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.
Comment 28 Frédéric Wang (:fredw) 2014-02-05 06:20:41 PST
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
Comment 29 chris fleizach 2014-02-05 08:48:19 PST
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
Comment 30 gur.trio 2014-02-05 20:47:45 PST
Created attachment 223300 [details]
Patch
Comment 31 WebKit Commit Bot 2014-02-06 09:59:49 PST
Comment on attachment 223300 [details]
Patch

Clearing flags on attachment: 223300

Committed r163543: <http://trac.webkit.org/changeset/163543>
Comment 32 WebKit Commit Bot 2014-02-06 09:59:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 33 Frédéric Wang (:fredw) 2014-03-10 12:26:24 PDT
Mass change: add WebExposed keyword to help MDN documentation.