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

row span and auto positionning in Firefox #2

Closed
ledahulevogyre opened this issue Jul 30, 2017 · 21 comments
Closed

row span and auto positionning in Firefox #2

ledahulevogyre opened this issue Jul 30, 2017 · 21 comments

Comments

@ledahulevogyre
Copy link

Here I try to layout 8 divs automatically, based on their row span definition.
https://codepen.io/anon/pen/Nvxdjo
Row heights appears to adapt well for the first 5 rows. But then, all the subsequent rows seem to have the height of the 5th row. The consequences are the blank spaces in the 4th and 5th divs.

The behavior seems good on Blink. It compresses the rows/divs as I expect.

@rachelandrew
Copy link
Owner

I think the issue is in a difference in how Firefox is interpreting those empty rows vs. Chrome.

If I add grid-auto-rows: 50px we end up with the same display in both. I'm just trying to figure out who is correct.

Should the auto size rows collapse to zero? That's what Chrome is doing. Or, should the space be distributed? This is what Firefox does.

@ledahulevogyre
Copy link
Author

Thanks Rachel.
Firefox does not seem to always distribute the space evenly either. Not all the rows (of divs 1, 2, and 3 for example) have the same height.
I guess this is where the answer is but I don't grasp every concepts.

@mrego
Copy link
Contributor

mrego commented Aug 2, 2017

I'd need to check the track sizing algorithm, but on a first sight it seems that Chromium is right as it doesn't increase the size of rows without a clear reason.

Check this reduced example: https://codepen.io/mrego/pen/wqWbOg
First 2 rows doesn't need to be bigger than 20px in total (10px each), but Firefox is increasing them somehow.

@rachelandrew
Copy link
Owner

Yes, my initial feeling was that Chrome was doing the right thing. If there was nothing at all in the rows they would be zero height. The fact something spans them probably shouldn't make any difference.

@ledahulevogyre
Copy link
Author

Chrome has bugs too, imo.
Here is @mrego 's example a bit modified (third element added in first colum) : https://codepen.io/anon/pen/JyRPzz
It behaves weird to me, when i3 has a row span of 2 or 3 (higher values seem ok)

@svillar
Copy link

svillar commented Aug 2, 2017

In the test case by @mrego Blink is clearly the one behaving correctly. Without checking the algorithm is obvious that we are not increasing the sizes of the tracks if not needed, something that is a basis for all the heuristics used in the algorithm.

Now the technical justification based on the algorithm. The relevant section is section 11.5, list item 2. where it says "Increase sizes to accommodate spanning items"

  • After processing the 1st item (the one spanning 2 rows, the sizes of the rows is as follows:
    [10, 10] [10, 10] [0, inf]
  • Now we process the 2nd item (the algorithm processes 1st the non-spanning items, then the ones spanning 2, then spanning 3...). There are 6 different steps, we'll go over them:
    1. We need to accomodate a 80px item, rows 1 and 2 sum 20px, so we have to distribute 60px. As tracks 1 and 2 cannot grow (they are both [10, 10]) we distribute everything to the 3rd track. Here I suspect FF does the wrong thing, it distributes 60px equally between the 3 rows.
    2. Does nothing
    3. Does nothing
    4. Does nothing
    5. Same as in 1, as tracks 1 and 2 have a fixed growth limit of 10, then we distribute 60px to the third row exclusively
    6. Does nothing

So the correct result is indeed [10,10] [10,10] [60, 60]

@svillar
Copy link

svillar commented Aug 2, 2017

Regarding the other example posted by @ledahulevogyre although it looks a bit weird it's correct in Chrome and again a bug in Firefox. Technically it's more complicated to explain than the above case. The algorithm sizes tracks by grouping items by the amount of span, meaning that it processes first the non-spanning items, then the ones spanning 2, then spanning 3... But there is one important thing to take into account, and is that when computing the track sizes for a particular span level it must do it independently on the DOM order of each item, meaning that if I have 2 items spanning 3 tracks, I should get the same result independently on how they're specified in the DOM.

I've modified your example slightly. First I specified all the item positions so that autoplacement does not generate different results, and then created the very same example but switching the positions of items 2 and 3 in the source code. Since the track sizing algorithm should work independently of the order (https://drafts.csswg.org/css-grid/#distribute-extra-space), in Blink the result is the same in both cases as it should be, while in Firefox they behave differently.

See:
https://codepen.io/anon/pen/WEGpPe?editors=1100#0
https://codepen.io/anon/pen/VzKpgx?editors=1100#0

@rachelandrew
Copy link
Owner

Thanks for all the digging into this!

I have raised the two issues with Firefox:

  1. https://bugzilla.mozilla.org/show_bug.cgi?id=1386921
  2. https://bugzilla.mozilla.org/show_bug.cgi?id=1386932

I have linked to this discussion but please add comments to those reports if you feel I have not fully represented the issue at hand.

Also added all of this as issue #12 on this site.

@ledahulevogyre
Copy link
Author

Thank you @svillar
What does "The algorithm sizes tracks by grouping items by the amount of span" mean concretely? How does this rule make the last track to have a non zero height ? I don't understand.

@svillar
Copy link

svillar commented Aug 3, 2017

So I'll try to explain it using the test case you've posted. As mentioned after processing the item spanning 2 rows the row sizes look like this:

[10,10] [10,10] [0, inf] [0,inf] [0,inf]

Now you need to process the other two items which span 3 columns. Item 2 spans rows 1,2 & 3 and item 3 spans rows 3,4 & 5. As I mentioned the algorithm ensures that the order in which you process items should not affect the final result. So the way it works is that we get candidate sizes for the tracks for every single item and then we run a final pass setting the sizes that make everything valid. These are the candidate sizes computed for each item spanning 3 rows:

item 2 (height 80px) computes: [10,10] [10,10] [60, 60] [0,inf] [0,inf]
item 3 (height 20px) computes: [10,10] [10,10] [6, 6] [6, 6] [6, 6]

So now you combine both results and get [10,10] [10,10] [60, 60] [6, 6] [6, 6]. You could think, "oh but you don't need nonzero tracks 4&5 because the item 3 already fits in row 3". That's true but you've explicitly set that the item spans through 3 auto rows so the right thing to do is to distribute equally among these tracks.

Hope I have clarified your doubts

@ledahulevogyre
Copy link
Author

I see now. Thanks for the explanation.
I think it's good that the algorithm isn't influenced by the DOM order.
But intuitively, it's the layout order that should be prevalent. That is to say, the items starting "higher" should be processed first.
@rachelandrew , do you think it's good to ask the spec to be corrected that way ?

@rachelandrew
Copy link
Owner

I don't think this is a spec issue, you are of course able to raise issues against the spec if you want to.

@svillar
Copy link

svillar commented Aug 3, 2017

Not sure what you mean by "it's the layout order that should be prevalent" but what grid does is preciselly layout its tracks so that:
a) content sized tracks ensure that the largest min-content contribution from any item in the track is respected
b) track sizes should be as compact as possible, meaning that we should not grow them if not needed

@ledahulevogyre
Copy link
Author

But b) is not respected in this case. Or I misunderstood (quite possibly).
@rachelandrew why do you think it's not an issue ? Do you think it's normal that a grid can be smaller if you only increase one span number ?

@rachelandrew
Copy link
Owner

I don't really follow your concern, however I'm not an editor of the specification so if you think there is an issue you should raise it against the spec itself.

@ledahulevogyre
Copy link
Author

Here is an example of my concern : https://codepen.io/anon/pen/qXaygO?editors=1100
I guess the algorithm can be improved to fix this paradox. I'm not sure.
I'll try to have more experts/implementers opinions on this, and maybe raise an issue.

@rachelandrew
Copy link
Owner

The appropriate place for spec discussion is on the spec where the editors can comment.

@svillar
Copy link

svillar commented Aug 3, 2017

@ledahulevogyre: what you must do is definitely a). Then you should try to do b) the best you can. If you think about it there is likely no better way to do that. You're explicitly asking to span 3 rows so you have to give them non-zero size.

@svillar
Copy link

svillar commented Aug 3, 2017

@ledahulevogyre: I don't deny that it might seems wrong to have a more compact grid when using an item that spans more tracks but the fact is that it's working as expected. I'll explain why.

In the second grid you have items spanning 2,3 and 4 rows. So they'll be processed in 3 different steps. After processing the one spanning 2 you'd get

[10,10] [10,10] [0,inf] [0,inf] [0,inf] [0,inf]

then you process just the one spanning 3 (in grid 2 there is just 1) so you get

[10,10] [10,10] [60,60] [0,inf] [0,inf] [0,inf]

Now you process the one spanning 4. As track 3 has been already resolved in the previous step, the algorithm decides not to change anything, because the item has 20px height which perfectly fits in the current grid area already defined for tracks 3,4,5,6.

Again I am not saying that it is not weird from the author POV but just wanted to clarify that is not a bug from the spec POV. As @rachelandrew suggested if you think this is a bug in the spec then feel free to report it.

@scheinercc
Copy link

@svillar has this been fixed in the meantime? Just checked with FF DevEdition 62.0b16 (64-bit)/macOS and the second example renders the same way as it does in Chrome.

@svillar
Copy link

svillar commented Aug 14, 2018

No idea, let's ask @MatsPalgrem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants