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

Revert "[css-grid] Clear the override width for computing percent margins" #18988

Merged
merged 1 commit into from Sep 11, 2019

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Sep 11, 2019

This reverts commit a445d168b9f54cc8902f8bac8da3bc490e441059.

Reason for revert: It caused a performance regression (issue #1002700)

Original change's description:

[css-grid] Clear the override width for computing percent margins

When calculating the min-content contribution of a grid item of an auto
sized grid track we must consider the grid item's margin. When the grid
item's area is indefinite, a percent margin is resolved to zero.
However, when performing a relayout, the percent margin may be solved
against the previously computed grid area, since the grid item has
already an OverrideContainingBlockLogicalWidth value.

In order to re-compute the percent margin properly, we need to clear
the previously override value. It's important be careful of not
clearing the override value set during intrinsic size, since we need
it for the actual layout phase. Hence, we only reset the 'override'
value when we are executing a definite strategy.

Bug: 834643
Change-Id: Ib936b26bee1da76afbdc886eb775746e13d40988
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1782840
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#694849}

TBR=cbiesinger@chromium.org,jfernandez@igalia.com,rego@igalia.com

Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 834643, 1002700
Change-Id: I66f2b94417be0c74dc408bc55eee3a8d44447480
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1796803
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Cr-Commit-Position: refs/heads/master@{#695531}

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.

…gins"

This reverts commit a445d168b9f54cc8902f8bac8da3bc490e441059.

Reason for revert: It caused a performance regression (issue #1002700)

Original change's description:
> [css-grid] Clear the override width for computing percent margins
>
> When calculating the min-content contribution of a grid item of an auto
> sized grid track we must consider the grid item's margin. When the grid
> item's area is indefinite, a percent margin is resolved to zero.
> However, when performing a relayout, the percent margin may be solved
> against the previously computed grid area, since the grid item has
> already an OverrideContainingBlockLogicalWidth value.
>
> In order to re-compute the percent margin properly, we need to clear
> the previously override value. It's important be careful of not
> clearing the override value set during intrinsic size, since we need
> it for the actual layout phase. Hence, we only reset the 'override'
> value when we are executing a definite strategy.
>
> Bug: 834643
> Change-Id: Ib936b26bee1da76afbdc886eb775746e13d40988
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1782840
> Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
> Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#694849}

TBR=cbiesinger@chromium.org,jfernandez@igalia.com,rego@igalia.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 834643, 1002700
Change-Id: I66f2b94417be0c74dc408bc55eee3a8d44447480
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1796803
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Cr-Commit-Position: refs/heads/master@{#695531}
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