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

[css-grid] Applying 'justify-content' content distribution is in the wrong place in the overall grid sizing algo #2557

Closed
MatsPalmgren opened this Issue Apr 12, 2018 · 26 comments

Comments

Projects
None yet
7 participants
@MatsPalmgren
Copy link

MatsPalmgren commented Apr 12, 2018

The Grid spec currently says that "the tracks are aligned within the grid container according to the align-content and justify-content properties." in step 4, after the sizes of both columns and rows have been determined. This leads to the wrong layout because intrinsic row sizes are then determined using the wrong Containing Block inline-size for spanning items. Here's a testcase demonstrating the wrong layout in Chrome (the row size is too large for the text). The layout in Firefox is IMHO the correct one - it sizes the row based on the height of the text when the CB inline-size is 500px, which is what the grid-column: span 2 grid area is sized to after justify-content is applied.

The correct place to apply justify-content in the overall algo is between step 1 and 2, i.e. first you determine the column sizes, then you apply justify-content. This allows you to calculate the correct CB inline-size (and padding/margin percentage basis) for the items. Now you can calculate the correct row sizes, and after that apply align-content.

@mrego

This comment has been minimized.

Copy link
Member

mrego commented Apr 12, 2018

I knew this would end up in a issue here. I discussed the case with @tabatkins today, and he was pretty sure Chromium was right.
The spec even has a note about this, so it seems intentional:

Note: This can introduce extra space between tracks, potentially enlarging the grid area
of any grid items spanning the gaps beyond the space allotted to during track sizing.

Anyway let's see what other people think about this (CC @fantasai & @atanassov).
Or if someone recalls the reason why this is done this way.

BTW, Firefox and Edge behave the same for this case, and different from Chromium and WebKit.
Note that for the same example with an orthgonal item neither of Edge, Chromium or WebKit recompute the size of the column. Firefox just don't take orthogonal into accounts so the result is different.

@mrego mrego added the css-grid-1 label Apr 12, 2018

@fantasai

This comment has been minimized.

Copy link
Contributor

fantasai commented Apr 20, 2018

IIRC we considered the effect of alignment on same-axis sizing, e.g. it would add some slack to a spanning element which was spanning only max-content sized tracks. However, we didn't consider the effect of alignment on sizing in the other axis, and it does seem to me that doing so would be an improvement in most cases. (If orthogonal cases don't get quite the same benefits that's probably OK, as long as we don't break them.)

@mrego

This comment has been minimized.

Copy link
Member

mrego commented Apr 23, 2018

If we do this as part of the track sizing algorithm, this will conflict with what was discussed at #2409.

For example you have 2 rows of 100px each, and then you've a orthogonal item spanning both rows.
In #2409 we said that the item should consider it has 200px height, for the purpose of computing the proper intrinsic size with baseline.
But, if there's align-content: space-between the final height of the item could be 300px or 500px (it'd depend on the grid container's height). We couldn't compute that in advance to know the estimated height to do the intrinsic size computation.

@fantasai

This comment has been minimized.

Copy link
Contributor

fantasai commented May 1, 2018

@mrego I'm not following. We use 200px height to calculate the intrinsic width of the orthogonal item, sure. And then we start the track sizing algorithm, sizing the columns based on that information. Whether we perform alignment of the tracks after that set of tracks is sized or at the end of the algorithm, the final size of the orthogonal item will still be 300px or 500px. If we apply alignment within the grid algorithm, it seems to me we get a better result regardless, no?

The proposal, if I understand correctly (just so we're all clear) is to change from:

  1. Size the columns, making some estimates about row sizes (if needed for orthogonal flows)
  2. Size the rows, using the column sizes we just calculated (if needed for orthogonal flows)
  3. Size the column, using the row sizes we just calculated (skip if not needed)
  4. Size the rows, using the column sizes we just calculated (skip if not needed)
  5. Align the tracks
  6. Lay out items into the resulting grid areas

to:

  1. Size the columns, making some estimates about row sizes (if needed for orthogonal flows). Align the columns (so colspans in the next step are more accurate).
  2. Size the rows, using the column sizes we just calculated (if needed for orthogonal flows). Align the rows (so rowspans in the next step are more accurate).
  3. Size the column, using the row sizes we just calculated (skip if not needed). Align the rows (so colspans in the next step are more accurate).
  4. Size the rows, using the column sizes we just calculated (skip if not needed). Align the rows.
  5. Lay out items into the resulting grid areas
@javifernandez

This comment has been minimized.

Copy link
Contributor

javifernandez commented May 1, 2018

Whether we perform alignment of the tracks after that set of tracks is sized or at the end of the algorithm, the final size of the orthogonal item will still be 300px or 500px. If we apply alignment within the grid algorithm, it seems to me we get a better result regardless, no?

The issue is that baseline alignment in the row-axis (justify-self) may depend on the final size of the row, as explained in #2409.

@fantasai

This comment has been minimized.

Copy link
Contributor

fantasai commented May 4, 2018

@javifernandez I'm not understanding why this is a problem. :(

@javifernandez

This comment has been minimized.

Copy link
Contributor

javifernandez commented May 4, 2018

This issue is about the place to run the Content Alignment logic; with the new baseline alignment integrated in the track sizing algorithm, the place where we finally run the Content Alignment logic will have an impact in the result of the Baseline Alignment, generating different results.

@tabatkins

This comment has been minimized.

Copy link
Member

tabatkins commented May 17, 2018

Hmm, indeed. Case in point:

<style>
.grid { 
 display: grid; 
 grid: 100px 100px auto / auto; 
 align-content: space-between; 
 height: 300px;
}
.ortho {
 grid-row: 1 / span 2;
 font: 50px Ahem;
 writing-mode: vertical-rl;
 background: purple;
}
.content {
 grid-row: 3;
}
</style>
<div class=grid>
  <div class=ortho>É É ÉÉ</div>
  <div class=content>foo</div>
</div>

Today, the spec says that the orthogonal item can tell how large its containing block is (200px high, as it spans only rows with fixed max track sizing functions). The actual height of the block might not be 200px, as it depends on how much space is left over in the grid after the content-sized track is sized.

If we move the distribution up in the algorithm, then we have to weaken the condition we're using - the amount of gap, and thus the size of the grid area, is content-sized if any track in the entire grid is content-sized. Per the spec, this means we have to assume the grid area is infinite, and we'll get the original bad layout from #2409.

(On that note, we should probably make it clearer that gaps are considered in the calculation of the grid-area size, even if we leave distribution at the end.)

@fantasai

This comment has been minimized.

Copy link
Contributor

fantasai commented May 17, 2018

No, we don't change the rules for estimating the height of the rows/gaps in step 1. It still starts out assuming the fixed heights and no content distribution gaps -- but at the end of step 2, we take the actually-calculated row sizes (and alignment) into account when performing the resizing check in step 3.

@tabatkins

This comment has been minimized.

Copy link
Member

tabatkins commented May 17, 2018

@fantasai points out that I'm wrong - I'm talking about Step 1 in my previous comment, but row-distribution would happen in a new Step 2.5. So for the purpose of Step 1, the gap is still 0, and the item would still lay out into a fixed size of 400px.

The distribution would then likely change the min-content contribution of the item, when we hit step 3, but that will just make us cycle once, and hopefully with a closer-to-correct min/max-content contribution.

@tabatkins

This comment has been minimized.

Copy link
Member

tabatkins commented May 17, 2018

@javifernandez Given the above, do you agree that it's okay to move content distribution up into new steps 1.5 and 2.5 (and eliminate step 4)? In the common case you'll get tighter, more acccurate grid sizes; in some cases (ortho flows, column-wrap flexboxes, etc) you might trigger a step-3 cycle more often than the spec currently does, but it should give a better result, too.

@fantasai fantasai added the Agenda+ label May 17, 2018

@javifernandez

This comment has been minimized.

Copy link
Contributor

javifernandez commented May 21, 2018

@javifernandez Given the above, do you agree that it's okay to move content distribution up into new steps 1.5 and 2.5 (and eliminate step 4)?

@tabatkins are you talking about the original 6 steps fantasai described ?

  1. Size the columns, making some estimates about row sizes (if needed for orthogonal flows)
  2. Size the rows, using the column sizes we just calculated (if needed for orthogonal flows)
  3. Size the column, using the row sizes we just calculated (skip if not needed)
  4. Size the rows, using the column sizes we just calculated (skip if not needed)
  5. Align the tracks
  6. Lay out items into the resulting grid areas

Wouldn't be the step 5 the one that should be removed ? I don't follow why we should remove step 4, neither in the 6 original steps or in the new ones (reduced to 5 steps)

In any case, and with some caution until I had the time to implement a prototype for this change, I think those new steps 1.5 and 2.5 will produce better results and I'm for it. I'd like to know the @MatsPalmgren 's opinion about this, since he was the one creating the issue; besides, I'm curious about how and why he implemented it in the first place, because this is the Firefox's current behavior, right ?

@tabatkins

This comment has been minimized.

Copy link
Member

tabatkins commented May 22, 2018

I was talking about the actual spec, where Step 4 is the "align tracks" step. (Well, it's "size the grid container and align tracks", but obviously we'll keep "size the grid container" around.)

@javifernandez

This comment has been minimized.

Copy link
Contributor

javifernandez commented May 22, 2018

I was talking about the actual spec, where Step 4 is the "align tracks" step. (Well, it's "size the grid container and align tracks", but obviously we'll keep "size the grid container" around.)

I'd be fine with that change, at least give it a try. I think I'll have some time in the coming days to implement some prototypes, just in case we want to wait to see if there are problems with the proposed changes. In any case, I'd like know what @MatsPalmgren thinks before committing the changes.

@javifernandez

This comment has been minimized.

Copy link
Contributor

javifernandez commented May 22, 2018

Given the above, do you agree that it's okay to move content distribution up into new steps 1.5 and 2.5 (and eliminate step 4)?

@tabatkins Moving content-distribution just after step 1 wouldn't be enough to address the issues that @mrego described in #2557 (comment)

@tabatkins

This comment has been minimized.

Copy link
Member

tabatkins commented May 22, 2018

It would. Step 1 would still proceed as currently stated; in @mrego's example, it would still lay out into 200px. Then distribution would happen in the new step 2 (step 1.5, between current step 1 and 2).

The benefit, then, is that if step 3 causes us to cycle back and re-do steps 1 and 2, you do so with the column gaps already taken into account as affecting the width. (Row gaps, in step 2.5, wouldn't have an immediate effect, as no layout is done between step 2 and step 3.)

Contrast this with the current spec, which does the same thing but doesn't let the step-3 cycle take column gaps into account; the column gaps don't appear until after sizing is totally done.

@javifernandez

This comment has been minimized.

Copy link
Contributor

javifernandez commented May 22, 2018

The new changes indeed solve cases like the one @mrego described, thanks to the Step 3, as you mentioned. However, in case of chrome and webkit implementation it only works like that when the grid container has definite inline size.

Perhaps this is an implementation detail, but we only run step 1 of the algorithm in order to determine the preferred width. Once set, we use this preferred width during the layout, so we can't then change it if Step 3 gives a different result for the columns.

@MatsPalmgren

This comment has been minimized.

Copy link
Author

MatsPalmgren commented May 23, 2018

Doing the distribution step immediately after the track sizes are known always leads to better results as far as I can tell. You need both the position and size of the columns to calculate the correct CB inline-size for column-spanning items, and the CB inline-size is required to determine any content-based block-axis contributions for the items. I don't think the proposed change makes any difference to how row sizes are estimated for orthogonal items, since that happens during (or before) step 1.

(I tend to think that the row estimation is suboptimal/wrong in both Chrome and Firefox currently though and that we can improve the heuristic there, but that seems like a separate issue.)

Perhaps this is an implementation detail, but we only run step 1 of the algorithm in order to determine the preferred width. Once set, we use this preferred width during the layout, so we can't then change it if Step 3 gives a different result for the columns.

That's basically what Gecko does too. The proposed change doesn't affect that in any way though, since distribution of space only happens when the grid container has a definite size in that axis (so it doesn't occur during intrinsic sizing).

@anjia

This comment has been minimized.

Copy link

anjia commented May 23, 2018

Frankly speaking, I did not dive into the implementation details about Grid Sizing Algo.

Standing at the point of usage, my preference would be Firefox does correctly which at least meets my expectation and is similar to the existing layout such as Flex (here's a simple demo).

@javifernandez

This comment has been minimized.

Copy link
Contributor

javifernandez commented May 23, 2018

Ok, I think we have an agreement on the proposed changes, so that these new steps 1.5 and 2.5 will produce better results. I also agree that it shouldn't interfere with row estimation in case of orthogonal items, which will be addressed in the extra cycle, as tab mentioned.

That's basically what Gecko does too. The proposed change doesn't affect that in any way though, since distribution of space only happens when the grid container has a definite size in that axis (so it doesn't occur during intrinsic sizing).

Well, I was talking about a case where an orthogonal item spans several rows in a grid with a definite height. For example:

<div style="display: inline-grid; grid-template-rows: 50px 50px; height: 200px;  font: 20px/1 Ahem; justify-content: start; align-content: space-between;">
   <div style="grid-row: span 2; background: cyan; writing-mode: vertical-lr">XXX XX X XX X XXX</div>
</div>

While in Firefox the example above is not rendered correctly because it doesn't perform a estimation of the row size, in Chrome it renders correctly, but column tracks are not computed correctly (at least not as good as we intend with the new steps we added now). The reason is that, as you said, row estimation happens during step 1, hence, it doesn't take Content Alignment into account. Once the preferred width is set, we can't change it, even if the actual rows's size is bigger during layout.

Anyway, I don't think this is a blocker to land the changes in the spec to add the new steps in the sizing algorithm.

@MatsPalmgren

This comment has been minimized.

Copy link
Author

MatsPalmgren commented May 23, 2018

Right, that's what I meant with the current row estimation being suboptimal since it leads to surprising results for simple cases. I filed issue #2697 to improve it.

@w3c w3c deleted a comment from css-meeting-bot May 23, 2018

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 25, 2018

[css-grid] Content Alignment as part of the track sizing algorithm
The CSS WG resolved [1] that Content Alignment should account to the
track sizing algorithm.

The sizing algorithm has been modified so that two new steps (1.5
and 2.5) were added to compute the Content Alignment offsets after
resolving the columns' and rows' sizes respectively.

[1] w3c/csswg-drafts#2557

Added WPT
  * grid-content-distribution-must-account-for-track-sizing-001.html
  * grid-content-distribution-must-account-for-track-sizing-002.html

Bug: 844744
Change-Id: I90019a96b148d3713467e35fde9482d0d39b1ced
@css-meeting-bot

This comment has been minimized.

Copy link
Member

css-meeting-bot commented May 30, 2018

The Working Group just discussed Applying 'justify-content' content distribution is in the wrong place in the overall grid sizing algo, and agreed to the following:

  • RESOLVED: Accept proposed edits in https://github.com/w3c/csswg-drafts/issues/2557#issuecomment-385571268 to amend grid algo
The full IRC log of that discussion <dael> Topic: Applying 'justify-content' content distribution is in the wrong place in the overall grid sizing algo
<dael> github: https://github.com//issues/2557
<fantasai> https://github.com//issues/2557#issuecomment-385571268
<dael> fantasai: Summary is here ^
<dael> fantasai: When we size the grid we do all the sizing account for items etc. and at end align the track or justify them. Problem is that sometimes this increases size of gap so spanning item has more space.
<dael> fantasai: Mats said we should integrate alignment into sizing algo more close so that you have alignment effects on sizing accounted for when doing sizing
<dael> fantasai: Seems like everyone agrees this is a good idea so bringing to WG for approval.
<dael> fantasai: Questions?
<dael> astearns: Comment you linked to has...does it have both 1.5 and 2.5 that are in further discussion?
<dael> fantasai: 1.5 is second sentence in 1.
<dael> astearns: And 2.5 is second sentence in 2?
<dael> fantasai: Yes and so on down.
<dael> astearns: Other comments to add?
<dael> astearns: Objections to accepting https://github.com//issues/2557#issuecomment-385571268
<dael> RESOLVED: Accept proposed edits in https://github.com//issues/2557#issuecomment-385571268 to amend grid algo
<fantasai> https://github.com//issues/2697
<dael> fantasai: Follow-up we have a heuristic to get rough sizes and we need to also change the heuristic
<dael> fantasai: Technically different issue

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 6, 2018

[css-grid] Content Alignment as part of the track sizing algorithm
The CSS WG resolved [1] that Content Alignment should account to the
track sizing algorithm.

The sizing algorithm has been modified so that two new steps (1.5
and 2.5) were added to compute the Content Alignment offsets after
resolving the columns' and rows' sizes respectively.

[1] w3c/csswg-drafts#2557

Added WPT
  * grid-content-distribution-must-account-for-track-sizing-001.html
  * grid-content-distribution-must-account-for-track-sizing-002.html

Bug: 844744
Change-Id: I90019a96b148d3713467e35fde9482d0d39b1ced

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 6, 2018

[css-grid] Content Alignment as part of the track sizing algorithm
The CSS WG resolved [1] that Content Alignment should account to the
track sizing algorithm.

The sizing algorithm has been modified so that two new steps (1.5
and 2.5) were added to compute the Content Alignment offsets after
resolving the columns' and rows' sizes respectively.

[1] w3c/csswg-drafts#2557

Added WPT
  * grid-content-distribution-must-account-for-track-sizing-001.html
  * grid-content-distribution-must-account-for-track-sizing-002.html

Bug: 844744
Change-Id: I90019a96b148d3713467e35fde9482d0d39b1ced

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 6, 2018

[css-grid] Content Alignment as part of the track sizing algorithm
The CSS WG resolved [1] that Content Alignment should account to the
track sizing algorithm.

The sizing algorithm has been modified so that two new steps (1.5
and 2.5) were added to compute the Content Alignment offsets after
resolving the columns' and rows' sizes respectively.

[1] w3c/csswg-drafts#2557

Added WPT
  * grid-content-distribution-must-account-for-track-sizing-001.html
  * grid-content-distribution-must-account-for-track-sizing-002.html

Bug: 844744
Change-Id: I90019a96b148d3713467e35fde9482d0d39b1ced

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 7, 2018

[css-grid] Content Alignment as part of the track sizing algorithm
The CSS WG resolved [1] that Content Alignment should account to the
track sizing algorithm.

The sizing algorithm has been modified so that two new steps (1.5
and 2.5) were added to compute the Content Alignment offsets after
resolving the columns' and rows' sizes respectively.

[1] w3c/csswg-drafts#2557

Added WPT
  * grid-content-distribution-must-account-for-track-sizing-001.html
  * grid-content-distribution-must-account-for-track-sizing-002.html

Bug: 844744
Change-Id: I90019a96b148d3713467e35fde9482d0d39b1ced
@fantasai

This comment has been minimized.

Copy link
Contributor

fantasai commented Jun 12, 2018

OK, I edited in a first pass at incorporating this change in 277817b and c9e9f7f

@MatsPalmgren @javifernandez @mrego Would you mind looking this over to make sure it adequately captures what we've discussed?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 12, 2018

[css-grid] Content Alignment as part of the track sizing algorithm
The CSS WG resolved [1] that Content Alignment should account to the
track sizing algorithm.

The sizing algorithm has been modified so that two new steps (1.5
and 2.5) were added to compute the Content Alignment offsets after
resolving the columns' and rows' sizes respectively.

This change decouples the Content Alignment logic from the tracks
position, so that we can use it as part of the track sizing algorithm.

I also had to store the whole ContentAlignmentData structure in two
class attributes. We need both, position and distribution offsets, to
be used in different parts of the layout logic.

Finally, since the ContentAlignmentData is now used as persistent
memory, we had to remove the STACK_ALLOCATED flag; hence, I think
using DISALLOW_COPY_AND_ASSIGN and let the functions update the
structure's internal field is a more efficient approach to commit
this change.

[1] w3c/csswg-drafts#2557

Added WPT
  * grid-content-distribution-must-account-for-track-sizing-001.html
  * grid-content-distribution-must-account-for-track-sizing-002.html

Bug: 844744
Change-Id: I90019a96b148d3713467e35fde9482d0d39b1ced

aarongable pushed a commit to chromium/chromium that referenced this issue Jun 12, 2018

[css-grid] Content Alignment as part of the track sizing algorithm
The CSS WG resolved [1] that Content Alignment should account to the
track sizing algorithm.

The sizing algorithm has been modified so that two new steps (1.5
and 2.5) were added to compute the Content Alignment offsets after
resolving the columns' and rows' sizes respectively.

This change decouples the Content Alignment logic from the tracks
position, so that we can use it as part of the track sizing algorithm.

I also had to store the whole ContentAlignmentData structure in two
class attributes. We need both, position and distribution offsets, to
be used in different parts of the layout logic.

Finally, since the ContentAlignmentData is now used as persistent
memory, we had to remove the STACK_ALLOCATED flag; hence, I think
using DISALLOW_COPY_AND_ASSIGN and let the functions update the
structure's internal field is a more efficient approach to commit
this change.

[1] w3c/csswg-drafts#2557

Added WPT
  * grid-content-distribution-must-account-for-track-sizing-001.html
  * grid-content-distribution-must-account-for-track-sizing-002.html

Bug: 844744
Change-Id: I90019a96b148d3713467e35fde9482d0d39b1ced
Reviewed-on: https://chromium-review.googlesource.com/1067918
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Manuel Rego Casasnovas <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#566412}

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 12, 2018

[css-grid] Content Alignment as part of the track sizing algorithm
The CSS WG resolved [1] that Content Alignment should account to the
track sizing algorithm.

The sizing algorithm has been modified so that two new steps (1.5
and 2.5) were added to compute the Content Alignment offsets after
resolving the columns' and rows' sizes respectively.

This change decouples the Content Alignment logic from the tracks
position, so that we can use it as part of the track sizing algorithm.

I also had to store the whole ContentAlignmentData structure in two
class attributes. We need both, position and distribution offsets, to
be used in different parts of the layout logic.

Finally, since the ContentAlignmentData is now used as persistent
memory, we had to remove the STACK_ALLOCATED flag; hence, I think
using DISALLOW_COPY_AND_ASSIGN and let the functions update the
structure's internal field is a more efficient approach to commit
this change.

[1] w3c/csswg-drafts#2557

Added WPT
  * grid-content-distribution-must-account-for-track-sizing-001.html
  * grid-content-distribution-must-account-for-track-sizing-002.html

Bug: 844744
Change-Id: I90019a96b148d3713467e35fde9482d0d39b1ced
Reviewed-on: https://chromium-review.googlesource.com/1067918
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Manuel Rego Casasnovas <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#566412}

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Jun 12, 2018

[css-grid] Content Alignment as part of the track sizing algorithm
The CSS WG resolved [1] that Content Alignment should account to the
track sizing algorithm.

The sizing algorithm has been modified so that two new steps (1.5
and 2.5) were added to compute the Content Alignment offsets after
resolving the columns' and rows' sizes respectively.

This change decouples the Content Alignment logic from the tracks
position, so that we can use it as part of the track sizing algorithm.

I also had to store the whole ContentAlignmentData structure in two
class attributes. We need both, position and distribution offsets, to
be used in different parts of the layout logic.

Finally, since the ContentAlignmentData is now used as persistent
memory, we had to remove the STACK_ALLOCATED flag; hence, I think
using DISALLOW_COPY_AND_ASSIGN and let the functions update the
structure's internal field is a more efficient approach to commit
this change.

[1] w3c/csswg-drafts#2557

Added WPT
  * grid-content-distribution-must-account-for-track-sizing-001.html
  * grid-content-distribution-must-account-for-track-sizing-002.html

Bug: 844744
Change-Id: I90019a96b148d3713467e35fde9482d0d39b1ced
Reviewed-on: https://chromium-review.googlesource.com/1067918
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Manuel Rego Casasnovas <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#566412}
@javifernandez

This comment has been minimized.

Copy link
Contributor

javifernandez commented Jun 14, 2018

@MatsPalmgren @javifernandez @mrego Would you mind looking this over to make sure it adequately captures what we've discussed?

The changes look good to me.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 11, 2018

Bug 1463665 [wpt PR 11115] - [css-grid] Content Alignment as part of …
…the track sizing algorithm, a=testonly

Automatic update from web-platform-tests[css-grid] Content Alignment as part of the track sizing algorithm

The CSS WG resolved [1] that Content Alignment should account to the
track sizing algorithm.

The sizing algorithm has been modified so that two new steps (1.5
and 2.5) were added to compute the Content Alignment offsets after
resolving the columns' and rows' sizes respectively.

This change decouples the Content Alignment logic from the tracks
position, so that we can use it as part of the track sizing algorithm.

I also had to store the whole ContentAlignmentData structure in two
class attributes. We need both, position and distribution offsets, to
be used in different parts of the layout logic.

Finally, since the ContentAlignmentData is now used as persistent
memory, we had to remove the STACK_ALLOCATED flag; hence, I think
using DISALLOW_COPY_AND_ASSIGN and let the functions update the
structure's internal field is a more efficient approach to commit
this change.

[1] w3c/csswg-drafts#2557

Added WPT
  * grid-content-distribution-must-account-for-track-sizing-001.html
  * grid-content-distribution-must-account-for-track-sizing-002.html

Bug: 844744
Change-Id: I90019a96b148d3713467e35fde9482d0d39b1ced
Reviewed-on: https://chromium-review.googlesource.com/1067918
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Manuel Rego Casasnovas <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#566412}

--

wpt-commits: 73ed379a1437407749c730239019bd61528b060b
wpt-pr: 11115

mykmelez pushed a commit to mozilla/gecko that referenced this issue Jul 13, 2018

Bug 1463665 [wpt PR 11115] - [css-grid] Content Alignment as part of …
…the track sizing algorithm, a=testonly

Automatic update from web-platform-tests[css-grid] Content Alignment as part of the track sizing algorithm

The CSS WG resolved [1] that Content Alignment should account to the
track sizing algorithm.

The sizing algorithm has been modified so that two new steps (1.5
and 2.5) were added to compute the Content Alignment offsets after
resolving the columns' and rows' sizes respectively.

This change decouples the Content Alignment logic from the tracks
position, so that we can use it as part of the track sizing algorithm.

I also had to store the whole ContentAlignmentData structure in two
class attributes. We need both, position and distribution offsets, to
be used in different parts of the layout logic.

Finally, since the ContentAlignmentData is now used as persistent
memory, we had to remove the STACK_ALLOCATED flag; hence, I think
using DISALLOW_COPY_AND_ASSIGN and let the functions update the
structure's internal field is a more efficient approach to commit
this change.

[1] w3c/csswg-drafts#2557

Added WPT
  * grid-content-distribution-must-account-for-track-sizing-001.html
  * grid-content-distribution-must-account-for-track-sizing-002.html

Bug: 844744
Change-Id: I90019a96b148d3713467e35fde9482d0d39b1ced
Reviewed-on: https://chromium-review.googlesource.com/1067918
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Manuel Rego Casasnovas <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#566412}

--

wpt-commits: 73ed379a1437407749c730239019bd61528b060b
wpt-pr: 11115
@mrego

This comment has been minimized.

Copy link
Member

mrego commented Aug 3, 2018

One question about the final text in the spec, we still need the last step (now number 5):

  1. Finally, the grid container is sized using the resulting size of the grid as its content size, and the tracks are aligned within the grid container according to the align-content and justify-content properties.

If content alignment was already done as part of the previous steps, why we still need it?

@fantasai

This comment has been minimized.

Copy link
Contributor

fantasai commented Aug 24, 2018

@mrego The previous steps calculate the effective column/row gaps taking content alignment into account, but they don't actually apply alignment e.g. centering to the tracks.

(Of course if in your implementation you've worked this into your algorithm earlier, you don't need to do it at this point, but it's generally true that CSS implementations don't need to follow the steps in the spec so long as they return the expected output.)

@fantasai fantasai closed this Aug 24, 2018

kisg pushed a commit to paul99/webkit-mips that referenced this issue Nov 6, 2018

jfernandez@igalia.com
CSS grid elements with justify-content: space-around have extra white…
…space, sometimes a lot

https://bugs.webkit.org/show_bug.cgi?id=191308

Reviewed by Dean Jackson.

LayoutTests/imported/w3c:

Imported WPT to cover the behavior changes added in this patch.

* resources/import-expectations.json:
* web-platform-tests/css/css-grid/layout-algorithm/grid-content-distribution-must-account-for-track-sizing-001-expected.txt: Added.
* web-platform-tests/css/css-grid/layout-algorithm/grid-content-distribution-must-account-for-track-sizing-001.html: Added.
* web-platform-tests/css/css-grid/layout-algorithm/grid-content-distribution-must-account-for-track-sizing-002-expected.txt: Added.
* web-platform-tests/css/css-grid/layout-algorithm/grid-content-distribution-must-account-for-track-sizing-002.html: Added.
* web-platform-tests/css/css-grid/layout-algorithm/grid-content-distribution-must-account-for-track-sizing-003-expected.txt: Added.
* web-platform-tests/css/css-grid/layout-algorithm/grid-content-distribution-must-account-for-track-sizing-003.html: Added.
* web-platform-tests/css/css-grid/layout-algorithm/grid-content-distribution-must-account-for-track-sizing-004-expected.txt: Added.
* web-platform-tests/css/css-grid/layout-algorithm/grid-content-distribution-must-account-for-track-sizing-004.html: Added.
* web-platform-tests/css/css-grid/layout-algorithm/w3c-import.log:

Source/WebCore:

The CSS WG resolved [1] that Content Alignment should account to the
track sizing algorithm.

The sizing algorithm has been modified so that two new steps (1.5
and 2.5) were added to compute the Content Alignment offsets after
resolving the columns' and rows' sizes respectively.

This change decouples the Content Alignment logic from the tracks
position, so that we can use it as part of the track sizing algorithm.

I also had to store the whole ContentAlignmentData structure in two
class attributes. We need both, position and distribution offsets, to
be used in different parts of the layout logic.

[1] w3c/csswg-drafts#2557

Tests: imported/w3c/web-platform-tests/css/css-grid/layout-algorithm/grid-content-distribution-must-account-for-track-sizing-001.html
       imported/w3c/web-platform-tests/css/css-grid/layout-algorithm/grid-content-distribution-must-account-for-track-sizing-002.html
       imported/w3c/web-platform-tests/css/css-grid/layout-algorithm/grid-content-distribution-must-account-for-track-sizing-003.html
       imported/w3c/web-platform-tests/css/css-grid/layout-algorithm/grid-content-distribution-must-account-for-track-sizing-004.html
       imported/w3c/web-platform-tests/css/css-grid/layout-algorithm/grid-percent-cols-filled-shrinkwrap-001.html
       imported/w3c/web-platform-tests/css/css-grid/layout-algorithm/grid-percent-cols-spanned-shrinkwrap-001.html
       imported/w3c/web-platform-tests/css/css-grid/layout-algorithm/grid-percent-rows-filled-shrinkwrap-001.html
       imported/w3c/web-platform-tests/css/css-grid/layout-algorithm/grid-percent-rows-spanned-shrinkwrap-001.html

* rendering/GridTrackSizingAlgorithm.cpp:
(WebCore::GridTrackSizingAlgorithm::gridAreaBreadthForChild const):
* rendering/RenderGrid.cpp:
(WebCore::RenderGrid::repeatTracksSizingIfNeeded):
(WebCore::RenderGrid::layoutBlock):
(WebCore::RenderGrid::gridItemOffset const):
(WebCore::RenderGrid::trackSizesForComputedStyle const):
(WebCore::RenderGrid::populateGridPositionsForDirection):
(WebCore::RenderGrid::gridAreaBreadthForOutOfFlowChild):
(WebCore::contentDistributionOffset):
(WebCore::RenderGrid::computeContentPositionAndDistributionOffset):
(WebCore::RenderGrid::nonCollapsedTracks const):
* rendering/RenderGrid.h:
(WebCore::ContentAlignmentData::isValid):
(WebCore::ContentAlignmentData::defaultOffsets):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@237884 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@fantasai fantasai added this to the css-grid-1 CR 2017-12-14+ milestone Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.