| 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: | P2 | ||||||||||||||
| 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: |
|
||||||||||||||
Created attachment 142383 [details]
Patch
Comment on attachment 142383 [details]
Patch
if we remove that column do we correctly kill the anonymous table?
(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 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 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
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. 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) 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. (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... (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. Created attachment 142537 [details]
Patch
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. Created attachment 142746 [details]
Patch for landing
Comment on attachment 142746 [details]
Patch for landing
D'oh. Forgot my skips. Will re-upload momentarily.
Created attachment 142749 [details]
Patch for landing
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 Comment on attachment 142749 [details] Patch for landing Clearing flags on attachment: 142749 Committed r117640: <http://trac.webkit.org/changeset/117640> All reviewed patches have been landed. Closing bug. |
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;