Recognize when an anonymous table cell has been generated to wrap a cell

Recognize when an anonymous table cell has been generated to wrap a cell

In https://codereview.chromium.org/1065563005 we started allowing whitespace
inside anonymous table cells. We're only interested in anonymous table cells
that were generated to contain non-table-part content. If the anonymous cell
was generated to contain a td element with display:block, for example, then
that is not a situation where we want to treat any whitespace before and 
after the td as renderable.

BUG=511401
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200183

[rhogan, 2015-07-22 18:58:47]

robhogan@gmail.com changed reviewers:
+ esprehn@chromium.org, leviw@chromium.org

[esprehn, 2015-07-22 19:12:51]

Why is this so specific to the first child?

I don't understand why having a real <td> inside a display: table: div should
change whitespace for the whole table.

Also if I appendChild a <div> to a <table> which then wraps it in an anonymous
cell that would also impact whitespace for the whole table because of old code
right?

https://codereview.chromium.org/1249433003/diff/1/Source/core/dom/Text.cpp
File Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/1249433003/diff/1/Source/core/dom/Text.cpp#ne...
Source/core/dom/Text.cpp:250: bool childIsTableCellWithAlternativeDisplayStyle =
firstChild && firstChild->node() && isHTMLTableCellElement(firstChild->node());
This variable name is hard to read, I think you just want to do:

return firstChild && firstChild->node() && ...;

and put a comment there.

https://codereview.chromium.org/1249433003/diff/1/Source/core/dom/Text.cpp#ne...
Source/core/dom/Text.cpp:251: return
!childIsTableCellWithAlternativeDisplayStyle;
Where is this in the spec? The tag name should have no influence I think...

[rhogan, 2015-07-23 12:20:17]

On 2015/07/22 at 19:12:51, esprehn wrote:
> Why is this so specific to the first child?

Because we're checking whether the table part has generated anonymous table part
wrappers to hold non-table part contents, so inspecting its first child will
suffice. So stuff like <table><div/><div/></table>.

> I don't understand why having a real <td> inside a display: table: div should
change whitespace for the whole table.
A real td inside a tr or table element means we should ignore the whitespace in
the tr or table. THe heuristic I use in this code is that a table or tr only
contains anonymous table cells if it contains nothing but elements other than
tds, checking whether the first child is an anonymous table cell is sufficient
for this. The exception we've found here is that you can have an isolated
anonymous table cell in a set of real table cell children if you give it a
display of block or flex or whatever. This throws off my check if you do it to
the first cell, so I need to check that the first  anonymous table cell isn't in
fact one of these:

 LayoutTable {TABLE}
   LayoutBlock (Anonymous) 
     LayoutFlexBox {TD}
   LayoutTableCell {TD}


> 
> Also if I appendChild a <div> to a <table> which then wraps it in an anonymous
cell that would also impact whitespace for the whole table because of old code
right?
Only if there are no table parts in the table - in which case we would expect it
to treat the whitespace as though it's inside the anonymous cell it has created.

I don't have to worry about cases where you put a random div into a table that
contains tds as these get relocated outside the table, e.g.
data:text/html,<table border=1> <div>a</div><td >1</td> <div>2</div> <td>3</td>
<td>4</td> <td>5</td> <td>6</td> <td>7</td> <td>8</td> </tr></table>

> 
>
https://codereview.chromium.org/1249433003/diff/1/Source/core/dom/Text.cpp#ne...
> Source/core/dom/Text.cpp:251: return
!childIsTableCellWithAlternativeDisplayStyle;
> Where is this in the spec? The tag name should have no influence I think...
Not with you here - maybe my response above clarifies this?

[esprehn, 2015-07-24 08:25:29]

On 2015/07/23 at 12:20:17, robhogan wrote:
>...
> 
> I don't have to worry about cases where you put a random div into a table that
contains tds as these get relocated outside the table, e.g.
data:text/html,<table border=1> <div>a</div><td >1</td> <div>2</div> <td>3</td>
<td>4</td> <td>5</td> <td>6</td> <td>7</td> <td>8</td> </tr></table>
> 

The parser is what moves the div out there, you can appendChild one into a
table. So you could insert a display: block div as the first child of a table by
just doing table.appendChild(div), then all the sibling text will find that
anonymous table cells and the whitespace will appear?

Could you write some more tests, does Firefox render them all the same?

[rhogan, 2015-07-27 21:25:48]

On 2015/07/24 at 08:25:29, esprehn wrote:
> On 2015/07/23 at 12:20:17, robhogan wrote:
> >...
> > 
> > I don't have to worry about cases where you put a random div into a table
that contains tds as these get relocated outside the table, e.g.
data:text/html,<table border=1> <div>a</div><td >1</td> <div>2</div> <td>3</td>
<td>4</td> <td>5</td> <td>6</td> <td>7</td> <td>8</td> </tr></table>
> > 
> 
> The parser is what moves the div out there, you can appendChild one into a
table. So you could insert a display: block div as the first child of a table by
just doing table.appendChild(div), then all the sibling text will find that
anonymous table cells and the whitespace will appear?
> 
> Could you write some more tests, does Firefox render them all the same?

Done - I added 2 tests where we append a div to a row of cells: one to the start
of the row and one to the middle of the row. FF renders them the same as we do.

[pdr., 2015-07-31 18:07:09]

pdr@chromium.org changed reviewers:
+ eae@chromium.org

[pdr., 2015-07-31 18:08:17]

pdr@chromium.org changed reviewers:
+ pdr@chromium.org

[pdr., 2015-07-31 18:08:18]

I think this makes sense but I'm really not an expert wrt tables. leviw and eae:
esprehn is out for a bit getting married. Does this look good to you too?

[eae, 2015-08-03 20:44:03]

Levi?

[leviw_travelin_and_unemployed, 2015-08-03 20:45:57]

The tag matching still seems a bit crazy to me, but I could be convinced.
Certainly the results make a lot more sense. Just to beat a dead horse, does
IE/Edge do this?

[rhogan, 2015-08-04 18:38:54]

On 2015/08/03 at 20:45:57, leviw wrote:
> The tag matching still seems a bit crazy to me, but I could be convinced.
Certainly the results make a lot more sense. Just to beat a dead horse, does
IE/Edge do this?

Yes, in fact it was MS who spotted the original bug.
https://code.google.com/p/chromium/issues/detail?id=473832

And yes, they pass the new test cases.

[leviw_travelin_and_unemployed, 2015-08-05 18:11:10]

Okay. I'm convinced, though it seems like a weird quirk to me. LGTM.

[rhogan, 2015-08-07 17:56:03]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2015-08-07 17:56:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1249433003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1249433003/20001

[commit-bot: I haz the power, 2015-08-07 19:13:29]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200183