Bug 75898 – table tests for CSS3 calc

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mike Lawther]

table tests for CSS3 calc

[Mike Lawther]

Created attachment 121736 [details]
Patch

[Julien Chaffraix]

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%;
}

[Mike Lawther]

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.

[Mike Lawther]

Created attachment 129812 [details]
Patch

[Julien Chaffraix]

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]

[Mike Lawther]

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.

[Mike Lawther]

Created attachment 130029 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 130029 [details]
Patch for landing

Clearing flags on attachment: 130029

Committed r109680: <http://trac.webkit.org/changeset/109680>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.