Bug 86671 – Standalone table-columns should be wrapped in anonymous tables

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Levi Weintraub]

According to the linked spec: 
- A 'table-column' box is misparented if its parent is neither a 'table-column-group' box nor a 'table' or 'inline-table' box.
- A row group box, 'table-column-group' box, or 'table-caption' box is misparented if its parent is neither a 'table' box nor an 'inline-table' box.

In the code, we have the following lines in RenderObject::addChild:

    if (newChild->isTableCol() && newChild->style()->display() == TABLE_COLUMN_GROUP)
        needsTable = !isTable();

This should be the following to cover both points mentioned above (table-column and table-column-group are both RenderTableCols):

    if (newChild->isTableCol() && newChild->style()->display() == TABLE_COLUMN_GROUP)
        needsTable = !isTable();
    else if (newChild->isTableCol())
        needsTable = !isTable() && style()->display() != TABLE_COLUMN_GROUP;

[Levi Weintraub]

Created attachment 142383 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 142383 [details]
Patch

if we remove that column do we correctly kill the anonymous table?

[Levi Weintraub]

(In reply to comment #2)
> (From update of attachment 142383 [details])
> if we remove that column do we correctly kill the anonymous table?

Not yet! I'm landing this so I can include a test like that in the patch for https://bugs.webkit.org/show_bug.cgi?id=86261

[WebKit Review Bot]

Comment on attachment 142383 [details]
Patch

Attachment 142383 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12724038

New failing tests:
fast/forms/form-hides-table.html
fast/table/form-with-table-style.html

[WebKit Review Bot]

Created attachment 142392 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Julien Chaffraix]

Comment on attachment 142383 [details]
Patch

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

You will need to investigate why fast/table/form-with-table-style.html has such a weird change in sizing (a quick glance didn't give me any insight of what's going wrong).

> Source/WebCore/rendering/RenderObject.cpp:279
>      if (newChild->isTableCol() && newChild->style()->display() == TABLE_COLUMN_GROUP)
>          needsTable = !isTable();
> +    else if (newChild->isTableCol())
> +        needsTable = !isTable() && style()->display() != TABLE_COLUMN_GROUP;

I think we could squash those 2 ifs with the following:

if (newChild->isTableCol())
    needsTable = !isTable();

isTableCol() returns true for both TABLE_COLUMN_GROUP and TABLE_COLUMN which is what you need to cover.

> LayoutTests/fast/table/table-column-generates-anonymous-table.html:4
> +

Please, please, please, don't forget to include the following in your test:
* bug id (even better *clickable* link to this bug).
* bug title.

> LayoutTests/fast/table/table-column-generates-anonymous-table.html:6
> +<div style="display:table-column; background-color: green;"></div>
> +<div style="display:table-row;"><div style="display:table-cell;">This box should be green</div></div>

If you don't need to use CSS tables (which is the case here AFAICT), you can just use the html equivalent:
* <table>
* <col> for columns
* <tr> for rows
* <td>, <th> for cells

> LayoutTests/fast/table/table-column-generates-anonymous-table.html:7
> +

Also let's remove the unneeded space.

[Levi Weintraub]

Comment on attachment 142383 [details]
Patch

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

>> Source/WebCore/rendering/RenderObject.cpp:279
>> +        needsTable = !isTable() && style()->display() != TABLE_COLUMN_GROUP;
> 
> I think we could squash those 2 ifs with the following:
> 
> if (newChild->isTableCol())
>     needsTable = !isTable();
> 
> isTableCol() returns true for both TABLE_COLUMN_GROUP and TABLE_COLUMN which is what you need to cover.

Yes, but table-column-group and table-column have different rules. The current set of ifs properly covers the spec:
A 'table-column' box is misparented if its parent is neither a 'table-column-group' box nor a 'table' or 'inline-table' box.
A row group box, 'table-column-group' box, or 'table-caption' box is misparented if its parent is neither a 'table' box nor an 'inline-table' box.
(see http://www.w3.org/TR/CSS21/tables.html)

[Julien Chaffraix]

Comment on attachment 142383 [details]
Patch

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

>>> Source/WebCore/rendering/RenderObject.cpp:279
>>> +        needsTable = !isTable() && style()->display() != TABLE_COLUMN_GROUP;
>> 
>> I think we could squash those 2 ifs with the following:
>> 
>> if (newChild->isTableCol())
>>     needsTable = !isTable();
>> 
>> isTableCol() returns true for both TABLE_COLUMN_GROUP and TABLE_COLUMN which is what you need to cover.
> 
> Yes, but table-column-group and table-column have different rules. The current set of ifs properly covers the spec:
> A 'table-column' box is misparented if its parent is neither a 'table-column-group' box nor a 'table' or 'inline-table' box.
> A row group box, 'table-column-group' box, or 'table-caption' box is misparented if its parent is neither a 'table' box nor an 'inline-table' box.
> (see http://www.w3.org/TR/CSS21/tables.html)

Indeed, the code is fine. I just got confused with the isTableCol() usage here. Removing the isTableCol() checks and just use display() instead would make the code more readable.

[Levi Weintraub]

(In reply to comment #8)
> Indeed, the code is fine. I just got confused with the isTableCol() usage here. Removing the isTableCol() checks and just use display() instead would make the code more readable.

Sounds good.

The fast/table/form-with-table-style.html failure is boggling. I don't see this visual difference in the Mac port. Building a Chromium build with the patch...

[Levi Weintraub]

(In reply to comment #9)
> (In reply to comment #8)
> > Indeed, the code is fine. I just got confused with the isTableCol() usage here. Removing the isTableCol() checks and just use display() instead would make the code more readable.
> 
> Sounds good.
> 
> The fast/table/form-with-table-style.html failure is boggling. I don't see this visual difference in the Mac port. Building a Chromium build with the patch...

Nevermind. The change to fast/table/form-with-table-style is on both platforms and is correct. In the gap that goes away with this patch are two divs. One has a table-column-group, one has a table-column. Previously, the table-column-group was correctly collapsed away, but the table-column, which wasn't properly wrapped in an anonymous table, still incorrectly reserved its line height. Now it, too, goes away.

[Levi Weintraub]

Created attachment 142537 [details]
Patch

[Julien Chaffraix]

Comment on attachment 142537 [details]
Patch

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

r=me with some changes.

> LayoutTests/ChangeLog:11
> +        * fast/table/table-column-generates-anonymous-table-expected.html: Added.
> +        * fast/table/table-column-generates-anonymous-table.html: Added.

As mentioned, I would like a test for the column-group case as I don't think it's properly covered.

> LayoutTests/ChangeLog:17
> +        * platform/chromium-linux/fast/forms/form-hides-table-expected.txt: Added. An anonymous
> +        table is now generated to enclose the misparented table column.
> +        * platform/chromium-linux/fast/table/form-with-table-style-expected.txt: Added.
> +        * platform/chromium-linux/fast/table/form-with-table-style-expected.png: Modified. The
> +        expectations previously were wrong in that the table column occupied height the
> +        equivalent of its line-height. This is correctly no longer the case.

These tests should be skipped on the other platforms or the bots will be sad.

[Levi Weintraub]

Created attachment 142746 [details]
Patch for landing

[Levi Weintraub]

Comment on attachment 142746 [details]
Patch for landing

D'oh. Forgot my skips. Will re-upload momentarily.

[Levi Weintraub]

Created attachment 142749 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 142749 [details]
Patch for landing

Rejecting attachment 142749 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
ueue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 70, in run_and_handle_errors
    self._run(tool, options, state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 50, in run
    if changelog_entry.has_valid_reviewer():
AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer'

Full output: http://queues.webkit.org/results/12715985

[Levi Weintraub]

Comment on attachment 142749 [details]
Patch for landing

Clearing flags on attachment: 142749

Committed r117640: <http://trac.webkit.org/changeset/117640>

[Levi Weintraub]

All reviewed patches have been landed.  Closing bug.