Figure out how to deal with HasPercentHeightDescendants() in LayoutNG |
||
Issue descriptionLayoutNG 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.
,
May 4 2018
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?
,
May 4 2018
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.
,
May 4 2018
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.
,
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
,
May 9 2018
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.
,
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
,
May 11 2018
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
,
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
,
Aug 20
|
||
►
Sign in to add a comment |
||
Comment 1 by cbiesin...@chromium.org
, May 4 2018