Tables with display:inline rendered in wrong position.

Tables with display:inline rendered as block element.

Table was created as inline renderer and another table block renderer wrapper is
created for table's content. So it was displayed as block element and all its style
properties also associated with inline renderer and was not applied to table.

Creating table inline-table renderer wrapper for table's content if parent
is inline except inline-block parent.

R=eseidel@chromium.org
BUG=53693
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157368

[esprehn, 2013-08-28 18:17:54]

https://codereview.chromium.org/23628004/diff/1/Source/core/rendering/RenderO...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/23628004/diff/1/Source/core/rendering/RenderO...
Source/core/rendering/RenderObject.cpp:170: return new RenderTable(element);
This does not appear correct, if you make a table display: inline why shouldn't
it be inline?

This will break things like:

<div>Hello <table><tr><td>World</td></tr></table> </div>

table, tr, td { display: inline; }

[suchit.agrawal, 2013-08-29 04:31:09]

https://codereview.chromium.org/23628004/diff/1/Source/core/rendering/RenderO...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/23628004/diff/1/Source/core/rendering/RenderO...
Source/core/rendering/RenderObject.cpp:170: return new RenderTable(element);
On 2013/08/28 18:17:54, esprehn wrote:
> This does not appear correct, if you make a table display: inline why
shouldn't
> it be inline?
> 
> This will break things like:
> 
> <div>Hello <table><tr><td>World</td></tr></table> </div>
> 
> table, tr, td { display: inline; }

I have tried this example and check the behavior.
You are right, now it's behavior is changed but it is matching with Firfox.

[suchit.agrawal, 2013-08-29 05:49:05]

https://codereview.chromium.org/23628004/diff/1/Source/core/rendering/RenderO...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/23628004/diff/1/Source/core/rendering/RenderO...
Source/core/rendering/RenderObject.cpp:170: return new RenderTable(element);
On 2013/08/29 04:31:09, suchit.agrawal wrote:
> On 2013/08/28 18:17:54, esprehn wrote:
> > This does not appear correct, if you make a table display: inline why
> shouldn't
> > it be inline?
> > 
> > This will break things like:
> > 
> > <div>Hello <table><tr><td>World</td></tr></table> </div>
> > 
> > table, tr, td { display: inline; }
> 
> I have tried this example and check the behavior.
> You are right, now it's behavior is changed but it is matching with Firfox.

Our previous behavior of this example was wrong. 'World' was rendered in the
next line. But It should be in same line of 'Hello'.
After this patch, Behavior is correct.

[esprehn, 2013-08-29 19:36:28]

On 2013/08/29 05:49:05, suchit.agrawal wrote:
>
https://codereview.chromium.org/23628004/diff/1/Source/core/rendering/RenderO...
> File Source/core/rendering/RenderObject.cpp (right):
> 
>
https://codereview.chromium.org/23628004/diff/1/Source/core/rendering/RenderO...
> Source/core/rendering/RenderObject.cpp:170: return new RenderTable(element);
> On 2013/08/29 04:31:09, suchit.agrawal wrote:
> > On 2013/08/28 18:17:54, esprehn wrote:
> > > This does not appear correct, if you make a table display: inline why
> > shouldn't
> > > it be inline?
> > > 
> > > This will break things like:
> > > 
> > > <div>Hello <table><tr><td>World</td></tr></table> </div>
> > > 
> > > table, tr, td { display: inline; }
> > 
> > I have tried this example and check the behavior.
> > You are right, now it's behavior is changed but it is matching with Firfox.
> 
> Our previous behavior of this example was wrong. 'World' was rendered in the
> next line. But It should be in same line of 'Hello'.
> After this patch, Behavior is correct.

This is not the right fix. If I made the table display inline it should be an
inline not a table (Firefox does this too). What you're observing is the parser
inserting a <tbody> which produces an anonymous table around it. If you change
the test case to have <style>table, tbody, tr, td { display: inline; }</style>
then it renders like Firefox. I think the bug to fix is that a <tbody> (or
thead, tfood) should not wrap itself in an anonymous table.

I also just verified in Gecko that they don't force table's that are display:
inline into table mode like you're doing here.

not lgtm

[a.suchit, 2013-08-30 16:21:31]

On 2013/08/29 19:36:28, esprehn wrote:
> On 2013/08/29 05:49:05, suchit.agrawal wrote:
> >
>
https://codereview.chromium.org/23628004/diff/1/Source/core/rendering/RenderO...
> > File Source/core/rendering/RenderObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/23628004/diff/1/Source/core/rendering/RenderO...
> > Source/core/rendering/RenderObject.cpp:170: return new RenderTable(element);
> > On 2013/08/29 04:31:09, suchit.agrawal wrote:
> > > On 2013/08/28 18:17:54, esprehn wrote:
> > > > This does not appear correct, if you make a table display: inline why
> > > shouldn't
> > > > it be inline?
> > > > 
> > > > This will break things like:
> > > > 
> > > > <div>Hello <table><tr><td>World</td></tr></table> </div>
> > > > 
> > > > table, tr, td { display: inline; }
> > > 
> > > I have tried this example and check the behavior.
> > > You are right, now it's behavior is changed but it is matching with
Firfox.
> > 
> > Our previous behavior of this example was wrong. 'World' was rendered in the
> > next line. But It should be in same line of 'Hello'.
> > After this patch, Behavior is correct.
> 
> This is not the right fix. If I made the table display inline it should be an
> inline not a table (Firefox does this too). What you're observing is the
parser
> inserting a <tbody> which produces an anonymous table around it. If you change
> the test case to have <style>table, tbody, tr, td { display: inline; }</style>
> then it renders like Firefox. I think the bug to fix is that a <tbody> (or
> thead, tfood) should not wrap itself in an anonymous table.
> 
> I also just verified in Gecko that they don't force table's that are display:
> inline into table mode like you're doing here.
> 
> not lgtm

As you suggested me on IRC, I looked for addChild.
RenderInline::addChildIgnoringContinuation always create a block anonymous
object.
Here, I can check for table tag and add INLINE_TABLE style and also create
inline anonymous object.
But problem is splitFlow handle only block renderer.
I need to go in deep for it.

Am I going in right direction?

[a.suchit, 2013-09-02 14:44:18]

I have updated the patch as you suggested me on IRC.

Now, I am creating a inline anonymous table wrapper for table section while
render tree construction.

[esprehn, 2013-09-04 07:19:42]

Why do you only check table elements? That doesn't seem correct (and isn't per
the spec). You can add a test that will fail with your patch:

<!DOCTYPE html>

<table>
    <tbody id="y">
        <tr><td>inside table</td></tr>
    </tbody>
</table>

<span>
before
  <div id="x"></div>
after
<span>

<script>
    var x = document.getElementById('x');
    x.parentNode.replaceChild(document.getElementById('y'), x);
</script>

https://codereview.chromium.org/23628004/diff/26001/Source/core/rendering/Ren...
File Source/core/rendering/RenderInline.cpp (right):

https://codereview.chromium.org/23628004/diff/26001/Source/core/rendering/Ren...
Source/core/rendering/RenderInline.cpp:305: if (!newChild->isInline() &&
!newChild->isTablePart() && !newChild->isFloatingOrOutOfFlowPositioned()) {
Why is this change correct for continuations?

https://codereview.chromium.org/23628004/diff/26001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/23628004/diff/26001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:278: return node() &&
node()->hasTagName(tableTag);
You shouldn't be checking tag names, you should be checking renderers. When you
append a table part to a RenderInline we should wrap it in an inline table. It
shouldn't have anything to do with tag names.

https://codereview.chromium.org/23628004/diff/26001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTable.cpp (right):

https://codereview.chromium.org/23628004/diff/26001/Source/core/rendering/Ren...
Source/core/rendering/RenderTable.cpp:1426: if (parent->isInline() &&
parent->isTableTag())
I don't think you should be checking tag names. Why don't you just check
isInline() ?

[a.suchit, 2013-09-04 13:15:01]

https://codereview.chromium.org/23628004/diff/26001/Source/core/rendering/Ren...
File Source/core/rendering/RenderInline.cpp (right):

https://codereview.chromium.org/23628004/diff/26001/Source/core/rendering/Ren...
Source/core/rendering/RenderInline.cpp:305: if (!newChild->isInline() &&
!newChild->isTablePart() && !newChild->isFloatingOrOutOfFlowPositioned()) {
On 2013/09/04 07:19:42, esprehn wrote:
> Why is this change correct for continuations?

Table parts should wrap with anonymous block or inline element which is based on
it's parent and It should be child of it's parent so that parent's style
property would be applicable on it.

https://codereview.chromium.org/23628004/diff/26001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/23628004/diff/26001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:278: return node() &&
node()->hasTagName(tableTag);
On 2013/09/04 07:19:42, esprehn wrote:
> You shouldn't be checking tag names, you should be checking renderers. When
you
> append a table part to a RenderInline we should wrap it in an inline table. It
> shouldn't have anything to do with tag names.

Done.

https://codereview.chromium.org/23628004/diff/26001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTable.cpp (right):

https://codereview.chromium.org/23628004/diff/26001/Source/core/rendering/Ren...
Source/core/rendering/RenderTable.cpp:1426: if (parent->isInline() &&
parent->isTableTag())
On 2013/09/04 07:19:42, esprehn wrote:
> I don't think you should be checking tag names. Why don't you just check
> isInline() ?

Right. isTableTag() check is not required here.

https://codereview.chromium.org/23628004/diff/26001/Source/core/rendering/Ren...
Source/core/rendering/RenderTable.cpp:1426: if (parent->isInline() &&
parent->isTableTag())
On 2013/09/04 07:19:42, esprehn wrote:
> I don't think you should be checking tag names. Why don't you just check
> isInline() ?

Done.

[eseidel, 2013-09-04 17:21:21]

Looks reasonable.  YOu need to wait for elliot to l-g-t-m however.

[Julien - ping for review, 2013-09-04 21:08:37]

The first line of the description doesn't tell me anything useful (What does
"wrong position" even mean? What's the UNDERLYING PROBLEM?). This is a CSS 2.1
compatibility change and is basically:
https://bugs.webkit.org/show_bug.cgi?id=15365

As the changes are identical and the code hasn't changed much since the previous
bug was landed / reverted, if we let this land, it *will* be reverted. IIRC the
reason was that the code assumes that anonymous + isTable() == table (not
inline-table).

I am not against relanding that change, just be mindful that it looks simple but
is not :)

[esprehn, 2013-09-06 04:13:45]

LGTM, while I believe Julien that this is will be reverted there's no test cases
attached to the original bug and I don't have access to the security bug that
was filed and mentioned in the last comment. We should land this and see what
ClusterFuzz does with it unless someone wants to make the test cases off that
bug public. :)

Note that you're going to need to use TestExpectations to rebaseline that test
since the cq can't land binary files like the png.

[commit-bot: I haz the power, 2013-09-06 05:32:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.suchit@samsung.com/23628004/34001

[commit-bot: I haz the power, 2013-09-06 05:32:15]

Can't process patch for file
LayoutTests/platform/linux/fast/table/table-display-inline-expected.png.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[commit-bot: I haz the power, 2013-09-06 06:06:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.suchit@samsung.com/23628004/49001

[commit-bot: I haz the power, 2013-09-06 07:10:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.suchit@samsung.com/23628004/63001

[commit-bot: I haz the power, 2013-09-06 11:03:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.suchit@samsung.com/23628004/77001

[commit-bot: I haz the power, 2013-09-06 12:32:29]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1286.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
f30018c0762684bed9722fc83e47dd12405d930c..14ddf18f15452519a6c2c3e88301cf44b9f43905
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1286,3 +1286,12 @@ crbug.com/276769
http/tests/security/contentSecurityPolicy/source-list-parsing-n
 
 crbug.com/286081 inspector/debugger/long-resume-vs-reload.html [ Failure Pass
Timeout ]
 crbug.com/285703 [ Debug ]
fast/table/table-rowspan-crash-with-huge-rowspan-cells.html [ Crash Pass ]
+
+#Below test case need to rebaseline after crbug.com/53693
+# Image only
+crbug.com/53693 [ Linux ] fast/table/table-display-inline.html [
NeedsRebaseline ]
+crbug.com/53693 fast/block/float/float-not-removed-from-pre-block.html [
NeedsRebaseline ]
+# Text and Image both
+crbug.com/53693 [ Win Mac ] fast/table/table-display-inline.html [
NeedsRebaseline ]
+crbug.com/53693 fast/dynamic/insert-before-table-part-in-continuation.html [
NeedsRebaseline ]
+crbug.com/53693 [ Mac ] tables/mozilla/bugs/bug3037-1.html [ NeedsRebaseline ]

[commit-bot: I haz the power, 2013-09-06 12:36:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.suchit@samsung.com/23628004/87001

[commit-bot: I haz the power, 2013-09-06 14:25:33]

Change committed as 157368