Bug 78027 - CSS 2.1 failure: fixed-table-layout-013 and fixed-table-layout-015 fail
Summary: CSS 2.1 failure: fixed-table-layout-013 and fixed-table-layout-015 fail
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords:
Depends on:
Blocks: 47141
  Show dependency treegraph
 
Reported: 2012-02-07 13:20 PST by Robert Hogan
Modified: 2012-03-17 03:49 PDT (History)
3 users (show)

See Also:


Attachments
Patch (12.37 KB, patch)
2012-02-07 13:58 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (14.98 KB, patch)
2012-02-08 13:56 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
column-width-problem (1.25 KB, text/html)
2012-02-20 06:06 PST, Vsevolod Vlasov
no flags Details
Patch (18.24 KB, patch)
2012-03-15 14:23 PDT, Robert Hogan
jchaffraix: review+
jchaffraix: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2012-02-07 13:20:47 PST
Col-group width does not influence column width in fixed table layout apparently..
Comment 1 Robert Hogan 2012-02-07 13:58:39 PST
Created attachment 125920 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-02-07 15:43:58 PST
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.
Comment 3 Julien Chaffraix 2012-02-07 20:14:49 PST
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.
Comment 4 Julien Chaffraix 2012-02-08 06:43:48 PST
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.
Comment 5 Robert Hogan 2012-02-08 13:56:00 PST
Created attachment 126147 [details]
Patch
Comment 6 Robert Hogan 2012-02-08 14:03:50 PST
(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. :/
Comment 7 Julien Chaffraix 2012-02-09 07:45:40 PST
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.
Comment 8 Robert Hogan 2012-02-09 11:02:17 PST
(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+ ? :)
Comment 9 Julien Chaffraix 2012-02-09 13:30:38 PST
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).
Comment 10 Robert Hogan 2012-02-16 12:40:58 PST
Committed r107970: <http://trac.webkit.org/changeset/107970>
Comment 11 Vsevolod Vlasov 2012-02-20 06:04:33 PST
(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.
Comment 12 Vsevolod Vlasov 2012-02-20 06:06:18 PST
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.
Comment 13 Robert Hogan 2012-02-20 11:12:19 PST
(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.
Comment 14 Robert Hogan 2012-03-15 14:23:43 PDT
Created attachment 132119 [details]
Patch
Comment 15 Robert Hogan 2012-03-15 14:27:17 PDT
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. :/
Comment 16 Julien Chaffraix 2012-03-15 15:43:38 PDT
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.
Comment 17 Robert Hogan 2012-03-17 03:49:23 PDT
Committed r111118: <http://trac.webkit.org/changeset/111118>