1335806 - [css-grid] Make 'align/justify-self:normal' behave as 'start' for elements with an intrinsic size or aspect ratio

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Mats Palmgren (inactive)]

It appears the Vogons^W CSSWG has decided that some grid items should treat
'normal' as 'start' (and not do any stretching by default).
https://log.csswg.org/irc.w3.org/css/2017-01-13/#e761792
"<gregwhitworth> Rossen: ok, any objections to resolve on option number #1: no change to the default sizing, non replace get stretched, replaced gets start and add new sizing keywords to address the issues"

There is some confusion about what was actually decided though, because:
"RESOLVED: We're keeping the current specified behavior as it is"
contradicts the above, since the "current specified behavior" is that
only items with an intrinsic ratio should treat 'normal as 'start'.
https://drafts.csswg.org/css-align-3/#justify-grid

The IRC discussion of "number #1" seems to be centered around "replaced items"
though, so I'm assuming the first statement is correct.
I have asked for clarification:
https://github.com/w3c/csswg-drafts/issues/523#issuecomment-275869984

(It appears that requests for clarification needs to be "signed in triplicate, sent in,
sent back, queried, lost, found, subjected to public inquiry, lost again, and finally
buried in soft peat for three months and recycled as firelighters" before one can
actually get a comprehensible answer though.)

Comment 1

[Mats Palmgren (inactive)]

So now someone else made a different summary of the resolution:
"
      - RESOLVED: We're keeping the current specified behavior as it
                  is (no change to the default sizing, non replace
                  get stretched, replaced gets start and add new
                  sizing keywords to address the issues)
"
https://lists.w3.org/Archives/Public/www-style/2017Feb/0061.html

I like that better so I'm going with that. :-)

Comment 2

[Mats Palmgren (inactive)]

https://hg.csswg.org/drafts/rev/e9e085454fcc

Comment 3

[Mats Palmgren (inactive)]

The CSS Grid spec now says:
"Otherwise, if the grid item has an intrinsic ratio or an intrinsic size in the relevant dimension,
the grid item is sized as for align-self: start"
https://drafts.csswg.org/css-grid/#grid-item-sizing

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 1 - Move the intrinsic size/ratio calculations up a bit (idempotent patch).

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 2 - Refactor the intrinsic size calculations a bit (idempotent patch)

Just imporving the code style a bit, instead of:
  if (bsizeCoord.GetUnit() == eStyleUnit_Coord) {
    hasIntrinsicBSize = true;
  } else {
    hasIntrinsicBSize = false;
  }

I'm doing:
hasIntrinsicBSize = bsizeCoord.GetUnit() == eStyleUnit_Coord;
if (hasIntrinsicBSize)  etc

and moving some variable declarations to make two independent
code blocks, first the isize stuff, then the bsize stuff.

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Make 'align/justify-self:normal' behave as 'start' for grid items that have an intrinsic size or aspect ratio

I'm leaving the code for handling eStretchPreservingRatio in, because
I think we can use that when implementing 'width/height:contain' eventually:
https://github.com/w3c/csswg-drafts/issues/934

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid] Tweak reftests where 'align/justify-self:normal' now means 'start' for grid items with an intrinsic size / aspect ratio

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f4217b15994143c42a4c339c759268d372528e5

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8847810 [details] [diff] [review]
part 1 - Move the intrinsic size/ratio calculations up a bit (idempotent patch).

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

r=me, with one comment nit:

::: layout/generic/nsFrame.cpp
@@ -5316,4 @@
>      minBSize = 0;
>    }
>  
>    // Resolve percentage intrinsic iSize/bSize as necessary:

This comment (just before the moved block) probably needs to be deleted.

Otherwise it's being left a bit awkward:
  // Resolve percentage intrinsic iSize/bSize as necessary:
  NS_ASSERTION(stuff);

  // Now calculate the used values for iSize and bSize:

It seems like the comments are implying that the NS_ASSERTION is resolving the percentage intrinsic iSize/bSize, but clearly it's not. :)

Both of these comments date back to this patch:
http://searchfox.org/mozilla-central/diff/d308285bb4fd504528af256706555c0d7c4969d1/layout/base/nsLayoutUtils.cpp#1816
...at which point there was some percent-resolution going on in the old version of the code that you're moving. But now there's not, really, so this comment should probably just go away.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8847813 [details] [diff] [review]
part 2 - Refactor the intrinsic size calculations a bit (idempotent patch)

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

r=me

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8847815 [details] [diff] [review]
part 3 - [css-grid] Make 'align/justify-self:normal' behave as 'start' for grid items that have an intrinsic size or aspect ratio

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

r=me

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a07c5822e83
part 1 - Move the intrinsic size/ratio calculations up a bit (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6ec480844fc
part 2 - Refactor the intrinsic size calculations a bit (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a64601d6efc5
part 3 - [css-grid] Make 'align/justify-self:normal' behave as 'start' for grid items that have an intrinsic size or aspect ratio.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2977f445119b
part 4 - [css-grid] Tweak reftests where 'align/justify-self:normal' now means 'start' for grid items with an intrinsic size / aspect ratio.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3a07c5822e83
https://hg.mozilla.org/mozilla-central/rev/f6ec480844fc
https://hg.mozilla.org/mozilla-central/rev/a64601d6efc5
https://hg.mozilla.org/mozilla-central/rev/2977f445119b

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8847810 [details] [diff] [review]
part 1 - Move the intrinsic size/ratio calculations up a bit (idempotent patch).

Approval Request Comment
[Feature/Bug causing the regression]: CSS Grid spec changes
[User impact if declined]: errors in CSS Grid layout involving image/video items; it'd be nice to have this for compliance with the latest Grid spec
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: the first two parts have no functional changes; part 3 is what fixes the bug and the two chunks there are inside "if (MOZ_UNLIKELY(isGridItem))" blocks so can't affect anything other than grid layout
[String changes made/needed]: none

(the uplift request is for all parts)

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8847810 [details] [diff] [review]
part 1 - Move the intrinsic size/ratio calculations up a bit (idempotent patch).

Fix compliance with the latest Grid spec and also fix the tests. Aurora54+ & Beta53+.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/08f74b1607bc
https://hg.mozilla.org/releases/mozilla-aurora/rev/6e8e5292e77a
https://hg.mozilla.org/releases/mozilla-aurora/rev/1520c23b33e5
https://hg.mozilla.org/releases/mozilla-aurora/rev/01942ae38375

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bc67e63667ef
https://hg.mozilla.org/releases/mozilla-beta/rev/77fad2f16297
https://hg.mozilla.org/releases/mozilla-beta/rev/1aca71052d1f
https://hg.mozilla.org/releases/mozilla-beta/rev/b962f6a14a7a