Closed
Bug 1228984
Opened 8 years ago
Closed 8 years ago
[css-grid] Issue with dense packing
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: rego, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: testcase)
Attachments
(5 files, 1 obsolete file)
623 bytes,
text/html
|
Details | |
626 bytes,
text/html
|
Details | |
630 bytes,
text/html
|
Details | |
1.52 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
13.16 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•8 years ago
|
||
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
Assignee | ||
Comment 2•8 years ago
|
||
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)
Reporter | ||
Comment 3•8 years ago
|
||
Attachment #8693529 -
Attachment is obsolete: true
Flags: needinfo?(rego)
Reporter | ||
Comment 4•8 years ago
|
||
Reporter | ||
Comment 5•8 years ago
|
||
Reporter | ||
Comment 6•8 years ago
|
||
(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.
Assignee | ||
Comment 7•8 years ago
|
||
OK, cool. Thanks!
Assignee | ||
Comment 8•8 years ago
|
||
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)
Assignee | ||
Comment 9•8 years ago
|
||
Comment 10•8 years ago
|
||
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+
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/16f3eb09533d https://hg.mozilla.org/integration/mozilla-inbound/rev/349b2d9ac5aa
Assignee | ||
Updated•8 years ago
|
Flags: in-testsuite+
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/16f3eb09533d https://hg.mozilla.org/mozilla-central/rev/349b2d9ac5aa
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•