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

third_party/WebKit/Source/core/layout/LayoutTableCell.cpp

 /*
  * Copyright (C) 1997 Martin Jones (mjones@kde.org)
  *           (C) 1997 Torben Weis (weis@kde.org)
  *           (C) 1998 Waldo Bastian (bastian@kde.org)
  *           (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc.
  *               All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 188 matching lines @@
 }
 
 void LayoutTableCell::ComputePreferredLogicalWidths() {
   // The child cells rely on the grids up in the sections to do their
   // computePreferredLogicalWidths work.  Normally the sections are set up
   // early, as table cells are added, but relayout can cause the cells to be
   // freed, leaving stale pointers in the sections' grids. We must refresh those
   // grids before the child cells try to use them.
   Table()->RecalcSectionsIfNeeded();
 
+  // We don't want the preferred width from children to be affected by any

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

Agreed. No preferred width calculation code should take actual layout as input.
Where's this code?

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

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]

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.

+  // notional height on the cell, such as can happen when a percent sized image
+  // scales up its width to match the available height. Setting a zero override
+  // height prevents this from happening.
+  LayoutUnit content_height = HasOverrideLogicalContentHeight()
+                                  ? OverrideLogicalContentHeight()
+                                  : LayoutUnit(-1);
+  SetOverrideLogicalContentHeight(LayoutUnit());

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

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

+

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

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/12 11:44:07]

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.

   LayoutBlockFlow::ComputePreferredLogicalWidths();
+
+  if (content_height > -1)
+    SetOverrideLogicalContentHeight(content_height);
+  else
+    ClearOverrideLogicalContentHeight();

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

And if it's possible to clear it above, you won't have to do it here, right?

+
   if (GetNode() && Style()->AutoWrap()) {
     // See if nowrap was set.
     Length w = StyleOrColLogicalWidth();
     const AtomicString& nowrap = ToElement(GetNode())->getAttribute(nowrapAttr);
     if (!nowrap.IsNull() && w.IsFixed()) {
       // Nowrap is set, but we didn't actually use it because of the fixed width
       // set on the cell. Even so, it is a WinIE/Moz trait to make the minwidth
       // of the cell into the fixed width. They do this even in strict mode, so
       // do not make this a quirk. Affected the top of hiptop.com.
       min_preferred_logical_width_ =
@@ skipping 1265 matching lines @@
 }
 
 bool LayoutTableCell::HasLineIfEmpty() const {
   if (GetNode() && HasEditableStyle(*GetNode()))
     return true;
 
   return LayoutBlock::HasLineIfEmpty();
 }
 
 }  // namespace blink