Bug 13598 – <td> tag can cause Webkit to generate empty space

• Component: Tables
• Status: NEW

[Joshua Lippai]

Webkit generates empty space between two <td> elements in a couple of the tables on this page, and I can't immediately tell why from Web Inspector. I tagged this as major because it looks like it could be a general compatibility issue, the frequency of which we're striving to reduce. Then again, this may have been fixed in 522+ already, so it would be good if someone could verify whether this is the case (I'll actually try that tonight).

[David Kilzer (:ddkilzer)]

Confirmed with a local debug build of WebKit r21253 with Safari 2.0.4 (419.3) on Mac OS X 10.4.9 (8P135).

[David Kilzer (:ddkilzer)]

Created attachment 14362 [details]
Screenshot of issue

In this case, the <td> has its width attribute set to 77px, but it's rendered at 101px.

[David Kilzer (:ddkilzer)]

No gap using Opera 9.10 or Firefox 2.0.0.3.

Mac IE 5.2.3 has the same bug as WebKit.

Created attachment 14372 [details]
Reduction

[Arpita Bahuguna]

Created attachment 161693 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161693 [details]
Patch

Attachment 161693 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13728106

New failing tests:
fast/table/003.html
fast/table/table-colspan-cell-widths.html

[Arpita Bahuguna]

Created attachment 162205 [details]
Patch

[Arpita Bahuguna]

Uploaded another patch to get the archive of the failing tests which was missing in the first patch.

[WebKit Review Bot]

Comment on attachment 162205 [details]
Patch

Attachment 162205 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13739991

New failing tests:
fast/table/003.html
fast/table/table-colspan-cell-widths.html

[Julien Chaffraix]

Comment on attachment 162205 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162205&action=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.

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

[Arpita Bahuguna]

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

[Arpita Bahuguna]

Created attachment 162765 [details]
Patch

[WebKit Review Bot]

Comment on attachment 162765 [details]
Patch

Attachment 162765 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13768970

New failing tests:
fast/table/003.html

[Arpita Bahuguna]

Created attachment 162953 [details]
Patch

[WebKit Review Bot]

Comment on attachment 162953 [details]
Patch

Attachment 162953 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13795237

New failing tests:
fast/table/003.html

[Arpita Bahuguna]

Created attachment 162956 [details]
Patch

[WebKit Review Bot]

Comment on attachment 162956 [details]
Patch

Attachment 162956 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13785970

New failing tests:
fast/table/003.html

[Julien Chaffraix]

Comment on attachment 162956 [details]
Patch

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

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

[Arpita Bahuguna]

Created attachment 164287 [details]
Patch

[Arpita Bahuguna]

Created attachment 164288 [details]
Examples for explanation of width distribution.

[Arpita Bahuguna]

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

[WebKit Review Bot]

Comment on attachment 164287 [details]
Patch

Attachment 164287 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13859582

New failing tests:
fast/table/003.html
tables/mozilla_expected_failures/bugs/bug80762-2.html

[Arpita Bahuguna]

Created attachment 164377 [details]
Patch

[Arpita Bahuguna]

Uploaded another patch for handling the newly failing test cases.

[WebKit Review Bot]

Comment on attachment 164377 [details]
Patch

Attachment 164377 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13861967

New failing tests:
fast/table/003.html
fast/table/colspanMinWidth-vertical.html
fast/table/colspanMinWidth.html
tables/mozilla/core/misc.html
tables/mozilla_expected_failures/bugs/bug80762-2.html
tables/mozilla_expected_failures/bugs/bug1647.html

[Arpita Bahuguna]

Hi Julien, the last patch has fix for the following things:
1. percent width handling for colspan.
2. fixed width handling for colspan.
3. handling when colspan count is more than the number of spanning <td>s in the other row, introducing empty cells.

The fix for 
1. percent width handling for colspan, and
2. fixed width handling for colspan
cause the following two tests to fail but it is expected and we behave as per FF and Opera after the patch.
fast/table/003.html
tables/mozilla_expected_failures/bugs/bug80762-2.html
(this is the last but one patch)

The fix for
3. handling when colspan count is more than the number of spanning <td>s in the other row, introducing empty cells.
cause the following tests to fail which is again expected as we now follow similar to FF and Opera:
tables/mozilla/core/misc.html
tables/mozilla_expected_failures/bugs/bug1647.html
fast/table/colspanMinWidth-vertical.html
fast/table/colspanMinWidth.html
(this is the last patch)

Do you think I should first patch 1. & 2. (as mentioned above) and 3. as a separate patch?

Also. not only for this bug but for others as well, I am unable to patch the new expected results properly causing the cr-linux bot to always go red.
This seems to happen only for the tests that have the expected image in the cr-linux path as well as its fallback cr-win path whereas the .txt is only available in the cr-win fallback path.

I understand that I might be doing something wrong, but can you please tell me where I am going wrong as I have tried various combinations to get the patch right but have been unsuccessful so far.

[Arpita Bahuguna]

Created attachment 166461 [details]
Patch

[Build Bot]

Comment on attachment 166461 [details]
Patch

Attachment 166461 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14086807

New failing tests:
fast/table/table-colspan-cell-widths.html

[Arpita Bahuguna]

Created attachment 167973 [details]
Patch

[Build Bot]

Comment on attachment 167973 [details]
Patch

Attachment 167973 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15906498

New failing tests:
tables/mozilla_expected_failures/bugs/bug1647.html

[Andreas Kling]

Comment on attachment 167973 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Ahmad Saleem]

Created attachment 460340 [details]
Safari 15.5 differs from other browsers

I am able to reproduce this bug in Safari 15.5 on macOS 12.4 based on attached reduction test case. All other browser render same as shown in the picture.

Look into (1) block [lime green] not filling up to table cell width. Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/95500818>