New issue
Advanced search Search tips

Issue 839661 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Aug 20
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 591099



Sign in to add a comment

Figure out how to deal with HasPercentHeightDescendants() in LayoutNG

Project Member Reported by cbiesin...@chromium.org, May 4 2018

Issue description

LayoutNG does not set HasPercentHeightDescendants(). There's code in Blink that relies on that; long-term we will need a solution for that.

https://chromium-review.googlesource.com/c/chromium/src/+/1043009 bypasses a caller for now in NG.
 
Blocking: 591099
Could we pass the flag up the fragment tree when performing layout?
It can be done similarly to out of flow descendants, except instead of passing a list, we can just pass a boolean.

Does flexbox need to know a boolean, or a full list of percentage descendants?
So, this is only a problem if we ship NG without NG-flexbox AND the optimization that you disable actually is critical. Or maybe there'll be similar issues for e.g. tables?

The percent-height-descendants machinery is a messy one (at least to me), so it's probably not going to be easy to have NG set it correctly. But maybe I'm pessimistic.
Re comment 2, the problem with passing up the flag is that in NG it is not entirely trivial to know the value of the flag! Ideally what you want is:
- Bubble up until you hit an explicit non-percent height
- If any property uses percentages that depend on the height

That's mainly height, min-height, max-height, flex-basis (sometimes). But maybe more? And finding out when to stop bubbling is not so easy either.
Project Member

Comment 5 by bugdroid1@chromium.org, May 7 2018

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

commit 5196ad73c485b672e567f82975711827873572a6
Author: Christian Biesinger <cbiesinger@chromium.org>
Date: Mon May 07 22:59:20 2018

[layoutng] Skip a flexbox optimization in LayoutNG

In LayoutNG, HasPercentHeightDescendants() does not produce correct results
and it is difficult to get it to produce correct results. Accordingly, let's
disable this optimization for now.

Bug: 635619,  839661 
Change-Id: I766ea2fbea647eb9d0f3767b64788b678d328953
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Reviewed-on: https://chromium-review.googlesource.com/1043009
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556596}
[modify] https://crrev.com/5196ad73c485b672e567f82975711827873572a6/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/5196ad73c485b672e567f82975711827873572a6/third_party/blink/renderer/core/layout/layout_flexible_box.cc

My plan for this is basically https://chromium-review.googlesource.com/c/chromium/src/+/1051005

I'm not entirely sure why css3/flexbox/ bug646288 .html fails, but it seems unrelated to percentage changes (instead related to scrollbars) so maybe I should just ignore that test for now.
Project Member

Comment 7 by bugdroid1@chromium.org, May 9 2018

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

commit 2093cb98b4ee6bf55cc109bca023852cefd0e29e
Author: Christian Biesinger <cbiesinger@chromium.org>
Date: Wed May 09 22:43:04 2018

[layoutng] Re-introduce optimization

This implements a previously skipped optimization in a different
way for flexbox with NG children.

Adding css3/flexbox/ bug646288 .html to expectations because it seems like
that passed by accident; it does not use percentages.

Bug:  839661 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: I910b18f24d9d7a27d25f2d9bbd00bc585a3c5516
Reviewed-on: https://chromium-review.googlesource.com/1051005
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557353}
[modify] https://crrev.com/2093cb98b4ee6bf55cc109bca023852cefd0e29e/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/2093cb98b4ee6bf55cc109bca023852cefd0e29e/third_party/blink/renderer/core/layout/layout_block_flow.cc
[modify] https://crrev.com/2093cb98b4ee6bf55cc109bca023852cefd0e29e/third_party/blink/renderer/core/layout/layout_block_flow.h
[modify] https://crrev.com/2093cb98b4ee6bf55cc109bca023852cefd0e29e/third_party/blink/renderer/core/layout/layout_flexible_box.cc
[modify] https://crrev.com/2093cb98b4ee6bf55cc109bca023852cefd0e29e/third_party/blink/renderer/core/layout/layout_flexible_box.h
[modify] https://crrev.com/2093cb98b4ee6bf55cc109bca023852cefd0e29e/third_party/blink/renderer/core/layout/ng/layout_ng_mixin.cc
[modify] https://crrev.com/2093cb98b4ee6bf55cc109bca023852cefd0e29e/third_party/blink/renderer/core/layout/ng/layout_ng_mixin.h

The big issue (flexbox performance) is addressed by that patch but I'll leave this open so we can figure out if any other callers of HasPercentHeightDescendants need work
Project Member

Comment 9 by bugdroid1@chromium.org, May 25 2018

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

commit bcef67c0645ca064628ffc9368356beb8f030d11
Author: Christian Biesinger <cbiesinger@chromium.org>
Date: Fri May 25 07:23:58 2018

[layoutng] Use ShouldForceLayoutForNGChild instead of HasPercentHeightDescendants

I already changed the other use of that function in this file, but I
missed this spot in 2093cb98b4ee6bf55cc109bca023852cefd0e29e

R=eae@chromium.org,mstensho@chromium.org

Bug:  839661 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: I6bc69933e12ad9d4fdfe9813337ec54f552dcf98
Reviewed-on: https://chromium-review.googlesource.com/1072322
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561797}
[modify] https://crrev.com/bcef67c0645ca064628ffc9368356beb8f030d11/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/bcef67c0645ca064628ffc9368356beb8f030d11/third_party/blink/renderer/core/layout/layout_flexible_box.cc

Status: Fixed (was: Available)

Sign in to add a comment