New issue
Advanced search Search tips

Issue 876459 link

Starred by 1 user
Status: Fixed
Owner: ----
Closed: Aug 27
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 635619



Sign in to add a comment

[layoutng] ApplyStretchAlignmentToChild optimization breaks some test cases

Project Member Reported by cbiesin...@chromium.org, Aug 21 
external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-definite-sizes-002.html
external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-definite-sizes-004.html

That test fails in NG. The reason is this code:
void LayoutFlexibleBox::ApplyStretchAlignmentToChild
    bool child_needs_relayout = desired_logical_height != child.LogicalHeight();
    if ((child.IsLayoutNGMixin() &&
         ShouldForceLayoutForNGChild(ToLayoutBlockFlow(child))) ||
        (child.IsLayoutBlock() &&
         ToLayoutBlock(child).HasPercentHeightDescendants())) {

In this testcase, the NG child is a *descendant* of the flexbox, so the first part of the if does not catch it. But because the percentage-sized item is an NG item, the second part of the if does not catch it either.

I'm not immediately sure how to fix this; perhaps ComputeBlockSizeForFragment should call AddPercentageHeightDescendant if height/min-height/max-height are percentage-sized...

Comment 1 by cbiesin...@chromium.org, Aug 21

Description: Show this description
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 27 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/229c8737a637b6d9b82e6fc2748c5574a16b27f1

commit 229c8737a637b6d9b82e6fc2748c5574a16b27f1
Author: Christian Biesinger <cbiesinger@chromium.org>
Date: Mon Aug 27 18:18:31 2018

[layoutng] Restore a legacy layout optimization

When we do have percentage descendants, we need to force an extra
layout when applying stretch alignment, because the flexbox height
is now definite (per https://drafts.csswg.org/css-flexbox/#algo-stretch)

LayoutNG never set the "has percentage descendants" flag, so we could
not skip this extra layout when there are none. We instead had a
different optimization that addresses the case where the height was
already definite.

This patch makes it so that NG does set the flag and makes flexbox
use this optimization in NG as well. This has the nice side effect
of fixing two tests, in addition to the performance improvement,
because we now correctly handle the case of legacy flexbox ->
legacy flexbox -> NG block -> NG percentage sized, where the
outer flexbox now knows it needs to redo layout in this case (see
 crbug.com/876459  for more details)

Performance improvements according to
https://pinpoint-dot-chromeperf.appspot.com/job/15b736ee640000:
flexbox-lots-of-data          +43.9%
flexbox-with-stretch-layout  +851.2%

Bug: 875147, 876459 

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I535f0c62993f4cdc03f15cdf91efe300989e43b2
Reviewed-on: https://chromium-review.googlesource.com/1190246
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586318}
[modify] https://crrev.com/229c8737a637b6d9b82e6fc2748c5574a16b27f1/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/229c8737a637b6d9b82e6fc2748c5574a16b27f1/third_party/blink/renderer/core/layout/layout_flexible_box.cc
[modify] https://crrev.com/229c8737a637b6d9b82e6fc2748c5574a16b27f1/third_party/blink/renderer/core/layout/layout_flexible_box.h
[modify] https://crrev.com/229c8737a637b6d9b82e6fc2748c5574a16b27f1/third_party/blink/renderer/core/layout/ng/ng_length_utils.cc

Comment 4 by cbiesin...@chromium.org, Aug 27

Status: Fixed (was: Available)
►

Sign in to add a comment