Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LayoutNG] Need parent's clearance offset when positioning child. #9984

Merged
merged 1 commit into from Mar 13, 2018

Conversation

Projects
None yet
4 participants
@chromium-wpt-export-bot
Copy link
Collaborator

chromium-wpt-export-bot commented Mar 12, 2018

If clear is specified on an element whose first piece of content is
inside a child (so that the element's BFC offset cannot be determined at
the beginning of layout), we need this child to know about the clearance
offset on the parent, or it will not be pushed below adjacent floats as
it should. Just pushing the parent, but leaving the children unaffected
by clearance won't do. We need this in order to be able to lay out in a
single pass (and apply clearance when we detect it, rather than doing
it on the element with 'clear' and relayout the children if something
moved).

Since a constraint space's clearance offset is now "inherited" down the
tree, as long as we're within the same block formatting context, we now
also need to propagate the "is pushed by floats" flag upwards, or we
won't detect class C break points correctly. Without this the unit test
ClassCBreakPointBeforeBlockMarginCollapsing in
NGColumnLayoutAlgorithmTest would regress, because it would incorrectly
detect a class C break point before the break-inside:avoid block.
We must make sure that class C break points are only detected on the
outermost block that got pushed by floats, because it's there that
we'll get the gap between the inner edge of the container and the outer
edge of the child.

Added some tests. One of them fails in legacy (but not in NG). One of
the tests, clear-on-child-with-margins.html, passes both with and
without this code change, but I started to wonder if we'd suddenly
could end up pulling the parent of the box with 'clear' downwards, so
thought I better add it, to make sure we don't regress in this regard.

The test NoClassCBreakPointBeforeBfc in
NGColumnLayoutAlgorithmTest no longer needs its workaround,
because the display:flow-root child of #container now sets its
position correctly (past the float) right away.

Acid2 also looks slightly better now!

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: I732b19398bd43b9874f6bb8c57ce435d1c510754
Reviewed-on: https://chromium-review.googlesource.com/957045
Commit-Queue: Morten Stenshorne mstensho@chromium.org
Reviewed-by: Emil A Eklund eae@chromium.org
Reviewed-by: Ian Kilpatrick ikilpatrick@chromium.org
Cr-Commit-Position: refs/heads/master@{#542769}

@wpt-pr-bot
Copy link
Collaborator

wpt-pr-bot left a comment

Already reviewed downstream.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 12, 2018

Build PASSED

Started: 2018-03-13 11:30:41
Finished: 2018-03-13 11:36:32

View more information about this build on:

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-957045 branch from 953a571 to c0ecaa1 Mar 13, 2018

[LayoutNG] Need parent's clearance offset when positioning child.
If clear is specified on an element whose first piece of content is
inside a child (so that the element's BFC offset cannot be determined at
the beginning of layout), we need this child to know about the clearance
offset on the parent, or it will not be pushed below adjacent floats as
it should. Just pushing the parent, but leaving the children unaffected
by clearance won't do. We need this in order to be able to lay out in a
single pass (and apply clearance when we detect it, rather than doing
it on the element with 'clear' and relayout the children if something
moved).

Since a constraint space's clearance offset is now "inherited" down the
tree, as long as we're within the same block formatting context, we now
also need to propagate the "is pushed by floats" flag upwards, or we
won't detect class C break points correctly. Without this the unit test
ClassCBreakPointBeforeBlockMarginCollapsing in
NGColumnLayoutAlgorithmTest would regress, because it would incorrectly
detect a class C break point before the break-inside:avoid block.
We must make sure that class C break points are only detected on the
outermost block that got pushed by floats, because it's there that
we'll get the gap between the inner edge of the container and the outer
edge of the child.

Added some tests. One of them fails in legacy (but not in NG). One of
the tests, clear-on-child-with-margins.html, passes both with and
without this code change, but I started to wonder if we'd suddenly
could end up pulling the parent of the box with 'clear' downwards, so
thought I better add it, to make sure we don't regress in this regard.

The test NoClassCBreakPointBeforeBfc in
NGColumnLayoutAlgorithmTest no longer needs its workaround,
because the display:flow-root child of #container now sets its
position correctly (past the float) right away.

Acid2 also looks slightly better now!

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: I732b19398bd43b9874f6bb8c57ce435d1c510754
Reviewed-on: https://chromium-review.googlesource.com/957045
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542769}

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-957045 branch from c0ecaa1 to b690a63 Mar 13, 2018

@chromium-wpt-export-bot chromium-wpt-export-bot merged commit b5b3c1f into master Mar 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-957045 branch Mar 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.