Bug 15365

Summary: [CSS2.1] Anonymous tables should be inline/block-level based off their parent
Product: WebKit Reporter: Mark Richards <markr>
Component: TablesAssignee: 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 Flags
Sample test-case
none
proposed patch
none
proposed patch
none
proposed patch
webkit.review.bot: commit-queue-
proposed patch. Most of it being test removal.
none
Proposed fix 1: Generate the proper display based on the parent's. Also ensure that RenderInline doesn't wrongly wrap any table parts.
none
Archive of layout-test-results from gce-cr-linux-08
none
Proposed patch 2. Rebaselined insert-before-table-part-in-continuation.html as it is a progression.
none
Patch for landing none

Description Mark Richards 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>
Comment 1 Mark Richards 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.
Comment 2 Dave Hyatt 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.

Comment 3 Dave Hyatt 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.)


Comment 4 Dave Hyatt 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.


Comment 5 Kang-Hao (Kenny) Lu 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.
Comment 6 WebKit Review Bot 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.
Comment 7 Kang-Hao (Kenny) Lu 2011-11-11 05:31:00 PST
Created attachment 114682 [details]
proposed patch

Remove tabs.
Comment 8 Kang-Hao (Kenny) Lu 2011-11-11 05:33:27 PST
Created attachment 114683 [details]
proposed patch
Comment 9 WebKit Review Bot 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
Comment 10 Kang-Hao (Kenny) Lu 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...
Comment 11 Julien Chaffraix 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.
Comment 12 Julien Chaffraix 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.
Comment 13 WebKit Review Bot 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
Comment 14 WebKit Review Bot 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
Comment 15 Julien Chaffraix 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.
Comment 16 Robert Hogan 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"
Comment 17 Abhishek Arya 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.
Comment 18 Julien Chaffraix 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.
Comment 19 Julien Chaffraix 2012-07-19 15:03:26 PDT
Created attachment 153358 [details]
Patch for landing
Comment 20 WebKit Review Bot 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>
Comment 21 WebKit Review Bot 2012-07-19 16:11:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Alexey Proskuryakov 2012-07-23 09:40:55 PDT
This caused bug 91929.
Comment 23 WebKit Review Bot 2012-07-26 11:27:15 PDT
Re-opened since this is blocked by 92406
Comment 24 Julien Chaffraix 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.