1228984 - [css-grid] Issue with dense packing

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Manuel Rego Casasnovas]

Attachment: jsbin.huwavi.1.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/602.1 (KHTML, like Gecko) Version/8.0 Safari/602.1 Debian/buildd-unstable (3.18.0-1) Epiphany/3.18.0

Steps to reproduce:

Testing the implementation of the grid placement algorithm it seems we've found an issue using "column dense" for grid-auto-flow.

You can reproduce the issue with the attached example (http://jsbin.com/huwavi/1/edit?html,css,output).




Actual results:


The item "i4" is placed in the 4th column. However in Chrome it's placed in the 2nd column.


Expected results:


Algorithm in the spec (https://drafts.csswg.org/css-grid/#auto-placement-algo) says:
"“dense” packing (dense specified)
  For each grid item that hasn’t been positioned by the previous steps, in order-modified document order:
  [...]
  If the item has a definite column position:
    1. Set the row position of the cursor to 1. Set the column position of the cursor to be equal to the column-start line index of the grid item.
    [...]
"

So we think it should use the gap in the 2nd column for the "i4" item.

Trying to reproduce it using "grid-auto-flow: row dense" was not possible. You can check the following example where "i4" is placed in the 2nd row (not the 4th one): http://jsbin.com/rojepi/1/edit?html,css,output

Comment 1

[Manuel Rego Casasnovas]

Actually I managed to reproduce it with "row dense" with a different use case: http://jsbin.com/sudora/1/edit?html,css,output

Here "i4" should be in the 1st row and 2nd column. And "i5" in the 3rd row and 1st column.

However that's not happening in Firefox.

Comment 2

[Mats Palmgren (inactive)]

Thanks Rego, this is indeed a bug in our code and the rendering in Canary looks correct.

BTW, is it OK with you if I copy-paste your tests and put it under this license:

<!--
     Any copyright is dedicated to the Public Domain.
     http://creativecommons.org/publicdomain/zero/1.0/
-->

so that I can add them to our (reftest) regression test suite?

Comment 3

[Manuel Rego Casasnovas]

Attachment: Original issue reported (column dense)

Comment 4

[Manuel Rego Casasnovas]

Attachment: Similar case using row dense where the issue doesn't happen

Comment 5

[Manuel Rego Casasnovas]

Attachment: Another case showing the issue with row dense too

Comment 6

[Manuel Rego Casasnovas]

(In reply to Mats Palmgren (:mats) from comment #2)
> BTW, is it OK with you if I copy-paste your tests and put it under this
> license:
> 
> <!--
>      Any copyright is dedicated to the Public Domain.
>      http://creativecommons.org/publicdomain/zero/1.0/
> -->
> 
> so that I can add them to our (reftest) regression test suite?

Sure. I misread you the first time, so I've uploaded the 3 examples adding the license. :-)

But next time and in other bugs you can modify them without problems. I'd try to upload them with the license already.

Comment 7

[Mats Palmgren (inactive)]

OK, cool.  Thanks!

Comment 8

[Mats Palmgren (inactive)]

Attachment: [css-grid] Auto-placement into columns missed some unoccupied span-1 column slots.

So we miss some column slots of length 1 here because when the inner
loop bumps into such an unoccupied slot it sets gap=0, which makes us
continue the outer loop at j+1 even though we saw an empty slot at j.
We should instead have set gap=1 in this particular case, so that
"gap < extent" (with extent=1) would exit the outer loop since we
did in fact find a candidate slot on this row.

We could fix it with s/gap = 0/gap = j < len ? 1 : 0/ but the code
here is kind of hard to grok as it is, so I chose to simplify it
instead by removing the optimization (inner loop) because I don't
think it does much good anyway.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Added reftests based on the tests in this bug and bug 1229009

Comment 10

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 8693799 [details] [diff] [review]
[css-grid] Auto-placement into columns missed some unoccupied span-1 column slots.

Review of attachment 8693799 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16f3eb09533d
https://hg.mozilla.org/integration/mozilla-inbound/rev/349b2d9ac5aa

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/16f3eb09533d
https://hg.mozilla.org/mozilla-central/rev/349b2d9ac5aa