Allow whitespace inside anonymous table cells

Allow whitespace inside anonymous table cells

IE and FF allow whitespace inside anonymous table cells. We don't because we're
not careful enough when determining when to allow whitespace. Check if the space
we want to add will end up in anonymous table cell and if so, allow it.

Test=LayoutTests/fast/table/whitespace-in-anonymous-table-cells.html

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

[rhogan, 2015-04-21 18:12:24]

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

[leviw_travelin_and_unemployed, 2015-04-21 18:42:38]

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

https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp#ne...
Source/core/dom/Text.cpp:299: if (isAncestorOfAnonymousTableCell(parent))
This is more expensive in the common case, and this path can be hot. Do we know
of any actual web content that's effected by this?

[rhogan, 2015-04-21 19:00:49]

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

https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp#ne...
Source/core/dom/Text.cpp:299: if (isAncestorOfAnonymousTableCell(parent))
On 2015/04/21 at 18:42:37, leviw wrote:
> This is more expensive in the common case, and this path can be hot. Do we
know of any actual web content that's effected by this?

It's not that much more expensive to my reading. In the common case it will hit
isTableCellAncestor and return false, so three extra checks.

Greg from MS raised it - he usually does that on the basis of an
interoperability bug reported to them. I'll ping him on the crbug.

[rhogan, 2015-05-04 20:21:29]

On 2015/04/21 at 18:42:38, leviw wrote:
> https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp
> File Source/core/dom/Text.cpp (right):
> 
>
https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp#ne...
> Source/core/dom/Text.cpp:299: if (isAncestorOfAnonymousTableCell(parent))
> This is more expensive in the common case, and this path can be hot. Do we
know of any actual web content that's effected by this?

I've added a couple of performance tests at:

https://codereview.chromium.org/1126673002

This CL regresses AddWhitespaceToAnonymousTable by 22%, but AddWhitespace is
unaffected. I think this shows that the common case is largely unaffected.

[esprehn, 2015-05-04 22:12:15]

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

[esprehn, 2015-05-04 22:12:16]

This check doesn't look right, we also shouldn't be drilling down into tables
like that...

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

https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp#ne...
Source/core/dom/Text.cpp:275: static inline bool isTableCellAncestor(const
LayoutObject& renderer)
not inline. also don't use the word renderer. Maybe layouter?

https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp#ne...
Source/core/dom/Text.cpp:277: return renderer.isTable() || renderer.isTableRow()
|| renderer.isTableSection() || renderer.isLayoutTableCol();
LayoutTableCol layouters only ever have more LayoutTableCol children and nothing
else. They can't contain whitespace.

https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp#ne...
Source/core/dom/Text.cpp:280: static inline bool
isAncestorOfAnonymousTableCell(const LayoutObject& parent)
not inline.

https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp#ne...
Source/core/dom/Text.cpp:288: return
isAncestorOfAnonymousTableCell(*firstChild);
This check doesn't make sense, why are you drilling down the firstChild pointers
of the tree? This shouldn't be recursive.

[rhogan, 2015-05-05 21:18:40]

On 2015/05/04 at 22:12:16, esprehn wrote:
> This check doesn't look right, we also shouldn't be drilling down into tables
like that...

We only need to do it if the table part is the ancestor through other anonymous
table parts of an anonymous cell and that's where the whitespace would end up
living. It bails very quickly if that's not the case.

[rhogan, 2015-05-05 21:18:49]

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

https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp#ne...
Source/core/dom/Text.cpp:277: return renderer.isTable() || renderer.isTableRow()
|| renderer.isTableSection() || renderer.isLayoutTableCol();
On 2015/05/04 at 22:12:16, esprehn wrote:
> LayoutTableCol layouters only ever have more LayoutTableCol children and
nothing else. They can't contain whitespace.

Right, this makes such cases slightly more expensive though they ultimately
return the correct result because no cell is found.

https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp#ne...
Source/core/dom/Text.cpp:288: return
isAncestorOfAnonymousTableCell(*firstChild);
On 2015/05/04 at 22:12:16, esprehn wrote:
> This check doesn't make sense, why are you drilling down the firstChild
pointers of the tree? This shouldn't be recursive.

If the table part is the ancestor of an anonymous table cell (i.e. the
whitespace would end up in a table cell) then the cell will be a descendant of
the table part through the first child of each ancestor.

[rhogan, 2015-05-22 18:48:25]

On 2015/05/05 at 21:18:49, rhogan wrote:
> https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp
> File Source/core/dom/Text.cpp (right):
> 
>
https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp#ne...
> Source/core/dom/Text.cpp:277: return renderer.isTable() ||
renderer.isTableRow() || renderer.isTableSection() ||
renderer.isLayoutTableCol();
> On 2015/05/04 at 22:12:16, esprehn wrote:
> > LayoutTableCol layouters only ever have more LayoutTableCol children and
nothing else. They can't contain whitespace.
> 
> Right, this makes such cases slightly more expensive though they ultimately
return the correct result because no cell is found.
> 
>
https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp#ne...
> Source/core/dom/Text.cpp:288: return
isAncestorOfAnonymousTableCell(*firstChild);
> On 2015/05/04 at 22:12:16, esprehn wrote:
> > This check doesn't make sense, why are you drilling down the firstChild
pointers of the tree? This shouldn't be recursive.
> 
> If the table part is the ancestor of an anonymous table cell (i.e. the
whitespace would end up in a table cell) then the cell will be a descendant of
the table part through the first child of each ancestor.

Hi Eliot,

Any thoughts on my responses below? I do believe this change isn't as expensive
as it looks.

-Rob

[esprehn, 2015-05-28 05:43:55]

I'll have to think about this more, drilling down the tree still doesn't seem
right.

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

https://codereview.chromium.org/1065563005/diff/1/Source/core/dom/Text.cpp#ne...
Source/core/dom/Text.cpp:277: return renderer.isTable() || renderer.isTableRow()
|| renderer.isTableSection() || renderer.isLayoutTableCol();
On 2015/05/05 at 21:18:49, rhogan wrote:
> On 2015/05/04 at 22:12:16, esprehn wrote:
> > LayoutTableCol layouters only ever have more LayoutTableCol children and
nothing else. They can't contain whitespace.
> 
> Right, this makes such cases slightly more expensive though they ultimately
return the correct result because no cell is found.

The code doesn't make sense though, you can't find whitespace or rows there why
check at all?

[rhogan, 2015-06-09 19:44:34]

On 2015/05/28 at 05:43:55, esprehn wrote:
> I'll have to think about this more, drilling down the tree still doesn't seem
right.

I had another look at this and didn't come up with a new approach, but my
revised patch is clearer about how limited the recursion is and what motivates
it.

Can you take another look?

> The code doesn't make sense though, you can't find whitespace or rows there
why check at all?
Indeed, removed.

[rhogan, 2015-06-27 14:35:08]

On 2015/06/09 at 19:44:34, rhogan wrote:
> On 2015/05/28 at 05:43:55, esprehn wrote:
> > I'll have to think about this more, drilling down the tree still doesn't
seem right.
> 
> I had another look at this and didn't come up with a new approach, but my
revised patch is clearer about how limited the recursion is and what motivates
it.
> 
> Can you take another look?
> 
> > The code doesn't make sense though, you can't find whitespace or rows there
why check at all?
> Indeed, removed.

esprehn@ got time to take another look?

[esprehn, 2015-07-06 23:11:04]

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

https://codereview.chromium.org/1065563005/diff/20001/Source/core/dom/Text.cp...
Source/core/dom/Text.cpp:263: return true;
It's not clear to me that this actually works, in
LayoutTests/fast/table/whitespace-in-anonymous-table-cells.html if you start
with:

<div style="display:table; width:700px;">
    <div class="ib">There should be whitespace</div>
</div>

touch offsetTop so we create the layout tree, then append
    <div class="ib">between the two red rectangles.</div>

we won't go back and reattach the previous sibling whitespace so there's no
space.

[rhogan, 2015-07-07 18:35:40]

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

https://codereview.chromium.org/1065563005/diff/20001/Source/core/dom/Text.cp...
Source/core/dom/Text.cpp:263: return true;
On 2015/07/06 at 23:11:04, esprehn wrote:
> It's not clear to me that this actually works, in
LayoutTests/fast/table/whitespace-in-anonymous-table-cells.html if you start
with:
> 
> <div style="display:table; width:700px;">
>     <div class="ib">There should be whitespace</div>
> </div>
> 
> touch offsetTop so we create the layout tree, then append
>     <div class="ib">between the two red rectangles.</div>
> 
> we won't go back and reattach the previous sibling whitespace so there's no
space.

There is though, the following test has whitespace between the two divs:

 <!DOCTYPE html>
 <style>
 .ib {
     display: inline-block;
     width: 300px;
     background:red;
 }
 </style>
 <p> crbug.com/473832: Allow whitespace between the children of anonymous table
cells. </p>
 <div style="display:table; width:700px;" id="table">
     <div class="ib">There should be whitespace</div>
 </div>
 <div class="ib" id="content">between the two red rectangles.</div>
 <script>
    document.body.offsetTop;
    table.appendChild(document.getElementById("content")); 
 </script>
                                                                                
           
Also:

<!DOCTYPE html>
<style>
.ib {
    display: inline-block;
    width: 300px;
    background:red;
}
</style>
<p> crbug.com/473832: Allow whitespace between the children of anonymous table
cells. </p>
<div style="display:table; width:700px;" id="table">
    <div class="ib">There should be whitespace</div>
</div>
<script>
   document.body.offsetTop;
    divElem = document.createElement('div');
    divElem.setAttribute('class', 'ib');
    divElem.appendChild(document.createTextNode('between the two rectangles.'));
   table.appendChild(divElem); 
</script>

[esprehn, 2015-07-07 18:52:42]

If you have:

Table
Empty row
Whitespace

Then you insert into the empty row

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2015-07-07 19:24:33]

Staring at this more I think this is okay, lets try it.

lgtm

[esprehn, 2015-07-07 19:24:37]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2015-07-07 19:24:54]

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

[commit-bot: I haz the power, 2015-07-07 19:27:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-07 19:27:39]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/62000)

[rhogan, 2015-07-07 21:04:05]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-07-07 21:04:06]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1065563005/#ps40001
(title: "Updated")

[commit-bot: I haz the power, 2015-07-07 21:04:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1065563005/40001

[commit-bot: I haz the power, 2015-07-07 22:09:05]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198429

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

On 2015/07/07 at 22:09:05, commit-bot wrote:
> Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198429

I think this might have made a bunch of tests fail:
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/62...

tables/mozilla/bugs/bug2479-1.html
tables/mozilla/bugs/bug30985.html
tables/mozilla/bugs/bug72359.xml

[esprehn, 2015-07-08 06:30:01]

On 2015/07/08 at 06:24:25, esprehn wrote:
> On 2015/07/07 at 22:09:05, commit-bot wrote:
> > Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198429
> 
> I think this might have made a bunch of tests fail:
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/62...
> 
> tables/mozilla/bugs/bug2479-1.html
> tables/mozilla/bugs/bug30985.html
> tables/mozilla/bugs/bug72359.xml

Confirmed it was you:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=tab...

it was auto rebaselined here:
http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog_blink.html?u...

but still fails on 10.9 always now.