New issue
Advanced search Search tips

Issue 813511 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Jan 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

Incorrect calculation of repeat() with multiple values, auto-fill and grid-gap

Reported by kizm...@gmail.com, Feb 19 2018

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 YaBrowser/17.11.1.1060 Yowser/2.5 Safari/537.36

Steps to reproduce the problem:
1. Go to https://codepen.io/kizu/pen/paayMm?editors=1100
2. Resize the viewport
3. Look at second example

What is the expected behavior?
The second example (the one with the bigger grid-gap) should work as well as the first one

What went wrong?
For some viewport values the width of the second example is calculated incorrectly, so there is a scrollbar created for the page and the layout looks broken.

Did this work before? No 

Does this work in other browsers? Yes

Chrome version: 62.0.3202.94  Channel: n/a
OS Version: OS X 10.13.2
Flash Version: 

The bigger the grid-gap, the bigger the problem, the first example at the codepen is also buggy, but as the grid-gap is very small, its almost unnoticable.

Other browsers: Edge works properly in all cases, but Firefox doesn't support multiple values at repeat() with auto-fill at all — https://bugzilla.mozilla.org/show_bug.cgi?id=1341507
 
grid-gap-repeat.png
86.8 KB View Download
Labels: Needs-Milestone

Comment 2 by e...@chromium.org, Feb 22 2018

Cc: r...@igalia.com
Components: -Blink>CSS Blink>Layout>Grid
Status: Available (was: Unconfirmed)

Comment 3 by r...@igalia.com, Feb 22 2018

Cc: svil...@igalia.com jfernan...@igalia.com
It seems that the problem is caused by the file   /src/third_party/blink/renderer/core/layout/layout_grid.cc

In the Grid Item Placement Algorithm, ( function ComputeAutoRepeatTracksCount ) some calculations go wrong, so it results into the wrong number of tracks and finally the wrong layout results.

I have tried to modify the calculations and for the example  https://codepen.io/kizu/pen/paayMm?editors=1100, the second example will not lead to a page scrollbar.
Issue 813511.mp4
1.5 MB View Download
Thanks for the report. We'll definitely take a look.
shenyu.tcv@ if you have any kind of patch feel free to share it here, or upload directly a proper patch to https://chromium-review.googlesource.com/ and ask us for review it (just in case documentation to upload a patch available at https://www.chromium.org/developers/contributing-code#TOC-Uploading-a-change-for-review).
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/67196393176e8c33b8e13380788b67a7d999f32d

commit 67196393176e8c33b8e13380788b67a7d999f32d
Author: shenyu.tcv <shenyu.tcv@gmail.com>
Date: Mon Jan 07 09:52:30 2019

[css-grid] Fix auto repeat with multiple tracks and gutters

The code that computes the number of auto repeat tracks wrongly assumes
that the second argument of the repeat() notation is a single track
function. That was true in the beginning, however specs were later on
modified to allow a <track-list>. We support a <track-list> as a second
argument since long ago but the code that computes the number of
auto-repeat tracks was never updated.

This patch modifies two places that relates to the gaps between the
auto-repeat tracks, which ensures the proper total length. 4 test
expected values in the auto-repeat-huge-grid.html are also modified.

BUG= 813511 

Change-Id: Iaf00e7d595a800f07cbe630868f30b82ad7fbd4d
Reviewed-on: https://chromium-review.googlesource.com/c/1393136
Commit-Queue: Manuel Rego <rego@igalia.com>
Reviewed-by: Manuel Rego <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#620278}
[modify] https://crrev.com/67196393176e8c33b8e13380788b67a7d999f32d/AUTHORS
[modify] https://crrev.com/67196393176e8c33b8e13380788b67a7d999f32d/third_party/blink/renderer/core/layout/layout_grid.cc
[add] https://crrev.com/67196393176e8c33b8e13380788b67a7d999f32d/third_party/blink/web_tests/external/wpt/css/css-grid/grid-definition/grid-auto-repeat-multiple-values-001.html
[add] https://crrev.com/67196393176e8c33b8e13380788b67a7d999f32d/third_party/blink/web_tests/external/wpt/css/css-grid/reference/grid-auto-repeat-multiple-values-001-ref.html
[modify] https://crrev.com/67196393176e8c33b8e13380788b67a7d999f32d/third_party/blink/web_tests/fast/css-grid-layout/grid-auto-repeat-huge-grid.html

Status: Fixed (was: Available)
Fixed by shenyu.tcv@, thank you very much!

Sign in to add a comment