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

Reland "[css-grid] Clear the override width for computing percent margins" #18979

Merged
merged 1 commit into from Sep 24, 2019

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

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

This is a reland of a445d168b9f54cc8902f8bac8da3bc490e441059..
The original patch has been reverted due to a performance regression,
described in bug #1002700.

Additionally, this patch solves some cases not covered by the original patch,
specifically when using 'non-auto' values for the grid item's min-width
property. It also provides additional regressions tests.

Finally, the performance regression is solved by checking out whether the
grid item needs layout before considering to clear the Override size.

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}

Bug: 834643, 942811
Change-Id: Id6fdf52fab471ebefecb2c087c086faf0e3f5dec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1796323
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Manuel Rego <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#699290}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1796323 branch from 01c62b4 to f70cc0e Sep 12, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot changed the title [css-grid] Additional cases where the override width must be cleared Reland "[css-grid] Clear the override width for computing percent margins" Sep 12, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1796323 branch 5 times, most recently from 7bb45fa to 2992278 Sep 12, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1796323 branch from 2992278 to 76b3ff9 Sep 24, 2019
…gins"

This is a reland of a445d168b9f54cc8902f8bac8da3bc490e441059..
The original patch has been reverted due to a performance regression,
described in bug #1002700.

Additionally, this patch solves some cases not covered by the original patch,
specifically when using 'non-auto' values for the grid item's min-width
property. It also provides additional regressions tests.

Finally, the performance regression is solved by checking out whether the
grid item needs layout before considering to clear the Override size.

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}

Bug: 834643, 942811
Change-Id: Id6fdf52fab471ebefecb2c087c086faf0e3f5dec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1796323
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Manuel Rego <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#699290}
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1796323 branch from 76b3ff9 to d32e2fc Sep 24, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 2462e55 into master Sep 24, 2019
14 checks passed
14 checks passed
update-pr-preview
Details
Azure Pipelines Build #20190924.73 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 - chrome[experimental] Chrome results
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 - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-1796323 branch Sep 24, 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.