Summary: | [CSS2.1] Anonymous tables should be inline/block-level based off their parent | ||
---|---|---|---|
Product: | WebKit | Reporter: | Mark Richards <markr> |
Component: | Tables | Assignee: | Dave Hyatt <hyatt> |
Status: | REOPENED --- | ||
Severity: | Normal | CC: | bdakin, dglazkov, eric, hyatt, jchaffraix, kennyluck, robert, webkit.review.bot |
Priority: | P2 | ||
Version: | 523.x (Safari 3) | ||
Hardware: | PC | ||
OS: | Windows XP | ||
Bug Depends on: | 92406 | ||
Bug Blocks: | 47141, 91137 | ||
Attachments: |
Description
Mark Richards
2007-10-03 12:11: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.
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. 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.) 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. 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. 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.
Created attachment 114682 [details]
proposed patch
Remove tabs.
Created attachment 114683 [details]
proposed patch
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 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...
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. 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.
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 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
Created attachment 153051 [details]
Proposed patch 2. Rebaselined insert-before-table-part-in-continuation.html as it is a progression.
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" 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. 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. Created attachment 153358 [details]
Patch for landing
Comment on attachment 153358 [details] Patch for landing Clearing flags on attachment: 153358 Committed r123159: <http://trac.webkit.org/changeset/123159> All reviewed patches have been landed. Closing bug. This caused bug 91929. Re-opened since this is blocked by 92406 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. |