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).
Confirmed with a local debug build of WebKit r21253 with Safari 2.0.4 (419.3) on Mac OS X 10.4.9 (8P135).
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.
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
Created attachment 161693 [details] Patch
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
Created attachment 162205 [details] Patch
Uploaded another patch to get the archive of the failing tests which was missing in the first patch.
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
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;"> </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.
(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;"> </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.
Created attachment 162765 [details] Patch
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
Created attachment 162953 [details] Patch
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
Created attachment 162956 [details] Patch
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
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.
Created attachment 164287 [details] Patch
Created attachment 164288 [details] Examples for explanation of width distribution.
(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.
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
Created attachment 164377 [details] Patch
Uploaded another patch for handling the newly failing test cases.
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
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.
Created attachment 166461 [details] Patch
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
Created attachment 167973 [details] Patch
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
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.
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!
<rdar://problem/95500818>