min-width on a display:table-cell element takes precedence over a width on an outer display:table;table-layout:fixed; element if the outer width is set after initial render

[Phil]

In the following test case, the outer and inner elements should end up both being 100px wide, because of the width that is set on the outer element, however, because the width on the outer element is set after initial render, the min-width on the inner element wins and both inner and outer elements end up being 200px wide.

<style>
    #outer {
        display: table;
        table-layout: fixed;
    }
    #inner {
        display: table-cell;
        height: 50px;
        background-color: green;
        min-width: 200px;
    }
</style>

<div id="outer">
    <div id="inner"></div>
</div>

<script>
    window.onload = function() {
        outer = document.getElementById('outer');
        outer.style.width = '100px';
    };
</script>

The following actions can be taken to work around the bug:
1. set the outer element's width in the stylesheet, or inline style, instead of setting the width after initial render
2. after setting the width on the outer element, hiding and showing it by setting display to "none" and then back to "table" will cause it to refresh and layout properly.

This bug is reproducible in the latest webkit nightly (March 7, 2014) and in the latest versions of Safari and Chrome.

[Gurpreet]

Created attachment 230978 [details]
Patch

[Gurpreet]

Created attachment 230979 [details]
Patch

[Gurpreet]

Please review. Thanks.

[Phil]

Here's an additional twist on what appears to be the same issue.  This time there is no min-width involved, just  a table/table-cell combo with table-layout:fixed and long non-wrapping text inside, but the symptom is the same - setting the width dynamically does not update the width of the outer element.  It always expands to include the width of the text, unless a refresh is triggered by toggling some style (display works) on a timer.


<style>
    #table {
        display: table;
        table-layout: fixed;
    }
    #cell {
        display: table-cell;
        white-space: nowrap;
        overflow: hidden;
    }
</style>

<div id="table">
    <div id="cell">Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.</div>
</div>

<script>
    window.onload = function() {
        table = document.getElementById('table');

        table.style.width = '50px';

        // workaround:
//        table.style.display = 'none';
//        setTimeout(function() {
//            table.style.display = '';
//        });
    };
</script>

Hi Phil. The latest content which you mentioned is also working with this patch right?

[Gurpreet]

Hi Simon/Darin please review.

[Jarred Nicholls]

Comment on attachment 230979 [details]
Patch

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

> Source/WebCore/rendering/RenderTable.cpp:105
> +    bool fixedTableLayout = oldStyle ? oldStyle->isFixedTableLayout() : false;

May as well move this variable down to be above the if() that uses it.

[Gurpreet]

(In reply to comment #7)
> (From update of attachment 230979 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230979&action=review
> 
> > Source/WebCore/rendering/RenderTable.cpp:105
> > +    bool fixedTableLayout = oldStyle ? oldStyle->isFixedTableLayout() : false;
> 
> May as well move this variable down to be above the if() that uses it.

Is this required? Earlier also the variable was defined at the same place.

[Gurpreet]

Hi. Its been more than and no one reviewed this. Please review. Its already landed on blink https://codereview.chromium.org/264283002.

[Gurpreet]

Hello. Please review.

[Darin Adler]

Hyatt, could you take a look? Do you agree this is correct and the correct approach?

[Michael Catanzaro]

Comment on attachment 230979 [details]
Patch

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.

[fred]

This issue still exists.
What must we do to get it addressed?

[Darin Adler]

Alan, Simon, can one of you look over this patch?

[EWS]

Committed 260143@main (b6c71868cecc): <https://commits.webkit.org/260143@main>

Reviewed commits have been landed. Closing PR #9924 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/105310280>