[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 #21 from Arpita Bahuguna <arpitabahuguna at gmail.com>  2012-09-15 09:23:28 PST ---
(In reply to comment #18)
> (From update of attachment 162956 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162956&action=review
> 

Hi Julien, thanks for the review.
Have tried to ammend the fixed width distribution as well along with this patch (in addition to the previous fix). We now follow FF and Opera (Opera behaves slightly differently for a particular case but FF seems to make more sense) for this width distribution logic.

I have attached a file with examples to explain the behavior in other browsers.

Earlier, we used to just allocate the logical width for fixed width columns and distribute the remaining width amongst the remaining columns.

Currently (trying to follow FF/Opera) I first allocate for percent width columns in the specified ratio. Next, we allocate for the fixed (fixed, rel, viewport) width cols (but only if we have at least one auto width col), upto a maximum of the effective max logical width.

Finally, we take care of the remaining columns (which are not covered by the previous two loops) using the existing logic.

Have also made the other changes as suggested by you.

> > Source/WebCore/ChangeLog:10
> > +        For tables with colspan, the algorithm for distribution of width between
> > +        the spanned columns is improper in case spanned cells with percent widths are
> > +        specified.
> 
> This is a pattern I have seen in your ChangeLogs: I can't figure out what is wrong just by reading them. "improper" is *not* a useful word as it could mean *anything*. Please use technically relevant details and explain the 'why' as much as you can. I would have given a try at this description but there is too many holes for me to fill at the moment.
> 
> > Source/WebCore/ChangeLog:21
> > +        After allocating the extra width over the columns with fixed width, the remaining
> > +        extra width should then be distributed amongst percent width columns (if any) before
> > +        prioritizing the auto or relative width columns.
> 
> I don't think I agree with your algorithm for spreading the extra logical width here. It would make way more sense to do the prioritization differently: 'auto' width first as they have no constrain on them, then 'percent' to try to keep the requested ratio and finally fixed widths ('fixed', viewport relative, calc). How does other browsers handle that?
> 
> > Source/WebCore/ChangeLog:24
> > +        For this, we need to first take the minimum logical width consumed by the auto or relative
> > +        width columns if any, and the extra width after taking out this value should be
> 
> relative width has a meaning in CSS that is likely not the one you are thinking of. 'em', 'ex' are relative units as they depends on your font.
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:420
> > +                int autoMinLogicalWidth = 0;
> > +                int widthForPercentAllocation = 0;
> 
> WebKit style is to move variable definitions to where they are needed.
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:422
> >                  // Give min to variable first, to fixed second, and to others third.
> 
> This comment should be updated to outline the new spreading algorithm.
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:434
> > +                // Take into account the minimum width for auto cells.
> 
> This comment is a 'what' comment and not super useful. On top of that, it's wrong as you recognized yourself as you bundle other non-handled length type together under the 'auto' umbrella.
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:437
> > +                        autoMinLogicalWidth += m_layoutStruct[pos].effectiveMinLogicalWidth;
> 
> > For that I wish to get the cumulative of the remaining columns i.e. of auto and relative type (or other types, if any).
> 
> I don't think it's correct to say 'or other types, if any' without thinking about those cases. Using explicitly checking for what you want is more correct and future proof. Again maybe it's right but the absence of good justification makes it hard to see.
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:440
> > +                // Next, allocate for the percent cells.
> 
> Another mostly useless 'what' comment. We prefer 'why' comments.
> 
> > LayoutTests/ChangeLog:17
> > +        The existing test case result needs modification as it is affected by the change.
> 
> Another example of a useless sentence. How can anybody evaluate if your rebaselining is correct? Helper the maintainers (and the reviewers) by stating what is the difference and why it is correct.

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