min-width is not implemented on <table> for table-layout: fixed

• Component: Tables
• Status: RESOLVED CONFIGURATION CHANGED

[Max Vujovic]

Created attachment 123815 [details]
Reproduction

This bug tracks min-width for table-layout: fixed.
Bug 76553 tracks min-width for table-layout: auto.

Both bugs are children of master bug 12396.

In the attached reproduction, the table style is "table-layout: fixed; width: 400px; min-width: 800px;". In FF and Opera, the table is 800px wide. In WebKit, it is 400px wide because min-width for table-layout: fixed has not been implemented yet.

[Antaryami Pandia (apandia)]

Please let me know if anybody is working on this.If not I am interested in working on this.

[Max Vujovic]

I haven't started any work on this bug, and I don't think anyone else has expressed interest in taking it yet, so you're welcome to pick it up :).

FYI, I just landed bug 76553, which implemented min-width for table-layout: auto. I decided to separate the auto and fixed cases because the computePreferredLogicalWidths method in FixedTableLayout.cpp has a comment: "FIXME: This entire calculation is incorrect for both minwidth and maxwidth." (where minwidth and maxwidth refer to the preferred widths, not the min-width and max-width styles). I would do some research around that comment as you work on this.

[Antaryami Pandia (apandia)]

Thanks for the information.I will start working on this.

[Antaryami Pandia (apandia)]

Hi Max,
Your change for issue 76553, seems to have fixed this also.

I had a look at you patch for issue 76553, and the below code in "RenderTable.cpp" seems to have fixed it:-

Method :- "RenderTable::computeLogicalWidth"
// Ensure we aren't smaller than our min-width style.
Length styleMinLogicalWidth = style()->logicalMinWidth();
if (styleMinLogicalWidth.isSpecified() && styleMinLogicalWidth.isPositive())
    setLogicalWidth(max(logicalWidth(), ConvertStyleLogicalWidthToComputedWidth(styleMinLogicalWidth, availableLogicalWidth)));

I think this piece of code takes the css min-width into consideration and sets width it accordingly.

Another Point I Observed:-

I think the above code will also be working for "table-layout: auto" case (issue 76553). I tried by removing the modified code from "AutoTableLayout::computePreferredLogicalWidths" and found that min-width for "table-layout: auto" is also working.

Code that I removed:-
Method :- AutoTableLayout::computePreferredLogicalWidths
Length tableLogicalMinWidth = m_table->style()->logicalMinWidth();
if (tableLogicalMinWidth.isFixed() && tableLogicalMinWidth.isPositive()) {
    minWidth = max<int>(minWidth, tableLogicalMinWidth.value());
    maxWidth = max<int>(minWidth, maxWidth);
}

I think either of the code could have fixed the issue for "table-layout: auto". So is there any specific reason for both the code changes?

Note:- I should have comment the in the issue 76553, for "table-layout: auto" case. But I am doing it here because that issue is already resolved and The code change also holds for this issue.

Please provide your feedback.

[Max Vujovic]

(In reply to comment #4)
> I had a look at you patch for issue 76553, and the below code in "RenderTable.cpp" seems to have fixed it:-
> 
> Method :- "RenderTable::computeLogicalWidth"
> // Ensure we aren't smaller than our min-width style.
> Length styleMinLogicalWidth = style()->logicalMinWidth();
> if (styleMinLogicalWidth.isSpecified() && styleMinLogicalWidth.isPositive())
>     setLogicalWidth(max(logicalWidth(), ConvertStyleLogicalWidthToComputedWidth(styleMinLogicalWidth, availableLogicalWidth)));
> 
> I think this piece of code takes the css min-width into consideration and sets width it accordingly.

It does seem to work in most cases, but I think some cases would fail for fixed table layout without additional code in FixedTableLayout::computePreferredLogicalWidths. I'm not exactly sure what these cases are. This is related to your observation below.

> Another Point I Observed:-
> 
> I think the above code will also be working for "table-layout: auto" case (issue 76553). I tried by removing the modified code from "AutoTableLayout::computePreferredLogicalWidths" and found that min-width for "table-layout: auto" is also working.
> 
> Code that I removed:-
> Method :- AutoTableLayout::computePreferredLogicalWidths
> Length tableLogicalMinWidth = m_table->style()->logicalMinWidth();
> if (tableLogicalMinWidth.isFixed() && tableLogicalMinWidth.isPositive()) {
>     minWidth = max<int>(minWidth, tableLogicalMinWidth.value());
>     maxWidth = max<int>(minWidth, maxWidth);
> }
> 
> I think either of the code could have fixed the issue for "table-layout: auto". So is there any specific reason for both the code changes?

If you look just above that code in AutoTableLayout::computePreferredLogicalWidths, you will see similar code that adjusts the min and max logical widths if a fixed width style is defined:

    if (tableLogicalWidth.isFixed() && tableLogicalWidth.isPositive()) {
        minWidth = max<int>(minWidth, tableLogicalWidth.value());
        maxWidth = minWidth;
    } else if (!remainingPercent && maxNonPercent) {
    
Based on your observation, I would think that this code is also unnecessary because RenderTable::computeLogicalWidth covers fixed and percent width styles. However, if you comment out that code in AutoTableLayout::computePreferredLogicalWidths like this:

    if (tableLogicalWidth.isFixed() && tableLogicalWidth.isPositive()) {
        //minWidth = max<int>(minWidth, tableLogicalWidth.value());
        //maxWidth = minWidth;
    } else if (!remainingPercent && maxNonPercent) {

You will see that these tests fail:

  fast/table/028-vertical.html
  fast/table/029.html
  fast/table/auto-100-percent-width.html
  fast/table/colspan-with-all-percent-cells.html

This suggests that some table layout code is depending on RenderTable's m_minPreferredLogicalWidth and m_maxPreferredLogicalWidth being set with fixed width styles. I haven't tracked it down though, but if you do, that would fantastic.

If you find out why, it would be nice to take out the code you want to remove for min-width *and* its related code for width, or at least to understand exactly what depends on that code to write some additional tests.

I'm working on some SVG bugs currently, so thanks for looking at this bug :).

[Antaryami Pandia (apandia)]

(In reply to comment #5)
> This suggests that some table layout code is depending on RenderTable's m_minPreferredLogicalWidth and m_maxPreferredLogicalWidth being set with fixed width styles. I haven't tracked it down though, but if you do, that would fantastic.
> 
> If you find out why, it would be nice to take out the code you want to remove for min-width *and* its related code for width, or at least to understand exactly what depends on that code to write some additional tests.
> 
> I'm working on some SVG bugs currently, so thanks for looking at this bug :).

Yes, I will look into this.

[Antaryami Pandia (apandia)]

(In reply to comment #5)
Yes the code AutoTableLayout::computePreferredLogicalWidths is required. Actually what I was saying was "AutoTableLayout::computePreferredLogicalWidths" already sets the minwidth and maxwidth, so what's purpose of code added in "RenderTable::computeLogicalWidth". Does this code solves any specific issue?

>>It does seem to work in most cases, but I think some cases would fail for fixed table layout without additional code in >>FixedTableLayout::computePreferredLogicalWidths. I'm not exactly sure what these cases are. This is related to your observation below.

Actually I was also thinking that can we put code similar to "AutoTableLayout" in "FixedTableLayout".

And if the code in "RenderTable::computeLogicalWidth" does not have any specific use case can we remove this. It's not because I don't like the code but think it suppresses some unknown like it fixes the "table-layout: fixed" case without any code being added to "FixedTableLayout.cpp". Here I am assuming that the purpose of the code in RenderTable.cpp is not to fix the "table-layout: fixed" case. Please correct me if I am wrong.

[Max Vujovic]

(In reply to comment #7)
> (In reply to comment #5)
> Yes the code AutoTableLayout::computePreferredLogicalWidths is required. Actually what I was saying was "AutoTableLayout::computePreferredLogicalWidths" already sets the minwidth and maxwidth, so what's purpose of code added in "RenderTable::computeLogicalWidth". Does this code solves any specific issue?

If I'm understanding your question correctly, the code in RenderTable::computeLogicalWidth is necessary to handle percent widths and percent min-widths. Specifically, the following line in RenderTable::convertStyleLogicalWidthToComputedWidth resolves a percent value based on the container size (or just returns the fixed value):

    return styleLogicalWidth.calcMinValue(availableWidth) + borders;

AutoTableLayout::computePreferredLogicalWidths does not look at percent values, but it does look at fixed values.

> And if the code in "RenderTable::computeLogicalWidth" does not have any specific use case can we remove this.

Which lines are you talking about specifically?

> Here I am assuming that the purpose of the code in RenderTable.cpp is not to fix the "table-layout: fixed" case. Please correct me if I am wrong.

You're right. It's not intended to fix the table-layout: fixed case.

[Antaryami Pandia (apandia)]

(In reply to comment #8)
Thanks for the information. I am just trying to understand the code changes.

> If I'm understanding your question correctly, the code in RenderTable::computeLogicalWidth is necessary to handle percent widths and percent min-widths. Specifically, the following line in RenderTable::convertStyleLogicalWidthToComputedWidth resolves a percent value based on the container size (or just returns the fixed value):
> 
>     return styleLogicalWidth.calcMinValue(availableWidth) + borders;
> 
> AutoTableLayout::computePreferredLogicalWidths does not look at percent values, but it does look at fixed values.

Sorry, I think I have mis-represented my query.The code in "RenderTable::computeLogicalWidth" is necessary.I was just asking about the below code:-

Method :- "RenderTable::computeLogicalWidth"
// Ensure we aren't smaller than our min-width style.
Length styleMinLogicalWidth = style()->logicalMinWidth();
if (styleMinLogicalWidth.isSpecified() && styleMinLogicalWidth.isPositive())
     setLogicalWidth(max(logicalWidth(), ConvertStyleLogicalWidthToComputedWidth(styleMinLogicalWidth, availableLogicalWidth)));
 


> > And if the code in "RenderTable::computeLogicalWidth" does not have any specific use case can we remove this.
> 
> Which lines are you talking about specifically?

Same code as mentioned above. I was just thinking if this is not doing any specific job, then can we remove it place code similar as "AutotableLayout" (which sets the min-width ) in fixedtablelayout.cpp. 
 
> > Here I am assuming that the purpose of the code in RenderTable.cpp is not to fix the "table-layout: fixed" case. Please correct me if I am wrong.
> 
> You're right. It's not intended to fix the table-layout: fixed case.

Thanks a lot for your time.

[Max Vujovic]

(In reply to comment #9)
> Method :- "RenderTable::computeLogicalWidth"
> // Ensure we aren't smaller than our min-width style.
> Length styleMinLogicalWidth = style()->logicalMinWidth();
> if (styleMinLogicalWidth.isSpecified() && styleMinLogicalWidth.isPositive())
>      setLogicalWidth(max(logicalWidth(), ConvertStyleLogicalWidthToComputedWidth(styleMinLogicalWidth, availableLogicalWidth)));
> 
> 
> 
> > > And if the code in "RenderTable::computeLogicalWidth" does not have any specific use case can we remove this.
> > 
> > Which lines are you talking about specifically?
> 
> Same code as mentioned above. I was just thinking if this is not doing any specific job, then can we remove it place code similar as "AutotableLayout" (which sets the min-width ) in fixedtablelayout.cpp. 

That code is necessary for the percent min-width case (e.g. min-width: 50%). The code calls convertStyleLogicalWidthToComputedWidth(styleMinLogicalWidth, availableLogicalWidth), which computes the percent min-width in layout units. If the min-width is a fixed value (e.g. min-width: 400px), it just returns that value.

I think the very first step for this bug is to find a case when min-width on table-layout: fixed fails. This is probably harder than the fix. To do that, I would look at the test cases that fail when we remove similar code for the width style in FixedTableLayout or AutoTableLayout, and if possible adapt those failing test cases to use min-width instead of width and see if an unexpected result occurs.

This is related to what I said before:
"""
However, if you comment out that code in AutoTableLayout::computePreferredLogicalWidths like this:

    if (tableLogicalWidth.isFixed() && tableLogicalWidth.isPositive()) {
        //minWidth = max<int>(minWidth, tableLogicalWidth.value());
        //maxWidth = minWidth;
    } else if (!remainingPercent && maxNonPercent) {

You will see that these tests fail:

  fast/table/028-vertical.html
  fast/table/029.html
  fast/table/auto-100-percent-width.html
  fast/table/colspan-with-all-percent-cells.html 
"""

If we can understand what those test cases are trying to do, perhaps we can craft similar test cases for min-width and fixed table layout to prove that it there is a failure in the first place.

> Thanks a lot for your time.

You're welcome. Thanks for working on this!

[Ahmad Saleem]

Created attachment 460193 [details]
Safari 15.5 matches other browsers

I am not able to reproduce this in Safari 15.5 on macOS 12.4 and as shown in the picture, it matches with other browsers. Further, from Comment 04, it seems that it was fixed in 2012 but there were some discussion around making enhancements to logic in the code. I don't know whether they are relevant or not or they can live as separate bug.

I think this can be closed as "RESOLVED CONFIGURATION CHANGED" (based on test case basis). Thanks!