Commit ffe80a57 authored by Aleks Totic's avatar Aleks Totic Committed by Commit Bot

[LayoutNG] nested-percent-height regression

Will need help with this one.
I think it is a real regression.
Offending CL modifed LayoutBox::AvailableLogicalHeight
https://chromium-review.googlesource.com/c/chromium/src/+/1093996
LayoutBox::ComputePercentageLogicalHeight gets called 740 times
for each layout.

I was unable to reproduce this on my Linux box. Uploading
so I can run on Android.

The test also renders incorrectly in Legacy, correctly in NG.
The root cause of incorrect rendering is that
LayoutBox::ComputePercentageLogicalHeight for TableCell is
incorrect in Legacy when:

  #target {
    overflow: auto;
    height: 100%;
  }
  td {
    height: 100%;
  }
  table {
  }

<table>
  <td>
    <div id="target">

LayoutBox::ComputePercentageLogicalHeight should in this case return
intrinsic_logical_height instead of 0.

Bug: 852976
Change-Id: Iaf65c226eb9727877d14e68156d522421af57c87
Reviewed-on: https://chromium-review.googlesource.com/1103289Reviewed-by: default avatarChristian Biesinger <cbiesinger@chromium.org>
Commit-Queue: Aleks Totic <atotic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568123}
parent 1550abeb
...@@ -3698,14 +3698,25 @@ LayoutUnit LayoutBox::ComputeReplacedLogicalHeightUsing( ...@@ -3698,14 +3698,25 @@ LayoutUnit LayoutBox::ComputeReplacedLogicalHeightUsing(
LayoutUnit LayoutBox::AvailableLogicalHeight( LayoutUnit LayoutBox::AvailableLogicalHeight(
AvailableLogicalHeightType height_type) const { AvailableLogicalHeightType height_type) const {
if (RuntimeEnabledFeatures::LayoutNGEnabled()) {
// LayoutNG code is correct, Legacy code incorrectly ConstrainsMinMax
// when height is -1, and returns 0, not -1.
// The reason this code is NG-only is that this code causes perfomance
// regression for nested-percent-height-tables test case.
// This code gets executed 740 times in the test case.
// https://chromium-review.googlesource.com/c/chromium/src/+/1103289
LayoutUnit height =
AvailableLogicalHeightUsing(Style()->LogicalHeight(), height_type);
if (UNLIKELY(height == -1))
return height;
return ConstrainContentBoxLogicalHeightByMinMax(height, LayoutUnit(-1));
}
// http://www.w3.org/TR/CSS2/visudet.html#propdef-height - We are interested // http://www.w3.org/TR/CSS2/visudet.html#propdef-height - We are interested
// in the content height. // in the content height.
// FIXME: Should we pass intrinsicContentLogicalHeight() instead of -1 here? // FIXME: Should we pass intrinsicContentLogicalHeight() instead of -1 here?
LayoutUnit height = return ConstrainContentBoxLogicalHeightByMinMax(
AvailableLogicalHeightUsing(Style()->LogicalHeight(), height_type); AvailableLogicalHeightUsing(Style()->LogicalHeight(), height_type),
if (height != -1) LayoutUnit(-1));
height = ConstrainContentBoxLogicalHeightByMinMax(height, LayoutUnit(-1));
return height;
} }
LayoutUnit LayoutBox::AvailableLogicalHeightUsing( LayoutUnit LayoutBox::AvailableLogicalHeightUsing(
...@@ -3773,6 +3784,7 @@ LayoutUnit LayoutBox::AvailableLogicalHeightUsing( ...@@ -3773,6 +3784,7 @@ LayoutUnit LayoutBox::AvailableLogicalHeightUsing(
// mode. // mode.
LayoutUnit available_height = LayoutUnit available_height =
ContainingBlockLogicalHeightForContent(height_type); ContainingBlockLogicalHeightForContent(height_type);
// FIXME: This is incorrect if available_height == -1 || 0
if (height_type == kExcludeMarginBorderPadding) { if (height_type == kExcludeMarginBorderPadding) {
// FIXME: Margin collapsing hasn't happened yet, so this incorrectly removes // FIXME: Margin collapsing hasn't happened yet, so this incorrectly removes
// collapsed margins. // collapsed margins.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment