Closed Bug 1335806 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect, P1)

52 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.)
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. :-)
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
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.
Attachment #8847813 - Flags: review?(dholbert)
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
Attachment #8847815 - Flags: review?(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.
Attachment #8847810 - Flags: review?(dholbert) → review+
Summary: [css-grid] Make 'align/justify-self:normal' behave as 'start' for replaced and intrinsic ratio elements → [css-grid] Make 'align/justify-self:normal' behave as 'start' for elements with an intrinsic size or aspect ratio
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
Attachment #8847813 - Flags: review?(dholbert) → review+
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
Attachment #8847815 - Flags: review?(dholbert) → review+
Pushed by mpalmgren@mozilla.com:
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.
Flags: in-testsuite+
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)
Attachment #8847810 - Flags: approval-mozilla-beta?
Attachment #8847810 - Flags: approval-mozilla-aurora?
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+.
Attachment #8847810 - Flags: approval-mozilla-beta?
Attachment #8847810 - Flags: approval-mozilla-beta+
Attachment #8847810 - Flags: approval-mozilla-aurora?
Attachment #8847810 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.