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 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 left a comment

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}
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1796803 branch from aa7a73f to 094ce4a Sep 11, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 364c517 into master Sep 11, 2019
12 checks passed
12 checks passed
update-pr-preview
Details
Azure Pipelines Build #20190911.48 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: 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 - firefox[experimental] Firefox 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-1796803 branch Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.