Closed Bug 1228984 Opened 8 years ago Closed 8 years ago

[css-grid] Issue with dense packing

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: rego, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: testcase)

Attachments

(5 files, 1 obsolete file)

Attached file jsbin.huwavi.1.html (obsolete) —
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
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.
Summary: [css-grid] Issue with "grid-auto-flow: column dense;" → [css-grid] Issue with dense packing
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?
Assignee: nobody → mats
Blocks: 1107778
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(rego)
Attachment #8693529 - Attachment is obsolete: true
Flags: needinfo?(rego)
(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.
Blocks: 1229009
OK, cool.  Thanks!
Keywords: testcase
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.
Attachment #8693799 - Flags: review?(dholbert)
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
Attachment #8693799 - Flags: review?(dholbert) → review+
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/16f3eb09533d
https://hg.mozilla.org/mozilla-central/rev/349b2d9ac5aa
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: