Bug 98455 – max-width property is does not overriding the width properties for css tables(display:table)

• Component: Tables
• Status: RESOLVED FIXED

[Pravin D]

Created attachment 167185 [details]
TestCase

The max-width property does not override the width property in case of a container having display:table.

Have attached a reduced test case for the same(from bug 25016).
courtesy: [e-mail]

[Pravin D]

Created attachment 167187 [details]
TestCase

[Pravin D]

Created attachment 167194 [details]
Patch

[Tony Chang]

Did you check to see what IE and Firefox do?  In bug 2516, Julien said that this behavior is not specified, in which case, we should see what other browsers do and try to get this spec'ed.

[Pravin D]

(In reply to comment #3)
> Did you check to see what IE and Firefox do?  In bug 2516, Julien said that this behavior is not specified, in which case, we should see what other browsers do and try to get this spec'ed.

> 

I did check with IE and FF. They respect max-width value. We will have similar behavior with this patch applied.

Also we have algorithms(Auto table layout and Fixed table layout) in place to distribute width among the cells when either the width of the table is greater or lesser than the sum of widths of its columns.
However there is no such algorithm(defined by CSS) for distributing height among rows of the table. I think Julien was addressing this issue in his Comment #11 on bug 25016. IMHO we should not have much of a problem in addressing the max-width case as most of the required stuff's in place.

[Tony Chang]

Comment on attachment 167194 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167194&action=review

Julien may have additional comments.

> Source/WebCore/rendering/RenderTable.cpp:266
> +    if (styleMaxLogicalWidth.isSpecified() && styleMaxLogicalWidth.isPositive())

0 should be a valid value for max-width.  I think you want !styleMaxLogicalWidth.isNegative().  Maybe add a test case for this?

> Source/WebCore/rendering/RenderTable.cpp:269
> +    setLogicalWidth(max<int>(computedMinLogicalWidth, min(logicalWidth(), computedMaxLogicalWidth)));

The mixing of max (template) and min (no-template) styles is weird, but maybe it's intentional to do the conversion to int at the last moment?

[Pravin D]

Created attachment 167598 [details]
Patch

[Pravin D]

(In reply to comment #5)
> (From update of attachment 167194 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167194&action=review
> 
Have upload a new patch with following changes:

> > Source/WebCore/rendering/RenderTable.cpp:266
> > +    if (styleMaxLogicalWidth.isSpecified() && styleMaxLogicalWidth.isPositive())
> 
> 0 should be a valid value for max-width.  I think you want !styleMaxLogicalWidth.isNegative().  Maybe add a test case for this?
> 
Changed  styleMaxLogicalWidth.isPositive() to !styleMaxLogicalWidth.isNegative().  Also added a new case for the max-width = 0 case.

> > Source/WebCore/rendering/RenderTable.cpp:269
> > +    setLogicalWidth(max<int>(computedMinLogicalWidth, min(logicalWidth(), computedMaxLogicalWidth)));
> 
> The mixing of max (template) and min (no-template) styles is weird, but maybe it's intentional to do the conversion to int at the last moment?
> 
As per the discussion with Tony(on IRC) removal of max(tempate) will be handled in a follow-up bug
as it might affect sub-pixel related test cases.

[Tony Chang]

Comment on attachment 167598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167598&action=review

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +

Nit: extra blank line.

[Tony Chang]

Comment on attachment 167598 [details]
Patch

This is causing a bunch of tests to fail.

[Pravin D]

Created attachment 167633 [details]
Patch

[Tony Chang]

Comment on attachment 167633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167633&action=review

> Source/WebCore/rendering/RenderTable.cpp:259
> +    LayoutUnit computedMaxLogicalWidth = max(logicalWidth(), availableLogicalWidth);

I think this could just be logicalWidth().  Which is to say, if max-width is not specified, the min(logicalWidth(), computedMaxLogicalWidth) line does nothing.

Another way to write the code would be:
if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative())
    setLogicalWidth(min<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMaxLogicalWidth, availableLogicalWidth)));

if (styleMinLogicalWidth.isSpecified() && styleMinLogicalWidth.isPositive())
    setLogicalWidth(max<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMinLogicalWidth, availableLogicalWidth)));

[Pravin D]

(In reply to comment #11)
> (From update of attachment 167633 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167633&action=review
> 
> > Source/WebCore/rendering/RenderTable.cpp:259
> > +    LayoutUnit computedMaxLogicalWidth = max(logicalWidth(), availableLogicalWidth);
> 
> I think this could just be logicalWidth().  Which is to say, if max-width is not specified, the min(logicalWidth(), computedMaxLogicalWidth) line does nothing.
> 
Think so...

> Another way to write the code would be:
> if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative())
>     setLogicalWidth(min<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMaxLogicalWidth, availableLogicalWidth)));
> 
> if (styleMinLogicalWidth.isSpecified() && styleMinLogicalWidth.isPositive())
>     setLogicalWidth(max<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMinLogicalWidth, availableLogicalWidth)));
> 
Seems like with this approach we can do away with the local variables and their initializations.

Will upload another patch with above approach...

[Julien Chaffraix]

Comment on attachment 167633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167633&action=review

The change looks globally good.

As a side note, we should probably think about adding a function similar to 
RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth that works on non-replaced elements.

>>> Source/WebCore/rendering/RenderTable.cpp:259
>>> +    LayoutUnit computedMaxLogicalWidth = max(logicalWidth(), availableLogicalWidth);
>> 
>> I think this could just be logicalWidth().  Which is to say, if max-width is not specified, the min(logicalWidth(), computedMaxLogicalWidth) line does nothing.
>> 
>> Another way to write the code would be:
>> if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative())
>>     setLogicalWidth(min<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMaxLogicalWidth, availableLogicalWidth)));
>> 
>> if (styleMinLogicalWidth.isSpecified() && styleMinLogicalWidth.isPositive())
>>     setLogicalWidth(max<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMinLogicalWidth, availableLogicalWidth)));
> 
> Think so...

I agree with Tony, the variables are unneeded outside the if statements (I don't mind keeping them for clarity but they don't need to be at the function scope).

Also variables should be defined them when you need them not before as you are doing now.

> Source/WebCore/rendering/RenderTable.cpp:266
> +    if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative())

Per CSS 2.1, "Negative values for 'min-width' and 'max-width' are illegal.". Both property behave the same with respect to negative values so I would rather that we write both if statements in the same way (0 for any of the value should be rare anyway).

> LayoutTests/fast/table/css-table-max-width.html:52
> +    </div>

I would throw a table with fixed layout in your test as you fix both auto and fixed table layout.

[Pravin D]

Created attachment 167714 [details]
Patch

[Pravin D]

(In reply to comment #13)
> (From update of attachment 167633 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167633&action=review
> 
> The change looks globally good.
> 
> As a side note, we should probably think about adding a function similar to 
> RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth that works on non-replaced elements.
> 
I guess so... I'll raise a bug for the same...
> >>> Source/WebCore/rendering/RenderTable.cpp:259
> >>> +    LayoutUnit computedMaxLogicalWidth = max(logicalWidth(), availableLogicalWidth);
> >> 
> >> I think this could just be logicalWidth().  Which is to say, if max-width is not specified, the min(logicalWidth(), computedMaxLogicalWidth) line does nothing.
> >> 
> >> Another way to write the code would be:
> >> if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative())
> >>     setLogicalWidth(min<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMaxLogicalWidth, availableLogicalWidth)));
> >> 
> >> if (styleMinLogicalWidth.isSpecified() && styleMinLogicalWidth.isPositive())
> >>     setLogicalWidth(max<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMinLogicalWidth, availableLogicalWidth)));
> > 
> > Think so...
> 
> I agree with Tony, the variables are unneeded outside the if statements (I don't mind keeping them for clarity but they don't need to be at the function scope).
> 
> Also variables should be defined them when you need them not before as you are doing now.
> 
Have uploaded with these suggested changes.

> > Source/WebCore/rendering/RenderTable.cpp:266
> > +    if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative())
> 
> Per CSS 2.1, "Negative values for 'min-width' and 'max-width' are illegal.". Both property behave the same with respect to negative values so I would rather that we write both if statements in the same way (0 for any of the value should be rare anyway).
> 
In the patch handled the case where max-width and min-width can have non-negative values. 
The reasoning is that as 0 is a rare value for the above properties
if (styleMinLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative())
will not be called often for 0 case(not much of a perf hit) 

and 

if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative())
will also take care of the case when max-width:0, potential bug which need not be addressed later.
I think you meant otherwise(i.e zero value case can be safely ignored). If you want I can upload another patch with styleMaxLogicalWidth.isPositive() and 
styleMinLogicalWidth.isPositive() in place of non-negative check.

> > LayoutTests/fast/table/css-table-max-width.html:52
> > +    </div>
> 
> I would throw a table with fixed layout in your test as you fix both auto and fixed table layout.
> 
Done

[WebKit Review Bot]

Comment on attachment 167714 [details]
Patch

Clearing flags on attachment: 167714

Committed r130774: <http://trac.webkit.org/changeset/130774>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.