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] Fix stability issues with NGBlockLayoutAlgorithm. #16907

Merged
merged 1 commit into from May 20, 2019

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

chromium-wpt-export-bot commented May 17, 2019

...we had all the right concepts, but we're holding them the wrong way.

This patch does a few inter-related things to ensure we don't have
different parts of the block layout algorithm fighting each other.

Previously we could end up in a state where a child believed it should
be placed at a particular BFC-block-offset, and the parent disagreeing
with that. This caused us to perform three layouts, and fail the
DCHECK_EQ(layout_result->Status(), kSuccess) check.
(if we did the layouts in a loop, this would otherwise result in a
timeout).

This introduces/changes:

  • NGLayoutResult::IsEmptyBlock This flag is determined at the very
    end of the layout algorithm. If we didn't resolve our BFC
    block-offset, or were an empty-inline line-box we get this flag.
    This flag is more stable than checking if the
    NGLayoutResult::BlockBfcOffset had a value as this would sometimes
    be present for empty-blocks.
  • Changes the NGConstraintSpace::FloatsBfcBlockOffset to
    NGConstraintSpace::ForcedBfcBlockOffset. This is used in almost
    an identical way, except that:
    • Empty-blocks will always set their position to this.
    • Non-empty-blocks will typically also set their position to this
      (unless shifted by clearance).
  • AdjoiningFloatsTypes are now "passed" parent->child->sibling much
    like exclusion spaces are. Only at the end of an algorithm do we
    determine if they should be "passed" up to a parent based on if it
    is an empty block or not.
    This helps in determining clearance past adjoining floats.
  • Determining the BFC-block-offset when a child has aborted now has a
    more complex check to determine if the current layout should be
    considered as "clearing" floats also.
  • Removes the "forced-clearance" flag, in favour of the "forced" BFC
    block-offset, and adds the "ancestor has clearance past adjoining
    floats" flag.

Bug: 962344, 962300
Change-Id: Ife4030847f2e2b97aa5726072fadf581696ab8e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1611111
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#661462}

Copy link
Collaborator

wpt-pr-bot left a comment

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1611111 branch 4 times, most recently from fe6eb53 to 19d0cb8 May 17, 2019
...we had all the right concepts, but we're holding them the wrong way.

This patch does a few inter-related things to ensure we don't have
different parts of the block layout algorithm fighting each other.

Previously we could end up in a state where a child believed it should
be placed at a particular BFC-block-offset, and the parent disagreeing
with that. This caused us to perform three layouts, and fail the
DCHECK_EQ(layout_result->Status(), kSuccess) check.
(if we did the layouts in a loop, this would otherwise result in a
 timeout).

This introduces/changes:
 - NGLayoutResult::IsEmptyBlock This flag is determined at the very
   end of the layout algorithm. If we didn't resolve our BFC
   block-offset, or were an empty-inline line-box we get this flag.
   This flag is more stable than checking if the
   NGLayoutResult::BlockBfcOffset had a value as this would sometimes
   be present for empty-blocks.
 - Changes the NGConstraintSpace::FloatsBfcBlockOffset to
   NGConstraintSpace::ForcedBfcBlockOffset. This is used in *almost*
   an identical way, except that:
   - Empty-blocks will always set their position to this.
   - Non-empty-blocks will typically also set their position to this
     (unless shifted by clearance).
 - AdjoiningFloatsTypes are now "passed" parent->child->sibling much
   like exclusion spaces are. Only at the end of an algorithm do we
   determine if they should be "passed" up to a parent based on if it
   is an empty block or not.
   This helps in determining clearance past adjoining floats.
 - Determining the BFC-block-offset when a child has aborted now has a
   more complex check to determine if the current layout should be
   considered as "clearing" floats also.
 - Removes the "forced-clearance" flag, in favour of the "forced" BFC
   block-offset, and adds the "ancestor has clearance past adjoining
   floats" flag.

Bug: 962344, 962300
Change-Id: Ife4030847f2e2b97aa5726072fadf581696ab8e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1611111
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#661462}
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1611111 branch from 19d0cb8 to 7761a5f May 20, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot merged commit c3302fa into master May 20, 2019
15 checks passed
15 checks passed
manifest-build-and-tag manifest-build-and-tag
Details
staging.wpt.fyi - firefox[experimental] Firefox results
Details
website-build-and-publish website-build-and-publish
Details
wpt.fyi - firefox[experimental] Firefox results
Details
Azure Pipelines Build #20190520.90 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests (Safari Technology Preview)) affected tests (Safari Technology Preview) succeeded
Details
Azure Pipelines (affected tests without changes (Safari Technology Preview)) affected tests without changes (Safari Technology Preview) succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
staging.wpt.fyi - chrome[experimental] Chrome results
Details
staging.wpt.fyi - safari[experimental] Safari results
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - safari[experimental] Safari results
Details
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-1611111 branch May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.