[webkit-reviews] review granted: [Bug 76553] min-width is not implemented on <table> for table-layout: auto : [Attachment 124402] Patch

Mon Jan 30 19:48:58 PST 2012

Julien Chaffraix <jchaffraix at webkit.org> has granted Max Vujovic
<mvujovic at adobe.com>'s request for review:
Bug 76553: min-width is not implemented on <table> for table-layout: auto
https://bugs.webkit.org/show_bug.cgi?id=76553

Attachment 124402: Patch
https://bugs.webkit.org/attachment.cgi?id=124402&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=124402&action=review

I did not repeat the comment for all the tests but all comments apply to the 4
tests.

> LayoutTests/fast/table/min-width-css-table-block-expected.txt:6
> +PASS computeTableProperty(/*propertyName*/ 'width', /*isCSSTable*/ true,
/*min-intrinsic*/ 100, /*max-intrinsic*/ 250, /*extraTableStyle*/ 'min-width:
500px; display: table; padding-left: 10px; padding-right: 10px;
border-left-width: 5px; border-right-width: 5px; ') is '500px'

If you put all the default styles in a common CSS class and dumped it once at
the beginning, the output would be way more readable! We know that a CSS table
should use display: table, ...

> LayoutTests/fast/table/min-width-css-table-block-expected.txt:11
> +PASS computeTableProperty(/*propertyName*/ 'width', /*isCSSTable*/ true,
/*min-intrinsic*/ 100, /*max-intrinsic*/ 250, /*extraTableStyle*/ 'min-width:
500px; width: 60%; display: table; padding-left: 10px; padding-right: 10px;
border-left-width: 5px; border-right-width: 5px; ') is '570px'

Interestingly we don't include borders & padding if width is a percentage for
CSS table. This is inconsistent with the HTML case or even the CSS one. Is this
what other browsers return? (agreed, it's another issue but it could be
bug-worthy)

> LayoutTests/fast/table/min-width-css-table-block-expected.txt:36
> +PASS computeTableProperty(/*propertyName*/ 'height', /*isCSSTable*/ true,
/*min-intrinsic*/ 100, /*max-intrinsic*/ 250, /*extraTableStyle*/ 'min-height:
500px; display: table; -webkit-writing-mode: vertical-rl; writing-mode:
vertical-rl; padding-top: 10px; padding-bottom: 10px; border-top-width: 5px;
border-bottom-width: 5px; ') is '500px'

Please add a debug() comment here and an empty line to see the switch between
writing modes.

> LayoutTests/fast/table/min-width-css-table-block.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Nit: we usually use <!DOCTYPE html>