Ensure cell's replaced content doesn't get the wrong width

Ensure cell's replaced content doesn't get the wrong width

We need to make the cell's height unusable for calculating perecent/calc height
against before computing its preferred width, otherwise replaced children may use it
to scale up their intrinsic width.

BUG=666730
Review-Url: https://codereview.chromium.org/2851513004
Cr-Commit-Position: refs/heads/master@{#472228}
Committed: https://chromium.googlesource.com/chromium/src/+/03d66d9ce1e602df26a9d544d9258a88b9194349

[rhogan, 2017-04-28 09:18:56]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 09:19:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rhogan, 2017-04-28 09:21:59]

Description was changed from

==========
Ensure cell's replaced content doesn't get the wrong width

BUG=666730
==========

to

==========
Ensure cell's replaced content doesn't get the wrong width

We need to reset the cell's overridden height before computing its preferred
width, 
otherwise replaced content may use it to scale up its intrinsic width.

BUG=666730
==========

[commit-bot: I haz the power, 2017-04-28 11:26:28]

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

[commit-bot: I haz the power, 2017-04-28 11:26:29]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[rhogan, 2017-04-29 11:57:39]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 11:57:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 14:01:24]

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

[commit-bot: I haz the power, 2017-04-29 14:01:25]

Dry run: This issue passed the CQ dry run.

[rhogan, 2017-04-30 08:57:59]

Description was changed from

==========
Ensure cell's replaced content doesn't get the wrong width

We need to reset the cell's overridden height before computing its preferred
width, 
otherwise replaced content may use it to scale up its intrinsic width.

BUG=666730
==========

to

==========
Ensure cell's replaced content doesn't get the wrong width

We need to make the cell's height unusable for calculating perecent/calc height
against before computing its preferred width, otherwise replaced children may
use it
to scale up their intrinsic width.

BUG=666730
==========

[rhogan, 2017-04-30 08:58:39]

robhogan@gmail.com changed reviewers:
+ mstensho@opera.com

[rhogan, 2017-05-01 16:12:38]

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:217: 
OK I know this is ugly, but I think what makes cells special here is that its
not safe to clear their override height properly here - the section they're in
may not get a layout. We would need to be certain that if the logical width has
been dirtied it will definitely change the width of the column and ensure the
layout of the cell. That doesn't seem unreasonable but I wouldn't bet on it.

[mstensho (USE GERRIT), 2017-05-03 16:21:51]

I'll get to this as soon as I can. One comment on the test in the meantime. :)

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/table/percent-height-replaced-content-in-cell.html
(right):

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/percent-height-replaced-content-in-cell.html:19:
img.addEventListener("load", onImageLoaded, false);
No need for a forced layout before this? (document.offsetTop)

[mstensho (USE GERRIT), 2017-05-12 11:44:07]

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:209: // We don't want
the preferred width from children to be affected by any
Agreed. No preferred width calculation code should take actual layout as input.
Where's this code?

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:217: 
On 2017/05/01 16:12:38, rhogan wrote:
> OK I know this is ugly, but I think what makes cells special here is that its
> not safe to clear their override height properly here - the section they're in
> may not get a layout. We would need to be certain that if the logical width
has
> been dirtied it will definitely change the width of the column and ensure the
> layout of the cell. That doesn't seem unreasonable but I wouldn't bet on it.

Yeah, if we really need a special height override here, we should definitely put
things back to how they were, when we're done being weird. :)

My question is: do we really have to do this? Preferred width calculation should
never take actual layout as input (it's the other way around: preferred widths
are input to layout). Actual layout should not be fed back through preferred
width calculation and then back into layout.

[rhogan, 2017-05-12 19:12:23]

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:209: // We don't want
the preferred width from children to be affected by any
On 2017/05/12 at 11:44:07, mstensho wrote:
> Agreed. No preferred width calculation code should take actual layout as
input. Where's this code?

Here you go:

diff --git a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
index b444f08..8dd75e1 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
@@ -26,6 +26,8 @@
 
 #include "core/layout/LayoutBox.h"
 
 #include <math.h>
 #include <algorithm>
 #include "core/dom/Document.h"
@@ -3679,8 +3681,10 @@ LayoutUnit LayoutBox::AvailableLogicalHeightUsing(
   // some new height, and then when we lay out again we'll use the calculation
   // below.
   if (IsTableCell() && (h.IsAuto() || h.IsPercentOrCalc())) {
-    if (HasOverrideLogicalContentHeight())
+    if (HasOverrideLogicalContentHeight())  {
+      printf("in here\n");
       return OverrideLogicalContentHeight();
+    }
     return LogicalHeight() - BorderAndPaddingLogicalHeight();
   }


callstack:

#0 0x7f68e52c8007 base::debug::StackTrace::StackTrace()
#1 0x7f68e19dcc8c blink::LayoutBox::AvailableLogicalHeightUsing()
#2 0x7f68e19dc004 blink::LayoutBox::ComputeReplacedLogicalHeightUsing()
#3 0x7f68e1a59540 blink::LayoutReplaced::ComputeReplacedLogicalHeight()
#4 0x7f68e1a59457 blink::LayoutReplaced::ComputeReplacedLogicalWidth()
#5 0x7f68e1a5994c blink::LayoutReplaced::ComputePreferredLogicalWidths()
#6 0x7f68e19d0420 blink::LayoutBox::MinPreferredLogicalWidth()
#7 0x7f68e19a0f02 blink::LayoutBlock::ComputeChildPreferredLogicalWidths()
#8 0x7f68e19c40dd blink::LayoutBlockFlow::ComputeInlinePreferredLogicalWidths()
#9 0x7f68e199f6bd blink::LayoutBlock::ComputeIntrinsicLogicalWidths()
#10 0x7f68e19a071b blink::LayoutBlock::ComputePreferredLogicalWidths()
#11 0x7f68e1a6fb79 blink::LayoutTableCell::ComputePreferredLogicalWidths()
#12 0x7f68e19d04e0 blink::LayoutBox::MaxPreferredLogicalWidth()
#13 0x7f68e1aba0c4 blink::TableLayoutAlgorithmAuto::RecalcColumn()
#14 0x7f68e1abb40a blink::TableLayoutAlgorithmAuto::FullRecalc()
#15 0x7f68e1abb6b1
blink::TableLayoutAlgorithmAuto::ComputeIntrinsicLogicalWidths()
#16 0x7f68e1a69100 blink::LayoutTable::ComputePreferredLogicalWidths()
#17 0x7f68e19d04e0 blink::LayoutBox::MaxPreferredLogicalWidth()
#18 0x7f68e1a6470c blink::LayoutTable::UpdateLogicalWidth()
#19 0x7f68e1a6670f blink::LayoutTable::UpdateLayout()
#20 0x7f68e19aaeff blink::LayoutBlockFlow::PositionAndLayoutOnceIfNeeded()
#21 0x7f68e19ab318 blink::LayoutBlockFlow::LayoutBlockChild()
#22 0x7f68e19a9f8e blink::LayoutBlockFlow::LayoutBlockChildren()
#23 0x7f68e19a7fcc blink::LayoutBlockFlow::LayoutChildren()
#24 0x7f68e19a7b21 blink::LayoutBlockFlow::UpdateBlockLayout()
#25 0x7f68e199b251 blink::LayoutBlock::UpdateLayout()
#26 0x7f68e19aaeff blink::LayoutBlockFlow::PositionAndLayoutOnceIfNeeded()
#27 0x7f68e19ab318 blink::LayoutBlockFlow::LayoutBlockChild()


When children get laid out the container's logical height has already been
reset, however the 'override' height used by table cells is not. I haven't
investigated if other users clear it before calculating preferred widths.

[mstensho (USE GERRIT), 2017-05-15 06:50:09]

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:209: // We don't want
the preferred width from children to be affected by any
On 2017/05/12 19:12:23, rhogan wrote:
> On 2017/05/12 at 11:44:07, mstensho wrote:
> > Agreed. No preferred width calculation code should take actual layout as
> input. Where's this code?
> 
> Here you go:
> 
> diff --git a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
> b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
> index b444f08..8dd75e1 100644
> --- a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
> +++ b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
> @@ -26,6 +26,8 @@
>  
>  #include "core/layout/LayoutBox.h"
>  
>  #include <math.h>
>  #include <algorithm>
>  #include "core/dom/Document.h"
> @@ -3679,8 +3681,10 @@ LayoutUnit LayoutBox::AvailableLogicalHeightUsing(
>    // some new height, and then when we lay out again we'll use the
calculation
>    // below.
>    if (IsTableCell() && (h.IsAuto() || h.IsPercentOrCalc())) {
> -    if (HasOverrideLogicalContentHeight())
> +    if (HasOverrideLogicalContentHeight())  {
> +      printf("in here\n");
>        return OverrideLogicalContentHeight();
> +    }
>      return LogicalHeight() - BorderAndPaddingLogicalHeight();
>    }
> 
> 
> callstack:
> 
> #0 0x7f68e52c8007 base::debug::StackTrace::StackTrace()
> #1 0x7f68e19dcc8c blink::LayoutBox::AvailableLogicalHeightUsing()
> #2 0x7f68e19dc004 blink::LayoutBox::ComputeReplacedLogicalHeightUsing()
> #3 0x7f68e1a59540 blink::LayoutReplaced::ComputeReplacedLogicalHeight()
> #4 0x7f68e1a59457 blink::LayoutReplaced::ComputeReplacedLogicalWidth()
> #5 0x7f68e1a5994c blink::LayoutReplaced::ComputePreferredLogicalWidths()
> #6 0x7f68e19d0420 blink::LayoutBox::MinPreferredLogicalWidth()

Here's the problem. Calculating preferred/intrinsic sizes should NEVER take
LogicalHeight() (or OverrideLogicalContentHeight(), for that matter) as input. I
think there's missing use of SizeType here. That one can be used to determine
whether we're calculating the used/actual size or preferred/intrinsic size.

ComputeReplacedLogicalHeightUsing() takes a SizeType, but either fails to
respect it, or AvailableLogicalHeightUsing() needs to take that parameter as
well. I didn't really look that closely at the code to tell which.

[rhogan, 2017-05-15 20:52:42]

On 2017/05/15 at 06:50:09, mstensho wrote:
>
https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):
> 
>
https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:209: // We don't
want the preferred width from children to be affected by any
> On 2017/05/12 19:12:23, rhogan wrote:
> > On 2017/05/12 at 11:44:07, mstensho wrote:
> > > Agreed. No preferred width calculation code should take actual layout as
> > input. Where's this code?
> > 
> > Here you go:
> > 
> > diff --git a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
> > b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
> > index b444f08..8dd75e1 100644
> > --- a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
> > +++ b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
> > @@ -26,6 +26,8 @@
> >  
> >  #include "core/layout/LayoutBox.h"
> >  
> >  #include <math.h>
> >  #include <algorithm>
> >  #include "core/dom/Document.h"
> > @@ -3679,8 +3681,10 @@ LayoutUnit LayoutBox::AvailableLogicalHeightUsing(
> >    // some new height, and then when we lay out again we'll use the
calculation
> >    // below.
> >    if (IsTableCell() && (h.IsAuto() || h.IsPercentOrCalc())) {
> > -    if (HasOverrideLogicalContentHeight())
> > +    if (HasOverrideLogicalContentHeight())  {
> > +      printf("in here\n");
> >        return OverrideLogicalContentHeight();
> > +    }
> >      return LogicalHeight() - BorderAndPaddingLogicalHeight();
> >    }
> > 
> > 
> > callstack:
> > 
> > #0 0x7f68e52c8007 base::debug::StackTrace::StackTrace()
> > #1 0x7f68e19dcc8c blink::LayoutBox::AvailableLogicalHeightUsing()
> > #2 0x7f68e19dc004 blink::LayoutBox::ComputeReplacedLogicalHeightUsing()
> > #3 0x7f68e1a59540 blink::LayoutReplaced::ComputeReplacedLogicalHeight()
> > #4 0x7f68e1a59457 blink::LayoutReplaced::ComputeReplacedLogicalWidth()
> > #5 0x7f68e1a5994c blink::LayoutReplaced::ComputePreferredLogicalWidths()
> > #6 0x7f68e19d0420 blink::LayoutBox::MinPreferredLogicalWidth()
> 
> Here's the problem. Calculating preferred/intrinsic sizes should NEVER take
LogicalHeight() (or OverrideLogicalContentHeight(), for that matter) as input. I
think there's missing use of SizeType here. That one can be used to determine
whether we're calculating the used/actual size or preferred/intrinsic size.
> 
> ComputeReplacedLogicalHeightUsing() takes a SizeType, but either fails to
respect it, or AvailableLogicalHeightUsing() needs to take that parameter as
well. I didn't really look that closely at the code to tell which.

kMainOrPreferredSize isn't really set up to do that, it's used for actual and
preferred - it's only widths that use ShouldComputePreferred to distinguish
between layout and style computation.

The thing to note is that the preferred widths calculation is already set up to
handle this correctly (by setting the container's logical height to zero before
calculating them). It's only where we have an override that things get wacky. If
it weren't for the fact that the cell's override may not get recalculated later
we could safely clear it before calculating preferred widths, like we would
probably do if we were a normal element. But tables being tables, we can't be
sure the cell will get a layout even though its widths are dirty.

So I think this is a wrinkle that lives in the tables code rather than in the
way we compute preferred widths in general.

[mstensho (USE GERRIT), 2017-05-16 07:57:54]

OK, let there be wrinkles! But I want them neat! :)

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:216:
SetOverrideLogicalContentHeight(LayoutUnit());
You wrote:

> The thing to note is that the preferred widths calculation is already
> set up to handle this correctly (by setting the container's logical
> height to zero before calculating them). It's only where we have an
> override that things get wacky.

So, you could instead do ClearOverrideLogicalContentHeight() here? That would
look slightly prettier, wouldn't it?

https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:223:
ClearOverrideLogicalContentHeight();
And if it's possible to clear it above, you won't have to do it here, right?

[rhogan, 2017-05-16 18:24:32]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 18:25:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rhogan, 2017-05-16 18:29:44]

On 2017/05/16 at 07:57:54, mstensho wrote:
> OK, let there be wrinkles! But I want them neat! :)
> 
>
https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):
> 
>
https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:216:
SetOverrideLogicalContentHeight(LayoutUnit());
> You wrote:
> 
> > The thing to note is that the preferred widths calculation is already
> > set up to handle this correctly (by setting the container's logical
> > height to zero before calculating them). It's only where we have an
> > override that things get wacky.
> 
> So, you could instead do ClearOverrideLogicalContentHeight() here? That would
look slightly prettier, wouldn't it?
> 
>
https://codereview.chromium.org/2851513004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:223:
ClearOverrideLogicalContentHeight();
> And if it's possible to clear it above, you won't have to do it here, right?

Yes, I've improved it a bit. I still need to set a zero
OverrideLogicalContentHeight so that it will avoid calculating against the
override value. If I clear it to -1 it will end up using the actual height on
the cell, which will be the value it has flexed to due to the override, i.e. the
same value.

I think this is still fine, even though its a narrower fix, as we are only
worried about cases where the content of the cell or another cell on the row has
flexed its height and they always have an override.

[mstensho (USE GERRIT), 2017-05-16 18:35:57]

I think this is as good as it gets. I like the repeated "if (content_height >
-1)". Pretty obvious what's going on now, and that there's nothing more to it.

lgtm

[commit-bot: I haz the power, 2017-05-16 20:52:30]

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

[commit-bot: I haz the power, 2017-05-16 20:52:31]

Dry run: This issue passed the CQ dry run.

[rhogan, 2017-05-16 21:44:20]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2017-05-16 21:45:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 22:15:24]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494971060974040,
"parent_rev": "25ae5928489058ac5bf0d0e7230f8dc5873da175", "commit_rev":
"03d66d9ce1e602df26a9d544d9258a88b9194349"}

[commit-bot: I haz the power, 2017-05-16 22:15:41]

Description was changed from

==========
Ensure cell's replaced content doesn't get the wrong width

We need to make the cell's height unusable for calculating perecent/calc height
against before computing its preferred width, otherwise replaced children may
use it
to scale up their intrinsic width.

BUG=666730
==========

to

==========
Ensure cell's replaced content doesn't get the wrong width

We need to make the cell's height unusable for calculating perecent/calc height
against before computing its preferred width, otherwise replaced children may
use it
to scale up their intrinsic width.

BUG=666730

Review-Url: https://codereview.chromium.org/2851513004
Cr-Commit-Position: refs/heads/master@{#472228}
Committed:
https://chromium.googlesource.com/chromium/src/+/03d66d9ce1e602df26a9d544d925...
==========

[commit-bot: I haz the power, 2017-05-16 22:15:42]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/03d66d9ce1e602df26a9d544d925...