1008969 - [css-grid] add nsGridContainerFrame::Reflow and a few grid related nsHTMLReflowState additions

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 0, make nsGridContainerFrame final for now, and remove some unnecessary boilerplate code, etc

I don't think the nsGridContainerFrameSuper pattern adds any value
in general and it makes finding call sites using 'grep' harder so
I'm changing it to use nsContainerFrame directly.

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1, Initialize the nsHTMLReflowState "frame type" correctly for display:grid/inline-grid. Add nsIFrame::IsFlexOrGridItem(), and use it to share some flexbox code in nsHTMLReflowState

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 2, Add a basic nsGridContainerFrame::Reflow() method

(the contents of this method will soon be mostly replaced so don't
put to much effort into reviewing it)

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8421023 [details] [diff] [review]
part 2, Add a basic nsGridContainerFrame::Reflow() method

>+nsresult
>+nsGridContainerFrame::Reflow(nsPresContext*           aPresContext,
>+                             nsHTMLReflowMetrics&     aDesiredSize,
>+                             const nsHTMLReflowState& aReflowState,
>+                             nsReflowStatus&          aStatus)
>+{

This should return void, now that bug 1008917 has landed, right?

>+  nscoord contentHeight = GetEffectiveComputedHeight(aReflowState);
>+  if (contentHeight == NS_AUTOHEIGHT) {
>+    contentHeight = 0;
>+  }

Nit: you technically need to clamp contentHeight to be at least as large as the min-height, in the NS_AUTOHEIGHT case. (I'm fine with that being addressed in the subsequent patch that replaces this code, though.)

>+  aStatus = NS_FRAME_COMPLETE;
>+  aDesiredSize.mCarriedOutBottomMargin.Zero();

I don't think mCarriedOutBottomMargin is (or will become) relevant here, and it's initialized to zero by default, so probably just drop this line.

>+  NS_FRAME_SET_TRUNCATION(aStatus, aReflowState, aDesiredSize);
>+  return NS_OK;
>+}

Just to finish out the boilerplate -- I think we also need a call to...
   FinishReflowWithAbsoluteFrames(aPresContext, aDesiredSize,
                                  aReflowState, aStatus);
...towards the end of the function.

If that's coming in your next version of this function, great (though IMHO it'd make more sense to add it here, since it's not really grid-specific and is boilerplate-ish.)

r=me with the above addressed as you see fit.

Comment 5

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> This should return void, now that bug 1008917 has landed, right?

Yes, I wrote this before that bug - thanks for the reminder though!

> I don't think mCarriedOutBottomMargin is (or will become) relevant here, and
> it's initialized to zero by default, so probably just drop this line.

True, I dropped it.

> Just to finish out the boilerplate -- I think we also need a call to...
>    FinishReflowWithAbsoluteFrames(aPresContext, aDesiredSize,
>                                   aReflowState, aStatus);
> ...towards the end of the function.
> 
> If that's coming in your next version of this function, great (though IMHO
> it'd make more sense to add it here, since it's not really grid-specific and
> is boilerplate-ish.)

I've looked into to reflowing abs.pos. children a bit, and it's probably not
going to be boilerplate (since each child needs a separate containing block).
Thanks for the advice though, but I think I'll skip this for now.

Comment 6

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad82809e623
https://hg.mozilla.org/integration/mozilla-inbound/rev/7048bc41993f
https://hg.mozilla.org/integration/mozilla-inbound/rev/46cf8e706620

Comment 7

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

(In reply to Mats Palmgren (:mats) from comment #6)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/46cf8e706620

This cset was backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/39d9450b3d60 because dholbert says it depended on bug 1008917, which was backed out.

Comment 8

[Daniel Holbert [:dholbert]]

[marking leave-open for the time being, so this doesn't get auto-closed when the first two parts are merged to central]

Comment 9

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

The failure that caused bug 1008917 to be backed out happened on retriggers prior to 1008917 landed, so once it relands, this can reland, too.

Comment 10

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7e1b2d6408

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ad82809e623
https://hg.mozilla.org/mozilla-central/rev/7048bc41993f
https://hg.mozilla.org/mozilla-central/rev/ac7e1b2d6408