1316051 - [css-grid] align-self/justify-self:stretch/normal doesn't work on <table> grid items

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Attachment: grid-item-table-stretch-001.html

Looks like we need to do something special to stretch <table> items.

Automatic Minimum Size clamping doesn't work either but I'm not sure
we should actually clamp tables below the min-content size that
falls out from the table algorithm.  I.e. comments like this make
me doubt we should clamp them at all:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/layout/base/nsLayoutUtils.cpp#5085

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - Make ChildShrinkWrapISize a method instead of a static function, to prepare for changes in the next patch (idempotent change).

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Don't shrink-wrap child frames when the table is a stretched grid item.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Store an explicit CB size that were given to the table-wrapper on a frame property. Propagate that (minus margins) to future ReflowInputs for the inner table frame.

This is the tricky bit.  We need to store the aContainingBlockSize that the grid
container calculated for the table-wrapper (the grid item child frame) and use
that (minus margins) for the inner table frame too.

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid][css-flexbox] Use the alignment container of the table-wrapper frame also for its child frames.

I think this is the correct fix also for table flex items, so I'm changing
both here.

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-grid] Subtract the caption size from the CB size that is used for stretching table items.

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 6 - [css-grid] Reftests for <table> grid items.

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 7 - [css-grid] Reftests for <fieldset> grid items.

(just some bonus tests for other elements while I'm at it)

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 8 - [css-grid] Reftests for <video> grid items.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8810258 [details] [diff] [review]
part 1 - Make ChildShrinkWrapISize a method instead of a static function, to prepare for changes in the next patch (idempotent change).

r=me

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8810259 [details] [diff] [review]
part 2 - [css-grid] Don't shrink-wrap child frames when the table is a stretched grid item.

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

::: layout/tables/nsTableWrapperFrame.cpp
@@ +386,5 @@
> +  auto flags = ComputeSizeFlags::eShrinkWrap;
> +  auto parent = GetParent();
> +  nsIAtom* parentFrameType = parent ? parent->GetType() : nullptr;
> +  bool isGridItem = (parentFrameType == nsGkAtoms::gridContainerFrame &&
> +                     !(GetStateBits() & NS_FRAME_OUT_OF_FLOW));

Consider using...
  !HasAnyStateBits(NS_FRAME_OUT_OF_FLOW)
...instead of...
  !(GetStateBits() & NS_FRAME_OUT_OF_FLOW)
for brevity, reduced parens-nesting, and improved human-readability/foolproofing.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #3)
> This is the tricky bit.  We need to store the aContainingBlockSize that the grid
> container calculated for the table-wrapper (the grid item child frame) and use
> that (minus margins) for the inner table frame too.

Hmm... It's a bit unfortunate that we have to clutter up ReflowInput with all this extremely-special-case grid+table-wrapper custom stuff.  And it's unfortunate that we're adding an additional way of overriding the containing block size in ReflowInput.

ReflowInput already has a way to pass in a custom containing block -- it seems like we should make use of that, if we can.  I suppose we don't know *what* to pass there, since the wrapper's ReflowInput doesn't store its aContainingBlockSize persistently for us to be able to re-use.

Could we do the following:
 (1) In the *one and only* case where Grid passes in a custom CB (for childRI in nsGridContainerFrame::ReflowInFlowChild): add a special case to check the child type & set nsTableWrapperFrame::GridItemCBSizeProperty() if the child is a table-wrapper.

 (2) In nsTableWrapperFrame::InitChildReflowInput(), add a special-case to do what you've currently got in ReflowInput.cpp -- something like this:

   LogicalSize* cbSize = nullptr;
   if (aReflowInput.mFrame == InnerTableFrame()) {
     const auto& props = mFrame->Properties();
     cbSize = props.Get(nsTableWrapperFrame::GridItemCBSizeProperty());
     if (cbSize) {
       // Subtract off margin from the wrapper frame, via aReflowInput.mParentReflowInput.ComputedLogicalMargin()
     }

 (3) ...and then update the existing aReflowInput.Init(...) call in nsTableWrapperFrame::InitChildReflowInput to pass cbSize instead of nullptr as its second argument.

Would that work? I think it'd be about the same amount of code-changes, but has these benefits:
 - it uses the existing ReflowInput cbSize-override mechanism, rather than introducing a new & conflicting way of doing that override.
 - it makes the code changes for this scenario in the frame classes that are involved (grid & table) rather than in general code (ReflowInput).

Comment 12

[Mats Palmgren (inactive)]

Attachment: [css-grid] Store the CB size that we give to table-wrappers on a frame property, then propagate that later (minus margins) to the inner table frame.

Sure, that works.  The drawback is that each caller needs to be aware of this
and do it manually, rather having a generic solution.  But I guess we can
move it back to ReflowInput later if deemed convenient for other container
types that needs to implement 'stretch' on their children.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8811983 [details] [diff] [review]
[css-grid] Store the CB size that we give to table-wrappers on a frame property, then propagate that later (minus margins) to the inner table frame.

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

Thanks - r=me, one trivial nit:

::: layout/tables/nsTableWrapperFrame.h
@@ +180,5 @@
>    }
>  
> +  /**
> +   * The CB size to use for the inner table frame if we're a grid item.
> +   * @see ReflowInput::InitCBReflowInput

This @see reference needs updating. (since you're not changing ReflowInput::InitCBReflowInput anymore).

Probably simplest to have "@see nsTableWrapperFrame::InitChildReflowInput" (or just drop the @see entirely and let people grep for GridItemCBSizeProperty if they want to learn more.)

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8810261 [details] [diff] [review]
part 4 - [css-grid][css-flexbox] Use the alignment container of the table-wrapper frame also for its child frames.

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

r=me

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8810262 [details] [diff] [review]
part 5 - [css-grid] Subtract the caption size from the CB size that is used for stretching table items.

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

::: layout/tables/nsTableWrapperFrame.cpp
@@ +947,2 @@
>        nscoord captionBSize = 0;
> +      nscoord captionISize = 0;

Could you rename these variables to something clearer (perhaps in a followup patch)?

Right now we have these in scope, which are really easy to confuse:
 - nscoord     captionBSize & captionISize
 - LogicalSize captionSize (which has ISize() / BSize() members)

The former variables are *really* "{B,I}Size that we need to reserve for the caption".  (And one of them is always 0, which is confusing with their current naming, since the caption isn't actually 0-size in that dimension.)

How about we rename these to something like bSizeReservedForCaption & iSizeReservedForCaption? (Or something else that makes it more intuitive why one of these is 0, despite the fact that the caption itself is not 0-size.)

@@ +962,5 @@
> +        innerRI->AvailableBSize() =
> +          std::max(0, innerRI->AvailableBSize() - captionBSize);
> +      }
> +      if (cbSize && ((captionBSize > 0 && cbSize->BSize(wm) > 0) ||
> +                     (captionISize > 0 && cbSize->ISize(wm) > 0))) {

Do we really need the cbSize->BSize / cbSize->ISize checks here? (Is cbSize->BSize/ISize==0  expected to be a common case?)

I think you're really aiming to check whether cbSize actually changes or not (so we only restart reflow if we need to).  Seems like it'd be better to just directly check for that, like so:

 if (cbSize) {
   LogicalSize oldCBSize = *cbSize;
   cbSize->ISize(wm) = std::max(0, cbSize->ISize(wm) - captionISize);
   cbSize->BSize(wm) = std::max(0, cbSize->BSize(wm) - captionBSize);
   if (oldCBSize != *cbSize) {
     // Reset the inner table's [...]
   }
 }

Comment 16

[Mats Palmgren (inactive)]

> How about we rename these to something like bSizeReservedForCaption & iSizeReservedForCaption? 

Meh, too long; it would push several lines > 80 columns and then the code
is less readable due to that, so overall it's not much of an improvement.
They are only used in relatively short scope and it's easy to read from
the code what the actually mean IMO.

I rephrased my added comment though to help clarify the meaning:
+        // Shrink the CB size by the size reserved for the caption.

>  if (cbSize) { ...

Yes, this is better, thanks.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7da20e00b2ff
part 1 - Make ChildShrinkWrapISize a method instead of a static function, to prepare for changes in the next patch (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/333774e147ee
part 2 - [css-grid] Don't shrink-wrap child frames when the table is a stretched grid item.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4719fb74384
part 3 - [css-grid] Store the CB size that we give to table-wrappers on a frame property, then propagate that later (minus margins) to the inner table frame.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1c1dc37eb5
part 4 - [css-grid][css-flexbox] Use the alignment container of the table-wrapper frame also for its child frames.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/70a0b4ff9d08
part 5 - [css-grid] Subtract the caption size from the CB size that is used for stretching table items.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/728c0feff623
part 6 - [css-grid] Reftests for <table> grid items.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6d26f6de35
part 7 - [css-grid] Reftests for <fieldset> grid items.
https://hg.mozilla.org/integration/mozilla-inbound/rev/33055c246eb3
part 8 - [css-grid] Reftests for <video> grid items.

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #16)
> > How about we rename these to something like bSizeReservedForCaption & iSizeReservedForCaption? 
> 
> Meh, too long; it would push several lines > 80 columns and then the code
> [...]
> I rephrased my added comment though to help clarify the meaning:
> +        // Shrink the CB size by the size reserved for the caption.

Thanks for the commment-tweak.  Maybe "reservedISize" "reservedBSize" would be clearer as variable-names? (while still being pretty short) *shrug*  I suppose they're fine as-is, too.

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 8811983 [details] [diff] [review]
[css-grid] Store the CB size that we give to table-wrappers on a frame property, then propagate that later (minus margins) to the inner table frame.

Approval Request Comment
[Feature/regressing bug #]: CSS Grid
[User impact if declined]: web-compat risk
[Describe test coverage new/current, TreeHerder]: have reftests, all tests pass
[Risks and why]: low risk
[String/UUID change made/needed]: none

This is part of the CSS Grid release, please uplift to Aurora 52, thanks.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7da20e00b2ff
https://hg.mozilla.org/mozilla-central/rev/333774e147ee
https://hg.mozilla.org/mozilla-central/rev/e4719fb74384
https://hg.mozilla.org/mozilla-central/rev/ca1c1dc37eb5
https://hg.mozilla.org/mozilla-central/rev/70a0b4ff9d08
https://hg.mozilla.org/mozilla-central/rev/728c0feff623
https://hg.mozilla.org/mozilla-central/rev/1f6d26f6de35
https://hg.mozilla.org/mozilla-central/rev/33055c246eb3

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 8811983 [details] [diff] [review]
[css-grid] Store the CB size that we give to table-wrappers on a frame property, then propagate that later (minus margins) to the inner table frame.

css-grid fixes for aurora52

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2006a7b09d2a
https://hg.mozilla.org/releases/mozilla-aurora/rev/b124474728a9
https://hg.mozilla.org/releases/mozilla-aurora/rev/4ac4af2ab114
https://hg.mozilla.org/releases/mozilla-aurora/rev/5d48cde12383
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d566e0afb53
https://hg.mozilla.org/releases/mozilla-aurora/rev/d4c4c6c42b1f
https://hg.mozilla.org/releases/mozilla-aurora/rev/b6a5fa467365
https://hg.mozilla.org/releases/mozilla-aurora/rev/1ec283702a34

Comment 23

[Takeharu Igari]

I found an another bug that it's related to this bug possibly.
https://bugzilla.mozilla.org/show_bug.cgi?id=1427148
(Against one's will, I marked the bug as security bug. If anybody has the permission can unmark, please unmark.)