[Webkit-unassigned] [Bug 76553] min-width is not implemented on <table> for table-layout: auto

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76553


Max Vujovic <mvujovic at adobe.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #123850|0                           |1
        is obsolete|                            |
 Attachment #124402|                            |review?, commit-queue?
               Flag|                            |




--- Comment #14 from Max Vujovic <mvujovic at adobe.com>  2012-01-27 16:50:38 PST ---
Created an attachment (id=124402)
 --> (https://bugs.webkit.org/attachment.cgi?id=124402&action=review)
Patch

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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.