New issue
Advanced search Search tips

Issue 849839 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug



Sign in to add a comment

[LayoutNG] fixed position DCHECK (fast/scrolling/fractional-scroll-offset-fixed-position-non-composited.html)

Project Member Reported by cbiesin...@chromium.org, Jun 5 2018

Issue description

I was investigating the DCHECKs in layout_object.h(345), specifically in this testcase:
fast/scrolling/fractional-scroll-offset-fixed-position-non-composited.html

What's going on is that we never (re-)lay out the inner fixed-pos element. It seems what's going on is this:
- We do lay it out once, through the code called by LayoutNGBlockFlow::UpdateOutOfFlowBlockLayout, specifically in the second iteration of the loop at NGOutOfFlowLayoutPart::Run
- That's because contains_fixed_ is true when we call this code for the outer fixedpos element through LayoutBlock::LayoutPositionedObject
- contains_fixed_ is true because LayoutNGBlockFlow::UpdateOutOfFlowBlockLayout uses css_container->CanContainFixedPositionObjects
- Because we do process it here, we never call UseOldOutOfFlowPositioning on the inner fixedpos object
- Because we don't do that, we never insert the inner object to the viewport's list of positioned objects
- Because we don't do that, we do not lay it out when we have to

I think the real issue is that UpdateOutOfFlowBlockLayout needs to make sure that it lays itself out based on css_container->CanContain*, *but* for any descendants it needs to use this->CanContain*

Maybe we needs two passes through the OOF layout part, one for descendants, one for this?

Tentatively assigning to myself to look into that suggestion but I won't have time to work on this until at least tomorrow.
 
Hm I don't think that analysis was correct. Investigating more.
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 6 2018

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

commit 6a5108214c636b6e7cf0da64bb39f0db06c1324f
Author: Christian Biesinger <cbiesinger@chromium.org>
Date: Wed Jun 06 23:47:59 2018

[layoutng] UpdateOutOfFlowBlockLayout should only lay out itself

This should avoid some double-layouts, and as a side-effect (the original
motivation) it also makes sure that we mark any descendants we find for
UseOldOutOfFlowPositioning(), which fixes a number of DCHECK(!NeedsLayout) failures.
That works through the result->OutOfFlowPositionedDescendants() loop in
LayoutNGBlockFlow::UpdateOutOfFlowBlockLayout.

Bug:  849839 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: If885b16cedaa01b65f7fe2548e42a48c5d3a8a0d
Reviewed-on: https://chromium-review.googlesource.com/1089652
Reviewed-by: Aleks Totic <atotic@chromium.org>
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565103}
[modify] https://crrev.com/6a5108214c636b6e7cf0da64bb39f0db06c1324f/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/6a5108214c636b6e7cf0da64bb39f0db06c1324f/third_party/blink/renderer/core/layout/ng/layout_ng_block_flow.cc
[modify] https://crrev.com/6a5108214c636b6e7cf0da64bb39f0db06c1324f/third_party/blink/renderer/core/layout/ng/ng_out_of_flow_layout_part.cc
[modify] https://crrev.com/6a5108214c636b6e7cf0da64bb39f0db06c1324f/third_party/blink/renderer/core/layout/ng/ng_out_of_flow_layout_part.h

Status: Fixed (was: Available)

Sign in to add a comment