Bug 15365 - [CSS2.1] Anonymous tables should be inline/block-level based off their parent
: [CSS2.1] Anonymous tables should be inline/block-level based off their parent
Status: REOPENED
: WebKit
Tables
: 523.x (Safari 3)
: PC Windows XP
: P2 Normal
Assigned To:
:
:
: 92406
: 47141 91137
  Show dependency treegraph
 
Reported: 2007-10-03 12:11 PST by
Modified: 2012-07-26 11:39 PST (History)


Attachments
Sample test-case (886 bytes, text/plain)
2007-10-03 12:13 PST, Mark Richards
no flags Details
proposed patch (22.77 KB, patch)
2011-11-11 05:14 PST, Kang-Hao (Kenny) Lu
no flags Review Patch | Details | Formatted Diff | Diff
proposed patch (22.81 KB, patch)
2011-11-11 05:31 PST, Kang-Hao (Kenny) Lu
no flags Review Patch | 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-
Review Patch | 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 Review Patch | 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 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (394.61 KB, application/zip)
2012-07-17 17:38 PST, 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 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (108.56 KB, patch)
2012-07-19 15:03 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2007-10-03 12:11:26 PST
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 From 2007-10-03 12:13:26 PST -------
Created an attachment (id=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 From 2007-10-03 12:16:43 PST -------
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 From 2007-10-03 12:21:58 PST -------
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 From 2007-10-03 12:27:38 PST -------
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 From 2011-11-11 05:14:51 PST -------
Created an attachment (id=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 From 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 From 2011-11-11 05:31:00 PST -------
Created an attachment (id=114682) [details]
proposed patch

Remove tabs.
------- Comment #8 From 2011-11-11 05:33:27 PST -------
Created an attachment (id=114683) [details]
proposed patch
------- Comment #9 From 2011-11-11 19:01:06 PST -------
(From update of attachment 114683 [details])
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 From 2011-11-12 07:49:44 PST -------
Created an attachment (id=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 From 2012-01-30 16:32:01 PST -------
(From update of attachment 114834 [details])
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 From 2012-07-17 16:24:16 PST -------
Created an attachment (id=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 From 2012-07-17 17:38:24 PST -------
(From update of attachment 152867 [details])
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 From 2012-07-17 17:38:28 PST -------
Created an attachment (id=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 From 2012-07-18 11:12:29 PST -------
Created an attachment (id=153051) [details]
Proposed patch 2. Rebaselined insert-before-table-part-in-continuation.html as it is a progression.
------- Comment #16 From 2012-07-19 13:00:00 PST -------
(From update of attachment 153051 [details])
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 From 2012-07-19 13:09:08 PST -------
(From update of attachment 153051 [details])
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 From 2012-07-19 14:45:59 PST -------
(From update of attachment 153051 [details])
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 From 2012-07-19 15:03:26 PST -------
Created an attachment (id=153358) [details]
Patch for landing
------- Comment #20 From 2012-07-19 16:11:03 PST -------
(From update of attachment 153358 [details])
Clearing flags on attachment: 153358

Committed r123159: <http://trac.webkit.org/changeset/123159>
------- Comment #21 From 2012-07-19 16:11:10 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #22 From 2012-07-23 09:40:55 PST -------
This caused bug 91929.
------- Comment #23 From 2012-07-26 11:27:15 PST -------
Re-opened since this is blocked by 92406
------- Comment #24 From 2012-07-26 11:39:25 PST -------
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.