[webkit-reviews] review requested: [Bug 76553] min-width is not implemented on <table> for table-layout: auto : [Attachment 124402] 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 124402: Patch
https://bugs.webkit.org/attachment.cgi?id=124402&action=review

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

Thanks for the review! I've upload a new patch in response to the comments.

> > LayoutTests/fast/table/min-width-expected.txt:246
> > +PASS successfullyParsed is true
> 
> The output is huge - which is a good thing as you are covering most of the
cases - but I would love for the test to be split in 2 to help maintenance and
review.

Agreed. I've decided to split the tests out into 4 to make it a little easier
to manage:

min-width-html-table-block.js
min-width-html-table-inline.js
min-width-css-table-inline.js
min-width-css-table-block.js

They all use helper functions from a new JS file:

min-width-helpers.js

> > LayoutTests/fast/table/script-tests/min-width.js:93
> > +	 var propertyValue = table[propertyName];
> 
> I don't think this is sound. I don't see where you store the style value on
the object (I tried this way and I did not always get the right value). I may
be missing something but it looks like you want the computed value here and not
the raw CSS value. If that's the case, you should use
window.getComputedStyle(table, null)[propertyName].

I like your approach better. You've got it right; I do want the computed value.
Now, I'm querying the width and height properties instead of offsetWidth and
offsetHeight, and I've changed that line to: 

    var propertyValue = window.getComputedStyle(table,
null).getPropertyValue(propertyName);

I was querying table.offsetWidth through table["offsetWidth"], which seemed to
work for my purposes. However, I do prefer getComputedStyle because it's in the
DOM CSS spec and offsetWidth is not in any spec. According to Mozilla Developer
Network, "offsetWidth is not part of any W3C specification or technical
recommendation" (https://developer.mozilla.org/en/DOM/element.offsetWidth).

> > Source/WebCore/ChangeLog:31
> > +		 In FixedTableLayout.cpp, the computePreferredLogicalWidths
method looks like it has
> > +		 issues based on the comment: "FIXME: This entire calculation
is incorrect for both
> > +		 minwidth and maxwidth." (minwidth and maxwidth refer to the
preferred widths, not the
> > +		 min-width and max-width styles). I have not implemented
min-width for FixedTableLayout
> > +		 in this patch since it requires some more research around that
comment.
> > +
> > +		 min-width for the table-layout: fixed case has been split out
into this bug:
> > +		 https://bugs.webkit.org/show_bug.cgi?id=76948
> 
> This is a great comment, this is preferably put at the beginning of the CL to
underline it.

Sounds good. I've moved it up.

> > Source/WebCore/rendering/AutoTableLayout.cpp:274
> > +	 if (tableLogicalMinWidth.isFixed() && tableLogicalMinWidth.value() >
0) {
> 
> You can use Length::isPositive here.

Done. I've also changed the existing code for width (not min-width) that
appears just before this from:

    if (tableLogicalWidth.isFixed() && tableLogicalWidth > 0) {

To:

    if (tableLogicalWidth.isFixed() && tableLogicalWidth.isPositive()) {

> > Source/WebCore/rendering/RenderTable.cpp:231
> > +	 if ((styleLogicalWidth.isFixed() || styleLogicalWidth.isPercent()) &&
styleLogicalWidth.isPositive())
> 
> styleLogicalWidth.isFixed() || styleLogicalWidth.isPercent() can be replaced
by styleLogicalWidth.isSpecified()

Done. Nice catch! :).

> > Source/WebCore/rendering/RenderTable.cpp:253
> > +	 if ((styleMinLogicalWidth.isFixed() ||
styleMinLogicalWidth.isPercent()) && styleMinLogicalWidth.isPositive())
> 
> Ditto here.

Done.