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

[bugzilla-daemon at webkit.org]

Max Vujovic <mvujovic at adobe.com> has asked  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 124874: Patch
https://bugs.webkit.org/attachment.cgi?id=124874&action=review

------- Additional Comments from Max Vujovic <mvujovic at adobe.com>
(In reply to comment #15)
> (From update of attachment 124402 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=124402&action=review

Thanks for the review! I've uploaded another patch to address the readability
issues with the 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, ...

I refactored the tests so that they produce much easier to read output but
still describe what they are testing.

I took your suggestion and factored out the default styles into a new
stylesheet: LayoutTests/fast/table/resources/min-width.css.

I tried displaying the style sheet at the beginning of each test file, but it
didn't look good. Instead, I've put a note at the beginning of each test that
links to the style sheet and describes the important properties that are
constant across all of the tests in the file.

Here's the note:

"The stylesheet used to style the table in each test is available at:
LayoutTests/fast/table/resources/min-width.css

Most importantly, note that each table has:
- minimum intrinsic width and height both equal to 100px based on the table
content
- maximum intrinsic width and height both equal to 250px based on the table
content
- borders and paddings that add up to 30px in both the horizontal and vertical
directions
- a parent whose dimensions are 1000px by 1000px

The function signature of computeLogicalWidth is:
function computeLogicalWidth(writingMode, direction, tableStyle)"

> > 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)

I opened bug 77028 a little while ago about this issue. Firefox and Opera
display borders and paddings in addition to percent width. WebKit does not.
WebKit considers borders and paddings as already included in the percent width.
After bug 77028 gets fixed, I can update the expected test results for percent
min-width.

> > 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.

Done. I've added debug() comments to highlight the switches between writing
modes and directions. Thanks for the tip! It looks much better now.

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

Done. Good to know!