Full Text Bug Listing

Mark Richards

Reported 2007-10-03 12:11:26 PDT

display:inline seems to be ignored for a table when the page is in standards mode but works fine when the page is in quirks mode. The code works the way I expect it when I try this sample in IE 6, IE 7, Firefox 2.0, and Opera. <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"> <html> <head> <meta http-equiv="Content-Type"> <style type="text/css"> #members { text-align:right; } .db { display: inline; } </style> <title>Title</title> </head> <body> <div> <form name="LoginForm" action="#"> <div id="members"> <label>Username</label> <input type="text"> <label>Password</label> <input type="password"> <table class="db" border="0" cellpadding="0" cellspacing="0"><tbody><tr><td style="border-width: 0pt;"><div class="button_border_2"><input value="GO" type="button"></div></td></tr></tbody></table> <input type="checkbox">Remember Me | <a href="#">Forgot Password?</a> </div> </form> </div> </body> </html>

Mark Richards

Comment 1 2007-10-03 12:13:26 PDT

Created attachment 16524 [details] Sample test-case Added a test-case as a downloadable file. The test-case is in standards mode, which shows the table dropping down onto a new line. If the DTD is removed the page goes into quirks mode and all elements are rendered on one line as expected.

Dave Hyatt

Comment 2 2007-10-03 12:16:43 PDT

In quirks mode to match WinIE, we fix up the author's "mistake" and turn display:inline into display:inline-table. In strict mode, we assume the author literally meant "inline", which means you are no longer a table and instead get converted into an inline flow. I'll need to study the rendering to see if we're still rendering as expected when the <table> is no longer a <table>. Note that Firefox and IE do not support display:inline-table.

Dave Hyatt

Comment 3 2007-10-03 12:21:58 PDT

Yeah this is not a bug as far as I can tell. We make a new anonymous table to wrap the <tbody>, and this table will be a block-level table. Thus it will get shoved down. I am keeping this open, however, because of the claim that this works in Firefox and Opera. We need someone to investigate the reduction in those browsers. (I would expect IE to render the same in strict mode, because it's doing the wrong thing in that mode, but I'd be suprised if the other browsers were rendering that way in strict mode also.)

Dave Hyatt

Comment 4 2007-10-03 12:27:38 PDT

Aha. http://www.w3.org/Style/Group/css2-src/tables.html#anonymous-boxes CSS2.1 has amended this so that you look at the parent box. Thus this is a bug that we should fix. We can drop the inline-table quirk then at the same time.

Kang-Hao (Kenny) Lu

Comment 5 2011-11-11 05:14:51 PST

Created attachment 114680 [details] proposed patch A simple try to solve this problem, although I am pretty ignorant about many CSS details so I am not at all confident about this. I thought about it might be better to have a m_tableInternal on RenderObject to avoid the isTableXXX chain but that seems too costly given that this problem shouldn't occur often. I though this blocks bug 47141 but it turns out that those huge amount of table anonymous tests don't have a test related to this! This is my first patch to WebKit, so bear with me.

WebKit Review Bot

Comment 6 2011-11-11 05:17:55 PST

Attachment 114680 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/ChangeLog:4: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:5: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:12: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:18: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:20: Line contains tab character. [whitespace/tab] [5] LayoutTests/ChangeLog:4: Line contains tab character. [whitespace/tab] [5] LayoutTests/ChangeLog:5: Line contains tab character. [whitespace/tab] [5] LayoutTests/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] LayoutTests/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] LayoutTests/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] LayoutTests/ChangeLog:12: Line contains tab character. [whitespace/tab] [5] Total errors found: 14 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Kang-Hao (Kenny) Lu

Comment 7 2011-11-11 05:31:00 PST

Created attachment 114682 [details] proposed patch Remove tabs.

Kang-Hao (Kenny) Lu

Comment 8 2011-11-11 05:33:27 PST

Created attachment 114683 [details] proposed patch

WebKit Review Bot

Comment 9 2011-11-11 19:01:06 PST

Comment on attachment 114683 [details] proposed patch Attachment 114683 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10350698 New failing tests: fast/dynamic/insert-before-table-part-in-continuation.html

Kang-Hao (Kenny) Lu

Comment 10 2011-11-12 07:49:44 PST

Created attachment 114834 [details] proposed patch. Most of it being test removal. Change the if clause a little bit so that for 'table-cell' in 'inline-block' it now follows the spec. I believe insert-before-table-part-in-continuation is the last test related to this but I am not sure...

Julien Chaffraix

Comment 11 2012-01-30 16:32:01 PST

Comment on attachment 114834 [details] proposed patch. Most of it being test removal. View in context: https://bugs.webkit.org/attachment.cgi?id=114834&action=review I would like to see some testing with continuations (which seems to work with your table patch). > Source/WebCore/ChangeLog:17 > + (WebCore::RenderInline::addChildIgnoringContinuation): where "block in inline" is dealt with I wonder what this means. Understand that people do read ChangeLogs so they need to understand them. > Source/WebCore/rendering/RenderInline.cpp:245 > + if (!newChild->isInline() && !newChild->isFloatingOrPositioned() > + && !newChild->isTableCell() && !newChild->isTableRow() > + && !newChild->isTableSection() && !newChild->isTableCol()) { I think this needs some explanations either with a comment or in the ChangeLog. The whole logic works because we will create the needed wrappers and thus recursively call RenderInline::addChild. Also your checks don't strictly match the ones in RenderObject::addChild which makes me think you are missing some cases: * isTableCol() does not guarantee that we have a table wrapper. If this part was tested, we would have seen it. * the table caption case is not handled. > Source/WebCore/rendering/RenderObject.cpp:321 > + if (style()->display() != INLINE) > + newStyle->setDisplay(TABLE); > + else > + newStyle->setDisplay(INLINE_TABLE); Preferably use an inline version: newStyle->setDisplay(style()->display() == INLINE ? INLINE_TABLE : TABLE); > LayoutTests/ChangeLog:8 > + * fast/dynamic/insert-before-table-part-in-continuation-expected.txt: Added. This change is wrong: there are different baselines for different platforms because of subtle differences between them (mostly text related in this case). By providing a unique baseline, you will make the test now fails on all platforms except the one for which you created the baselines. > LayoutTests/ChangeLog:39 > + * platform/chromium-linux/fast/dynamic/insert-before-table-part-in-continuation-expected.png: Removed. > + * platform/chromium-linux/tables/mozilla/bugs/bug3037-1-expected.png: Removed. > + * platform/chromium-mac-leopard/fast/dynamic/insert-before-table-part-in-continuation-expected.png: Removed. > + * platform/chromium-mac-leopard/tables/mozilla/bugs/bug3037-1-expected.png: Removed. > + * platform/chromium-mac/fast/dynamic/insert-before-table-part-in-continuation-expected.png: Removed. > + * platform/chromium-mac/tables/mozilla/bugs/bug3037-1-expected.png: Removed. > + * platform/chromium-win/fast/dynamic/insert-before-table-part-in-continuation-expected.png: Removed. > + * platform/chromium-win/fast/dynamic/insert-before-table-part-in-continuation-expected.txt: Removed. > + * platform/chromium-win/tables/mozilla/bugs/bug3037-1-expected.png: Removed. > + * platform/chromium-win/tables/mozilla/bugs/bug3037-1-expected.txt: Removed. > + * platform/efl/fast/dynamic/insert-before-table-part-in-continuation-expected.png: Removed. > + * platform/efl/fast/dynamic/insert-before-table-part-in-continuation-expected.txt: Removed. > + * platform/efl/tables/mozilla/bugs/bug3037-1-expected.png: Removed. > + * platform/efl/tables/mozilla/bugs/bug3037-1-expected.txt: Removed. > + * platform/gtk/fast/dynamic/insert-before-table-part-in-continuation-expected.png: Removed. > + * platform/gtk/fast/dynamic/insert-before-table-part-in-continuation-expected.txt: Removed. > + * platform/gtk/tables/mozilla/bugs/bug3037-1-expected.png: Removed. > + * platform/gtk/tables/mozilla/bugs/bug3037-1-expected.txt: Removed. > + * platform/mac-leopard/fast/dynamic/insert-before-table-part-in-continuation-expected.png: Removed. > + * platform/mac-leopard/tables/mozilla/bugs/bug3037-1-expected.png: Removed. > + * platform/mac/fast/dynamic/insert-before-table-part-in-continuation-expected.png: Removed. > + * platform/mac/fast/dynamic/insert-before-table-part-in-continuation-expected.txt: Removed. > + * platform/mac/tables/mozilla/bugs/bug3037-1-expected.png: Removed. > + * platform/mac/tables/mozilla/bugs/bug3037-1-expected.txt: Removed. > + * platform/qt-wk2/fast/dynamic/insert-before-table-part-in-continuation-expected.png: Removed. > + * platform/qt/fast/dynamic/insert-before-table-part-in-continuation-expected.png: Removed. > + * platform/qt/fast/dynamic/insert-before-table-part-in-continuation-expected.txt: Removed. > + * platform/qt/tables/mozilla/bugs/bug3037-1-expected.png: Removed. > + * platform/qt/tables/mozilla/bugs/bug3037-1-expected.txt: Removed. Removing all the baselines is not a good idea. We prefer to have contributors rebaseline any platforms they can and mark the others as needing a new baseline in test_expectations.txt for port maintainer. > LayoutTests/fast/table/table-anonymous-inline-table-expected.txt:9 > + RenderTable at (12,0) size 11x18 If you look at the output, the table is shifted 4px toward the top edge. It doesn't match Firefox or Opera in my testing. Looks like it is a bug in WebKit but nowhere do I see mention of that.

Julien Chaffraix

Comment 12 2012-07-17 16:24:16 PDT

Created attachment 152867 [details] Proposed fix 1: Generate the proper display based on the parent's. Also ensure that RenderInline doesn't wrongly wrap any table parts.

WebKit Review Bot

Comment 13 2012-07-17 17:38:24 PDT

Comment on attachment 152867 [details] Proposed fix 1: Generate the proper display based on the parent's. Also ensure that RenderInline doesn't wrongly wrap any table parts. Attachment 152867 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13272308 New failing tests: fast/dynamic/insert-before-table-part-in-continuation.html

WebKit Review Bot

Comment 14 2012-07-17 17:38:28 PDT

Created attachment 152886 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Julien Chaffraix

Comment 15 2012-07-18 11:12:29 PDT

Created attachment 153051 [details] Proposed patch 2. Rebaselined insert-before-table-part-in-continuation.html as it is a progression.

Robert Hogan

Comment 16 2012-07-19 13:00:00 PDT

Comment on attachment 153051 [details] Proposed patch 2. Rebaselined insert-before-table-part-in-continuation.html as it is a progression. View in context: https://bugs.webkit.org/attachment.cgi?id=153051&action=review > Source/WebCore/ChangeLog:9 > + The gist of this bug is that we wouldn't check our parent when generating anonymous table > + wrappers. Maybe reference the spec here, per Gerard in bug 91137. "If a table is contained by an inline element, then the anonymous tables should be inline-tables. Bullet 3 "Generate missing parents" of section 17.2.1 http://www.w3.org/TR/CSS2/tables.html#anonymous-boxes"

Abhishek Arya

Comment 17 2012-07-19 13:09:08 PDT

Comment on attachment 153051 [details] Proposed patch 2. Rebaselined insert-before-table-part-in-continuation.html as it is a progression. View in context: https://bugs.webkit.org/attachment.cgi?id=153051&action=review > Source/WebCore/ChangeLog:25 > + intended as we will wrap the new block child into an inline table per the check above. Please add a line or two on how it does not impact the case for normal table. As i understand it, it is because RenderObject::addChild will create the anonymous block table and then we will go inside the check. > Source/WebCore/rendering/RenderInline.cpp:306 > + if (!newChild->isInline() && !newChild->isFloatingOrOutOfFlowPositioned() && !newChild->isTablePart()) { clever trick. > Source/WebCore/rendering/RenderTable.cpp:1297 > + RefPtr<RenderStyle> newStyle = RenderStyle::createAnonymousStyleWithDisplay(parent->style(), parent->isInline() ? INLINE_TABLE : TABLE); You can split the display to its own line and link the spec to it in a comment. > LayoutTests/platform/chromium-win/fast/dynamic/insert-before-table-part-in-continuation-expected.txt:-1 > -layer at (0,0) size 785x748 can you check the test again with Firefox and Opera. There are still some differences and I am curious what is causing that.

Julien Chaffraix

Comment 18 2012-07-19 14:45:59 PDT

Comment on attachment 153051 [details] Proposed patch 2. Rebaselined insert-before-table-part-in-continuation.html as it is a progression. View in context: https://bugs.webkit.org/attachment.cgi?id=153051&action=review >> Source/WebCore/ChangeLog:9 >> + wrappers. > > Maybe reference the spec here, per Gerard in bug 91137. "If a table is contained by an inline element, then the anonymous tables should be inline-tables. Bullet 3 "Generate missing parents" of section 17.2.1 http://www.w3.org/TR/CSS2/tables.html#anonymous-boxes" Sure. >> Source/WebCore/ChangeLog:25 >> + intended as we will wrap the new block child into an inline table per the check above. > > Please add a line or two on how it does not impact the case for normal table. As i understand it, it is because RenderObject::addChild will create the anonymous block table and then we will go inside the check. Changed the last sentence to say: "This change works as intended as we will call RenderObject::addChild which will create an anonymous inline table that will be added under |this| instead of the table part. As the table is inline, it doesn't need to be wrapped when we recursively call RenderInline::addChild." The table will be inline per our change so it won't impact the block table case. >> Source/WebCore/rendering/RenderInline.cpp:306 >> + if (!newChild->isInline() && !newChild->isFloatingOrOutOfFlowPositioned() && !newChild->isTablePart()) { > > clever trick. Not mine, this is Kang-Hao's (that I wrongly commented against, my bad) >> Source/WebCore/rendering/RenderTable.cpp:1297 >> + RefPtr<RenderStyle> newStyle = RenderStyle::createAnonymousStyleWithDisplay(parent->style(), parent->isInline() ? INLINE_TABLE : TABLE); > > You can split the display to its own line and link the spec to it in a comment. Will be done. >> LayoutTests/platform/chromium-win/fast/dynamic/insert-before-table-part-in-continuation-expected.txt:-1 >> -layer at (0,0) size 785x748 > > can you check the test again with Firefox and Opera. There are still some differences and I am curious what is causing that. I acknowledged those differences in the ChangeLog but didn't spend a lot of time digging into them. I will file a follow-up bug to track them.

Julien Chaffraix

Comment 19 2012-07-19 15:03:26 PDT

Created attachment 153358 [details] Patch for landing

WebKit Review Bot

Comment 20 2012-07-19 16:11:03 PDT

Comment on attachment 153358 [details] Patch for landing Clearing flags on attachment: 153358 Committed r123159: <http://trac.webkit.org/changeset/123159>

WebKit Review Bot

Comment 21 2012-07-19 16:11:10 PDT

All reviewed patches have been landed. Closing bug.

Alexey Proskuryakov

Comment 22 2012-07-23 09:40:55 PDT

This caused bug 91929.

WebKit Review Bot

Comment 23 2012-07-26 11:27:15 PDT

Re-opened since this is blocked by 92406

Julien Chaffraix

Comment 24 2012-07-26 11:39:25 PDT

For the record, just enabling the inline-table logic is a bad idea. We have had numerous issues (both functional and security-related) with anonymous tables and table parts in RenderBlock and slowly build helper functions to properly handle all the cases. RenderInline has none of those facilities and toggling the switch means that we don't handle any of them. Before considering this patch for landing, the complex cases should be handled or we will end up rolling the patch again: * splitting an anonymous table in two. * mixed anonymous / non-anonymous table parts wrappers inside a RenderInline. * :after / :before content in an anonymous inline-table in a RenderInline that can do all of the above.

Attachments
Sample test-case (886 bytes, text/plain) 2007-10-03 12:13 PDT, Mark Richards	no flags	Details
proposed patch (22.77 KB, patch) 2011-11-11 05:14 PST, Kang-Hao (Kenny) Lu	no flags	Details Formatted Diff Diff
proposed patch (22.81 KB, patch) 2011-11-11 05:31 PST, Kang-Hao (Kenny) Lu	no flags	Details Formatted Diff Diff
proposed patch (22.81 KB, patch) 2011-11-11 05:33 PST, Kang-Hao (Kenny) Lu	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
proposed patch. Most of it being test removal. (113.04 KB, patch) 2011-11-12 07:49 PST, Kang-Hao (Kenny) Lu	no flags	Details Formatted Diff Diff
Proposed fix 1: Generate the proper display based on the parent's. Also ensure that RenderInline doesn't wrongly wrap any table parts. (12.65 KB, patch) 2012-07-17 16:24 PDT, Julien Chaffraix	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-08 (394.61 KB, application/zip) 2012-07-17 17:38 PDT, WebKit Review Bot	no flags	Details
Proposed patch 2. Rebaselined insert-before-table-part-in-continuation.html as it is a progression. (108.01 KB, patch) 2012-07-18 11:12 PDT, Julien Chaffraix	no flags	Details Formatted Diff Diff
Patch for landing (108.56 KB, patch) 2012-07-19 15:03 PDT, Julien Chaffraix	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Bug 15365