Bug 78027 – CSS 2.1 failure: fixed-table-layout-013 and fixed-table-layout-015 fail

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Robert Hogan]

Col-group width does not influence column width in fixed table layout apparently..

[Robert Hogan]

Created attachment 125920 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 125920 [details]
Patch

Can we just break this block out into a helper function?  It't not immediately obvious to me why moving this code is correct here.

[Julien Chaffraix]

Comment on attachment 125920 [details]
Patch

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

I agree with Darin, this change is good but could be cleaner and less confusing.

> Source/WebCore/rendering/FixedTableLayout.cpp:91
>      Length grpWidth;

This variable is unused now, it should be removed.

> Source/WebCore/rendering/FixedTableLayout.cpp:-129
> -        col->computePreferredLogicalWidths();

I concur with Eric here. You are moving the code after the following block which is:
1) useless as the code you touch don't use |child| and you actually may end up doing more work (if you have column-group)!
2) makes your change look artificially bigger / scarier / different.

> Source/WebCore/rendering/FixedTableLayout.cpp:103
> +        if (col->firstChild())

Would be nice to make this an helper function in RenderTableCol (like isColumnGroup()).

> Source/WebCore/rendering/FixedTableLayout.cpp:106
> +        Length w = col->style()->logicalWidth();

It should be columnStyleLogicalWidth.

> Source/WebCore/rendering/FixedTableLayout.cpp:107
> +        int effWidth = 0;

effectiveColumnLogicalWidth?

> Source/WebCore/rendering/FixedTableLayout.cpp:108
> +        if (w.isFixed() && w.value() > 0)

Interestingly we don't handle percent width here which is a discrepancy with the check below... Sounds like a bug to file or a big FIXME.

[Julien Chaffraix]

Comment on attachment 125920 [details]
Patch

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

>> Source/WebCore/rendering/FixedTableLayout.cpp:-129
>> -        col->computePreferredLogicalWidths();
> 
> I concur with Eric here. You are moving the code after the following block which is:
> 1) useless as the code you touch don't use |child| and you actually may end up doing more work (if you have column-group)!
> 2) makes your change look artificially bigger / scarier / different.

I take this one back after sleeping on it. You need to make sure |child| is progressing in your loop, however it sounds like you could achieve the same result if you used a for loop now, which would make the logic cleared and more bullet proof.

[Robert Hogan]

Created attachment 126147 [details]
Patch

[Robert Hogan]

(In reply to comment #3)
> 
> Interestingly we don't handle percent width here which is a discrepancy with the check below... Sounds like a bug to file or a big FIXME.

I'll take a closer look at this one in another bug. Addressing your comments has made the patch even harder to read unfortunately. :/

[Julien Chaffraix]

Comment on attachment 126147 [details]
Patch

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

r- for the missing |else| as I would like to see the new patch before landing.

> Source/WebCore/rendering/FixedTableLayout.cpp:80
> +static RenderObject* nextCol(RenderObject* child)

Please use a verb in the function name: findNextCol for example is a good name.

As discussed on IRC, this function is equivalent to the previous code block and is more readable. I don't think this area of the code is performance sensitive so the change should be fine.

> Source/WebCore/rendering/FixedTableLayout.cpp:105
> +    for (;child && child->isTableCol(); child = nextCol(child)) {

I think you found a bug in our style checker (filed bug 78238) :-)

There should be a space after the first semi-colon. Also please just move the initialization of |child| in the loop.

> Source/WebCore/rendering/FixedTableLayout.cpp:109
> +        if (col->isTableColGroup())

I don't like using isTableColGroup() here as it sounds like one of the RenderObject::isTable*() functions which are virtual. I would like to see it renamed to isColGroup(), renderColIsColGroup() or equivalent.

> Source/WebCore/rendering/FixedTableLayout.cpp:130
> -                } else {
> -                    if (span < m_table->spanOfEffCol(currentEffectiveColumn)) {
> -                        m_table->splitColumn(currentEffectiveColumn, span);
> -                        nEffCols++;
> -                        m_width.append(Length());
> -                    }
> -                    spanInCurrentEffectiveColumn = m_table->spanOfEffCol(currentEffectiveColumn);
>                  }

This branch has disappeared in your change. It is possible it is unneeded but this is a separate change.

> Source/WebCore/rendering/RenderTableCol.h:46
> +    bool isTableColGroup() { return firstChild() ? true : false; }

return firstChild(); is enough here.

[Robert Hogan]

(In reply to comment #7)
> This branch has disappeared in your change. It is possible it is unneeded but this is a separate change.

No, it's still there. It's just very fragmented in the diff.

r+ ? :)

[Julien Chaffraix]

Comment on attachment 126147 [details]
Patch

> r+ ? :)

Your deadly kawai eyes won me :-)

(and the fact that if I had followed more closely the logic, I would have seen the change did not miss anything).

[Robert Hogan]

Committed r107970: <http://trac.webkit.org/changeset/107970>

[Vsevolod Vlasov]

(In reply to comment #10)
> Committed r107970: <http://trac.webkit.org/changeset/107970>

This patch was rolled out: 
Committed r108237: <http://trac.webkit.org/changeset/108237>

It breaks column widths updates from javascript, (e.g. inspector's network panel).
I am attaching a reduced reproduction.

[Vsevolod Vlasov]

Created attachment 127816 [details]
column-width-problem

Button should change column width from 100 to 50 and back.
It does so in firefox and it did it in chrome before this patch.

[Robert Hogan]

(In reply to comment #12)
> Created an attachment (id=127816) [details]
> column-width-problem
> 
> Button should change column width from 100 to 50 and back.
> It does so in firefox and it did it in chrome before this patch.

Thanks for spotting this and rolling out. Unfortunately, this regression was caused by a dumb mistake in the refactoring that wasn't caught by any layout-tests. So I need to write a few more tests before fixing and re-landing.

[Robert Hogan]

Created attachment 132119 [details]
Patch

[Robert Hogan]

Comment on attachment 132119 [details]
Patch

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

> Source/WebCore/rendering/FixedTableLayout.cpp:110
> +            col->computePreferredLogicalWidths();

This was the culprit. :/

[Julien Chaffraix]

Comment on attachment 132119 [details]
Patch

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

> Source/WebCore/rendering/FixedTableLayout.cpp:105
> +    for (;child && child->isTableCol(); child = nextCol(child)) {

The initialization ("RenderObject* child = m_table->firstChild();") should be moved here too. Also space after ';' please.

> Source/WebCore/rendering/FixedTableLayout.cpp:143
>          col->computePreferredLogicalWidths();

AFAICT this doesn't depend on any of the computation we are doing here so it could just be hoisted before your |if| (around line 109) to avoid another breakage down the line.

[Robert Hogan]

Committed r111118: <http://trac.webkit.org/changeset/111118>