Bug 86671

Summary:

Standalone table-columns should be wrapped in anonymous tables

Product:

WebKit

Reporter:

Levi Weintraub <leviw>

Component:

Layout and Rendering

Assignee:

Levi Weintraub <leviw>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, eric, jchaffraix, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

URL:

http://www.w3.org/TR/CSS2/tables.html#anonymous-boxes

Bug Depends on:

Bug Blocks:

86261

Attachments:

Description	Flags
Patch	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03	none
Patch	none
Patch for landing	none
Patch for landing	none

Levi Weintraub

Reported 2012-05-16 13:38:20 PDT

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;

Attachments
Patch (3.45 KB, patch) 2012-05-16 18:15 PDT, Levi Weintraub	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-03 (567.69 KB, application/zip) 2012-05-16 19:17 PDT, WebKit Review Bot	no flags	Details
Patch (60.79 KB, patch) 2012-05-17 13:27 PDT, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch for landing (62.98 KB, patch) 2012-05-18 11:25 PDT, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch for landing (64.04 KB, patch) 2012-05-18 11:34 PDT, Levi Weintraub	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Levi Weintraub

Comment 1 2012-05-16 18:15:54 PDT

Created attachment 142383 [details] Patch

Eric Seidel (no email)

Comment 2 2012-05-16 18:17:03 PDT

Comment on attachment 142383 [details] Patch if we remove that column do we correctly kill the anonymous table?

Levi Weintraub

Comment 3 2012-05-16 18:18:16 PDT

(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 4 2012-05-16 19:17:51 PDT

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

Comment 5 2012-05-16 19:17:55 PDT

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 6 2012-05-17 10:33:07 PDT

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 7 2012-05-17 10:36:34 PDT

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 8 2012-05-17 10:47:45 PDT

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

Comment 9 2012-05-17 11:11:54 PDT

(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

Comment 10 2012-05-17 11:53:25 PDT

(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

Comment 11 2012-05-17 13:27:23 PDT

Created attachment 142537 [details] Patch

Julien Chaffraix

Comment 12 2012-05-17 14:03:59 PDT

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

Comment 13 2012-05-18 11:25:41 PDT

Created attachment 142746 [details] Patch for landing

Levi Weintraub

Comment 14 2012-05-18 11:27:37 PDT

Comment on attachment 142746 [details] Patch for landing D'oh. Forgot my skips. Will re-upload momentarily.

Levi Weintraub

Comment 15 2012-05-18 11:34:01 PDT

Created attachment 142749 [details] Patch for landing

WebKit Review Bot

Comment 16 2012-05-18 14:02:41 PDT

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 17 2012-05-18 15:08:54 PDT

Comment on attachment 142749 [details] Patch for landing Clearing flags on attachment: 142749 Committed r117640: <http://trac.webkit.org/changeset/117640>

Levi Weintraub

Comment 18 2012-05-18 15:08:59 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.