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

[css-grid] Fix resolution of percentage paddings and margins of grid items #10194

Merged
merged 1 commit into from Apr 2, 2018

Conversation

Projects
None yet
5 participants
@chromium-wpt-export-bot
Copy link
Collaborator

chromium-wpt-export-bot commented Mar 27, 2018

We were not resolving properly percentage paddings and margins
for tracks that have something like minmax(auto, 100px).
The reason was that while computing the minimum size of a grid item,
the percentages were resolved against the inline size of the grid container.
But for grid items we shouldn't never use the grid container size,
but the grid area size, as that's their containing block.

The patch modifies ContainingBlockLogicalWidthForContent() and
ContainingBlockLogicalHeightForContent() in LayoutBox,
so for grid items we return 0 if the area size hasn't been set yet.
We never want to use the grid container's sizes in these cases.

BUG=808758
TEST=external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-*
TEST=external/wpt/css/css-grid/grid-items/grid-items-percentage-paddings-*

Change-Id: Ib142e51aee1fe623d38688469b179f01f82eb07b
Reviewed-on: https://chromium-review.googlesource.com/980756
Reviewed-by: Javier Fernandez jfernandez@igalia.com
Commit-Queue: Manuel Rego Casasnovas rego@igalia.com
Cr-Commit-Position: refs/heads/master@{#547417}

@wpt-pr-bot
Copy link
Collaborator

wpt-pr-bot left a comment

Already reviewed downstream.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 27, 2018

Build ERRORED

Started: 2018-04-02 10:40:15
Finished: 2018-04-02 10:43:40

Failing Jobs

  • chrome:dev

Unstable Results

Browser: "Chrome Dev"

View in: WPT PR Status | TravisCI

Test Subtest Results Messages
/css/css-grid/grid-items/grid-items-percentage-margins-vertical-lr-002.html   OK: 10
/css/css-grid/grid-items/grid-items-percentage-paddings-vertical-lr-002.html   OK: 10
/css/css-grid/grid-items/grid-items-percentage-paddings-vertical-rl-002.html   OK: 10
  .grid 1 PASS: 10
FAIL: 10
  .grid 2 PASS: 10
FAIL: 10
  .grid 3 PASS: 1
FAIL: 9
assert_equals:
X
height expected 60 but got 73

  .grid 4 PASS: 1
FAIL: 9
assert_equals:
X
height expected 60 but got 73

  .grid 5 PASS: 10
FAIL: 10
  .grid 6 PASS: 10
FAIL: 10
  .grid 7 PASS: 1
FAIL: 9
assert_equals:
X
height expected 60 but got 72

  .grid 8 PASS: 1
FAIL: 9
assert_equals:
X
height expected 60 but got 72

@chromium-wpt-export-bot chromium-wpt-export-bot changed the title [css-grid] Fix resolution of percentage paddings and marings of grid items [css-grid] Fix resolution of percentage paddings and margins of grid items Mar 28, 2018

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-980756 branch from 7f23119 to 0a619a0 Mar 28, 2018

[css-grid] Fix resolution of percentage paddings and margins of grid …
…items

We were not resolving properly percentage paddings and margins
for tracks that have something like minmax(auto, 100px).
The reason was that while computing the minimum size of a grid item,
the percentages were resolved against the inline size of the grid container.
But for grid items we shouldn't never use the grid container size,
but the grid area size, as that's their containing block.

The patch modifies ContainingBlockLogicalWidthForContent() and
ContainingBlockLogicalHeightForContent() in LayoutBox,
so for grid items we return 0 if the area size hasn't been set yet.
We never want to use the grid container's sizes in these cases.

BUG=808758
TEST=external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-*
TEST=external/wpt/css/css-grid/grid-items/grid-items-percentage-paddings-*

Change-Id: Ib142e51aee1fe623d38688469b179f01f82eb07b
Reviewed-on: https://chromium-review.googlesource.com/980756
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Commit-Queue: Manuel Rego Casasnovas <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#547417}
@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Apr 2, 2018

@mrego it looks like some of these tests are flaky on Chrome Dev and this is what's blocking the PR from being merged. Can you check if you're able to reproduce the flakiness? From the Travis logs it looks like maybe a layout hasn't happened that the test assumes has happened.

@mrego

This comment has been minimized.

Copy link
Member

mrego commented Apr 2, 2018

@foolip I managed to run the tests locally (thanks @Ms2ger) with:

./wpt run --stability --binary=/usr/bin/google-chrome-unstable chrome css/css-grid/grid-items/grid-items-percentage-margins-vertical-rl-002.html

and also:

./wpt check-stability --binary=/usr/bin/google-chrome-unstable chrome css/css-grid/grid-items/grid-items-percentage-margins-vertical-rl-002.html

But I cannot reproduce the flakiness locally.

In Chromium we have some issues with the percentage resolutions and results vary depending if the layout phase is run once, twice, etc. The patch is fixing some of them (there might other things still present). What should we do regarding these tests?

PD: Sorry for not realizing about this Travis issue before merging the upstream patch (it'd be awesome that Travis somehow notifies the Chromium CL to avoid this kind of things).

@mrego

This comment has been minimized.

Copy link
Member

mrego commented Apr 2, 2018

I managed to reproduce the flakiness locally with a reduced test case, and I can confirm the patch that has just landed on Chromium fixes it.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Apr 2, 2018

Sorry for not realizing about this Travis issue before merging the upstream patch (it'd be awesome that Travis somehow notifies the Chromium CL to avoid this kind of things).

We want this as extension of #7475 (@lukebjerring FYI) but as made evident by this issue it's a bit tricky. Testing something that's just a little different from and a little bit older than the content_shell built and tested can get confusing.

One option might be to also test on using chrome+chromedriver built from the Chromium CQ and ignore failures that are only in Chrome Dev, but that'd also decrease the odds of detecting real flakiness.

@mrego, until we have this figured out, you don't need to worry about things grong wrong in Travis, I'd wager that more often than not no change to the original CL is needed, i.e., the signal:noise ratio isn't high enough with our current setup.

@foolip foolip merged commit e4efc10 into master Apr 2, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@foolip foolip deleted the chromium-export-cl-980756 branch Apr 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.