[Webkit-unassigned] [Bug 13598] <td> tag can cause Webkit to generate empty space

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Arpita Bahuguna <arpitabahuguna at gmail.com>  2012-09-07 06:28:39 PST ---
(In reply to comment #10)
> (From update of attachment 162205 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162205&action=review
Hi Julien, thanks for the review.
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:436
> > +                    if (!((m_layoutStruct[pos].logicalWidth.isPercent() || m_layoutStruct[pos].logicalWidth.isFixed())) && totalPercent > 0 && fixedWidth <= cellMinLogicalWidth)
> 
> I don't think that's right. How about calc() / viewport relative logical width(s)? How about the future if we support other values? If you want to handle only 'auto' wdith, you should probably check it explicitly and not check anything non-auto.
> 
The reason why such a check is required is because am trying to follow the width distribution algorithm in AutoTableLayout::layout(), wherein fixed width columns are given the highest priority followed by percent and then followed by relative and auto width types.

For this particular change, we have already distributed the width over fixed columns and next I want to prioritize percent width columns. For that I wish to get the cumulative of the remaining columns i.e. of auto and relative type (or other types, if any).

So since fixed is allocated and percent shall be allocated next, anything remaining should be accounted for by that loop and hence the check for !fixed || !percent.
Probably the variable name autoMinLogicalWidth is misleading to this extent.


> > LayoutTests/fast/table/table-colspan-cell-widths.html:6
> > +    cellspacing: 0;
> > +    cellpadding: 0;
> 
> This is wrong and you can see that in the TEXT dump as your cells are wider than your absolute width.
> 
> |cellspacing| and |cellpadding| are HTML attributes not CSS properties. The CSS equivalent are resp. border-spacing and padding.
> 
> AFAICT you don't need the padding rule as we use the cells' border boxes to compute our widths but could be wrong.
> 
> > LayoutTests/fast/table/table-colspan-cell-widths.html:19
> > +        <td width="100%">
> > +            <div style="width: 50px; background-color: green;">&nbsp;</div>
> > +        </td>            
> 
> This doesn't make much sense to not set a background-color on your cell as you don't see that it's properly sized.
Have made the aforementioned changes. You were right, the padding rule was not required.

Shall be uploading a patch with the suggested changes shortly.

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