Closed Bug 1316051 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(9 files, 1 obsolete file)

9.78 KB, text/html
Details
3.82 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
2.22 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
8.10 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
3.05 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
94.77 KB, patch
Details | Diff | Splinter Review
7.77 KB, patch
Details | Diff | Splinter Review
9.65 KB, patch
Details | Diff | Splinter Review
7.31 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
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
Blocks: 1217086
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.
Attachment #8810260 - Flags: review?(dholbert)
I think this is the correct fix also for table flex items, so I'm changing
both here.
Attachment #8810261 - Flags: review?(dholbert)
(just some bonus tests for other elements while I'm at it)
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
Attachment #8810258 - Flags: review?(dholbert) → review+
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.
Attachment #8810259 - Flags: review?(dholbert) → review+
(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).
Flags: needinfo?(mats)
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.
Attachment #8810260 - Attachment is obsolete: true
Attachment #8810260 - Flags: review?(dholbert)
Flags: needinfo?(mats)
Attachment #8811983 - Flags: review?(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.)
Attachment #8811983 - Flags: review?(dholbert) → review+
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
Attachment #8810261 - Flags: review?(dholbert) → review+
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 [...]
   }
 }
Attachment #8810262 - Flags: review?(dholbert) → review+
> 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.
Pushed by mpalmgren@mozilla.com:
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.
(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 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.
Attachment #8811983 - Flags: approval-mozilla-aurora?
Flags: in-testsuite+
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
Attachment #8811983 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1334403
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.)
Depends on: 1505410
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: