Bug 75898

Summary: table tests for CSS3 calc
Product: WebKit Reporter: Mike Lawther <mikelawther>
Component: New BugsAssignee: Mike Lawther <mikelawther>
Status: RESOLVED FIXED    
Severity: Normal CC: jchaffraix, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 16662    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Mike Lawther 2012-01-09 15:42:44 PST
table tests for CSS3 calc
Comment 1 Mike Lawther 2012-01-09 15:44:17 PST
Created attachment 121736 [details]
Patch
Comment 2 Julien Chaffraix 2012-01-26 11:56:12 PST
Comment on attachment 121736 [details]
Patch

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

> LayoutTests/css3/calc/table-calcs-expected.txt:9
> +

I am really not a huge fan of the output. A single FAIL / PASS per line would be more readable.

Here are some ways to achieve that:
* use several rows with one cell per row.
* remove your tables from your final output and dump into #result.
* add visibility: hidden on your tables and dump into #result.

> LayoutTests/css3/calc/table-calcs.html:4
> +    padding: 0;

Nit: Add the units.

> LayoutTests/css3/calc/table-calcs.html:7
> +    background-color: red;

red usually should be reserved for failure. Here you expect to see the cells so another color would be welcome.

> LayoutTests/css3/calc/table-calcs.html:71
> +    <div style="height: 416px">
> +      <table style="height: 25%">
> +          <tr><td class="height-percent-test">control height:25%</td></tr>
> +      </table>
> +    </div>
> +    <div style="height: 416px">
> +      <table style="height: -webkit-calc(25%);">
> +          <tr><td class="height-percent-test">simple 25%</td></tr>
> +      </table>
> +    </div>
> +    <div style="height: 416px">
> +      <table style="height: -webkit-calc(10% * 2 + 5%));">
> +          <tr><td class="height-percent-test">10% * 2 + 5%</td></tr>
> +      </table>

If you set border-spacing: 0px on your table (I would argue on all tables in your style), you would set your div's height to 400px and get the right value. This would be a lot more readable!

> LayoutTests/css3/calc/table-empty-cells-expected-mismatch.html:1
> +<style> 

The name does not mention quirks mode. Also I don't understand what it is doing in calc/ (no -webkit-calc in sight) and why we expect mismatch here?

> LayoutTests/css3/calc/table-empty-cells.html:1
> +<style> 

Is quirks mode needed?

> LayoutTests/css3/calc/table-empty-cells.html:8
> +    .bgcolor { background-color: yellow } 

Not sure that it's a useful style.

> LayoutTests/css3/calc/table-empty-cells.html:16
> +        <td class="cell1"> </td>

There no cell1 class (not repeated in both files).

> LayoutTests/css3/calc/table-empty-cells.html:40
> +    <table cellpadding="0" cellspacing="0" width="100%">

cellpadding and cellspacing are obsolete, please use border-spacing instead.

Also factor out this style into your stylesheet and override it if needed locally:

table {
    border-spacing: 0px;
    width: 100%;
}
Comment 3 Mike Lawther 2012-03-01 22:03:15 PST
Comment on attachment 121736 [details]
Patch

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

Thanks for your review Julien! I've updated these tests, and in the meantime as I've been adding actual calc functionality, these are passing. So the expectations have been updated as well.

>> LayoutTests/css3/calc/table-calcs-expected.txt:9
>> +
> 
> I am really not a huge fan of the output. A single FAIL / PASS per line would be more readable.
> 
> Here are some ways to achieve that:
> * use several rows with one cell per row.
> * remove your tables from your final output and dump into #result.
> * add visibility: hidden on your tables and dump into #result.

Done. I've used pre/post js for this.

>> LayoutTests/css3/calc/table-calcs.html:4
>> +    padding: 0;
> 
> Nit: Add the units.

Done.

>> LayoutTests/css3/calc/table-calcs.html:7
>> +    background-color: red;
> 
> red usually should be reserved for failure. Here you expect to see the cells so another color would be welcome.

The idea was that it would be red by default, and replaced by green if the test passed.

>> LayoutTests/css3/calc/table-calcs.html:71
>> +      </table>
> 
> If you set border-spacing: 0px on your table (I would argue on all tables in your style), you would set your div's height to 400px and get the right value. This would be a lot more readable!

Good call! Done.

>> LayoutTests/css3/calc/table-empty-cells-expected-mismatch.html:1
>> +<style> 
> 
> The name does not mention quirks mode. Also I don't understand what it is doing in calc/ (no -webkit-calc in sight) and why we expect mismatch here?

We talked about this offline - but this was an expected failure. Now that more calc functionality has landed, I've changed this to an expected.

>> LayoutTests/css3/calc/table-empty-cells.html:1
>> +<style> 
> 
> Is quirks mode needed?

No. Fixed.

>> LayoutTests/css3/calc/table-empty-cells.html:8
>> +    .bgcolor { background-color: yellow } 
> 
> Not sure that it's a useful style.

Deleted.

>> LayoutTests/css3/calc/table-empty-cells.html:16
>> +        <td class="cell1"> </td>
> 
> There no cell1 class (not repeated in both files).

Good catch. Fixed by deleting existing cell1 and then replacing cell2 with cell1.

>> LayoutTests/css3/calc/table-empty-cells.html:40
>> +    <table cellpadding="0" cellspacing="0" width="100%">
> 
> cellpadding and cellspacing are obsolete, please use border-spacing instead.
> 
> Also factor out this style into your stylesheet and override it if needed locally:
> 
> table {
>     border-spacing: 0px;
>     width: 100%;
> }

Thanks - good idea. Done.
Comment 4 Mike Lawther 2012-03-01 22:05:12 PST
Created attachment 129812 [details]
Patch
Comment 5 Julien Chaffraix 2012-03-02 08:41:57 PST
Comment on attachment 129812 [details]
Patch

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

Much better, thanks!

> LayoutTests/css3/calc/table-empty-cells-expected.html:17
> +    <table width="100%">

Unneeded width = 100% on all your tables as you factored it in the style :) [not repeated below]

> LayoutTests/css3/calc/table-empty-cells.html:17
> +    <table width="100%">

Unneeded width = 100% on all your tables as you factored it in the style :) [not repeated below]
Comment 6 Mike Lawther 2012-03-04 12:26:59 PST
Comment on attachment 129812 [details]
Patch

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

>> LayoutTests/css3/calc/table-empty-cells-expected.html:17
>> +    <table width="100%">
> 
> Unneeded width = 100% on all your tables as you factored it in the style :) [not repeated below]

Oops - good catch, thanks! Fixed.

>> LayoutTests/css3/calc/table-empty-cells.html:17
>> +    <table width="100%">
> 
> Unneeded width = 100% on all your tables as you factored it in the style :) [not repeated below]

Oops - good catch, thanks! Fixed.
Comment 7 Mike Lawther 2012-03-04 12:27:50 PST
Created attachment 130029 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-03-04 12:43:58 PST
Comment on attachment 130029 [details]
Patch for landing

Clearing flags on attachment: 130029

Committed r109680: <http://trac.webkit.org/changeset/109680>
Comment 9 WebKit Review Bot 2012-03-04 12:44:03 PST
All reviewed patches have been landed.  Closing bug.