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

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 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 Compare September 17, 2019 16:27
…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}
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