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 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 wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Compare May 20, 2019 19:50
...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}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants