[Webkit-unassigned] [Bug 14858] <col> width ignored when not tied to a single cell

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Mar 21 12:44:37 PDT 2010


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


David Kilzer (ddkilzer) <ddkilzer at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #49318|review-                     |review+, commit-queue-
               Flag|                            |




--- Comment #25 from David Kilzer (ddkilzer) <ddkilzer at webkit.org>  2010-03-21 12:44:37 PST ---
(From update of attachment 49318)
(In reply to comment #22)
> (From update of attachment 49318 [details])
> This looks great!  Just a couple nits below:
> 
> > Index: WebCore/ChangeLog
> > +        Fixed width calculation for cells with span when <col> is defined
> 
> You need to include a link to the bug in the ChangeLog entry.
> 
> > +        * rendering/RenderTableCell.cpp:
> > +        (WebCore::RenderTableCell::styleOrColWidth): added the calculation of cell width
> > +        in case of <col> defined and span>1.
> 
> Nit:  Change "span>1" to "span > 1".
> 
> > Index: WebCore/rendering/RenderTable.cpp
> > +    RenderTableCol* colElem = toRenderTableCol(child);
> > +    while (colElem) {
> > +        int span = colElem->span();
> > +        if (!colElem->firstChild()) {
> > +            int startCol = cCol;
> > +            int endCol = cCol + span - 1;
> > +            cCol += span;
> > +            if (cCol > col) {
> > +                if (startEdge)
> > +                    *startEdge = startCol == col;
> > +                if (endEdge)
> > +                    *endEdge = endCol == col;
> > +                return colElem;
> > +            }
> > +        }
> > +        colElem = nextColElement(colElem);
> > +        if (!colElem)
> >              break;
> >      }
> 
> You can actually leave off the final "if (!colElem) break;" statements in the
> while loop since the next iteration through the loop will catch this condition
> and then return 0.
> 
> > Index: WebCore/rendering/RenderTableCell.cpp
> > ===================================================================
> > --- WebCore/rendering/RenderTableCell.cpp	(revision 53844)
> > +++ WebCore/rendering/RenderTableCell.cpp	(working copy)
> > @@ -83,18 +83,42 @@ void RenderTableCell::updateFromElement(
> >  Length RenderTableCell::styleOrColWidth() const
> >  {
> >      Length w = style()->width();
> > -    if (colSpan() > 1 || !w.isAuto())
> > +    if (!w.isAuto())
> >          return w;
> > +
> >      RenderTableCol* tableCol = table()->colElement(col());
> > +
> >      if (tableCol) {
> > -        w = tableCol->style()->width();
> > -        
> > +        int colSpanCount = colSpan();
> > +
> > +        Length colWidthSum = Length(0, Fixed);
> > +        for (int i = 1; i <= colSpanCount; i++) {
> > +            Length colWidth = tableCol->style()->width();
> > +
> > +            // Percentage value should be returned only for colSpan=1.
> > +            // Otherwise we return original width for the cell.
> 
> Why is this true?  Is this part of the standard?  Is this covered by an
> existing test case?  (If not, you need to add a test case for this condition as
> well.)

I reviewed the new logic again per Comment #23, and I agree it preserves the
previous behavior, so this is no longer an issue.

> Nit:  "colSpan=1" should be "colSpan == 1" to be clearer that it's an equality
> check.
> 
> > +            if (!colWidth.isFixed()) {
> > +                if (colSpanCount > 1)
> > +                    return w;
> > +                return colWidth;
> > +            }
> > Index: LayoutTests/ChangeLog
> > +2010-01-27  Dmitry Gorbik  <socket.h at gmail.com>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Fixed width calculation for cells with span when <col> is defined
> 
> You need to include a link to the bug in the ChangeLog entry.
> 
> > Index: LayoutTests/fast/table/col-width-span-expand.html
> > +	<head>
> 
> There is a tab on this line that should be 8 spaces.

r=me!  I'll clean up the nits above and land this patch.

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


More information about the webkit-unassigned mailing list