Closed Bug 783470 Opened 12 years ago Closed 10 years ago

implement "visibility: collapse" behavior on flexbox

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: dholbert, Assigned: dholbert)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=CSS])

Attachments

(6 files, 1 obsolete file)

The flexbox spec has some requirements for special handling of "visibility:collapse" on flex items:  http://dev.w3.org/csswg/css3-flexbox/#visibility-collapse

The patches in bug 666041 don't currently implement this behavior.  Filing this bug to cover that.

We may be able to enable flexbox in builds without this, but I'm tentatively flagging this as blocking bug 783409 (enable flexbox in builds) for now.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
The most specific/explicit chunk of the spec on this is here:
 http://dev.w3.org/csswg/css-flexbox/#algo-visibility

Specifically, it says (below step "9.  Handle 'align-content: stretch'"):
  # 10. Collapse visibility:collapse items. If any flex items
  # have visibility: collapse, note the cross size of the line
  # they’re in as the item’s strut size, and restart layout from
  # the beginning.
  #
  # In this second layout round, when collecting items into lines,
  # treat the collapsed items as having zero main size. For the
  # rest of the algorithm following that step, ignore the
  # collapsed items entirely (as if they were display:none) except
  # that after calculating the cross size of the lines, if any
  # line’s cross size is less than the largest strut size among
  # all the collapsed items in the line, set its cross size to
  # that strut size.
  #
  # Skip this step in the second layout round.

Patches coming up. The high-level strategy of the patches is:
 - Split most of flex layout out of ::Reflow and into a helper function.

 - When we get to the step for the spec-text above, bail out from the helper function, and allow ::Reflow to invoke the helper-function again (which starting the second layout round), passing in metadata that we've collected about where we should place struts.

 - Use a special FlexItem() constructor to make struts.
(Non-functional patch -- just splitting some code into a helper function, and renaming variables that have been converted into arguments.)
Attachment #8361970 - Flags: review?(matspal)
This patch makes us create an array of "StrutInfo" objects, which we populate in the first reflow round. Each of these objects tells us the index and the cross-size of a visibility:collapse "strut" that we'll need to make on our second reflow round.

(We don't actually act on this information yet - that'll come in the next patch.)
Attachment #8361971 - Flags: review?(matspal)
Attachment #8361971 - Attachment description: part 2: maintain "StrutInfo" about the struts that we need to build (and restart flex layout if there are any) → part 2: Generate a "StrutInfo" for each strut that we need to build (and restart flex layout if there are any)
NOTE: For the second layout round, we can just make the struts as inflexible 0-main-size FlexItems, with a preordained cross-size, and otherwise let layout proceed as usual. (This is what the just-attached 'part 3' patch does.)

The struts will end up in whatever line they end up in, and (since flex lines are sized to accomodate their contents' cross sizes) the struts will automatically do their job of enforcing a minimum line-size.
Attached patch part 4: reftestsSplinter Review
Here are some reftests for this feature.

Generally, each reference case is identical to the testcase, but with "visibility:collapse" elements converted into hardcoded struts, effectively. (flex items with 0 main size and a fixed cross-size)

(The one exception to this pattern is for flexbox-collapsed-item-baseline-1.html. In that testcase, the collapsed element participates in baseline alignment during the first layout round, and the collective height of the baseline-aligned items establishes the line height and hence its strut size. Since that strut size isn't easy to guess in a cross-platform way (due to the dependence on baselines), I just made the reference case have some hidden baseline-aligned items, which together form a "strut equivalent".)
Attachment #8361985 - Flags: review?(matspal)
Attachment #8361970 - Flags: review?(matspal) → review+
Comment on attachment 8361971 [details] [diff] [review]
part 2: Generate a "StrutInfo" for each strut that we need to build (and restart flex layout if there are any)

>layout/generic/nsFlexContainerFrame.cpp
>+struct StrutInfo {
>+  StrutInfo(uint32_t aItemIdx,
>+            nscoord aStrutCrossSize)

Looks like the params would fit on one line.

>+        // Note the cross size of the line as the item's strut size

Append a period to end the sentence.

>layout/generic/nsFlexContainerFrame.h
>+class StrutInfo;

This type could go inside the nsFlexContainerFrame class?
Attachment #8361971 - Flags: review?(matspal) → review+
Comment on attachment 8361974 [details] [diff] [review]
part 3: Act on StrutInfo to make struts (specialized frozen 0-main-size FlexItems)

A few nits:

>layout/generic/nsFlexContainerFrame.cpp
>+  bool mIsStrut;     // Is this item a "strut" left behind by an element
>+                     //  with visibility:collapse?

Remove one space before "with".

>+FlexItem::FlexItem(nsIFrame* aChildFrame,
>+                   nscoord aCrossSize)

Looks like aCrossSize would fit on the line above.

For both FlexItem ctors: can they assert that
!aChildFrame->IsAbsolutelyPositioned() ?

>+      // Use the shortened "strut" constructor for FlexItem:

s/shortened/simplified/ perhaps? (because that's the word used in
the description of that ctor).

s/constructor for FlexItem/FlexItem constructor/ reads better?

>+      item = curLine->mItems.AppendElement(
>+               FlexItem(childFrame,
>+                        aStruts[nextStrutIdx].mStrutCrossSize));

I'd prefer "aStruts..." next to "childFrame," if it fits.

>   nsresult rv = GenerateFlexLines(aPresContext, aReflowState,
>                                   aContentBoxMainSize,
>                                   aAvailableHeightForContent,
>+                                  aStruts,
>                                   aAxisTracker, lines);

I'd prefer "aStruts, " before "aAxisTracker" if it fits.
Attachment #8361974 - Flags: review?(matspal) → review+
Comment on attachment 8361985 [details] [diff] [review]
part 4: reftests

>layout/reftests/w3c-css/submitted/flexbox/flexbox-collapsed-item-baseline-1.html
>+  <link rel="match" href="flexbox-collapsed-item-horiz-baseline-1-ref.html">

The href should be "flexbox-collapsed-item-baseline-1-ref.html".
Attachment #8361985 - Flags: review?(matspal) → review+
(In reply to Mats Palmgren (:mats) from comment #7)
> >layout/generic/nsFlexContainerFrame.h
> >+class StrutInfo;
> 
> This type could go inside the nsFlexContainerFrame class?

I believe that would mean I'd have to actually *define* it inside of the nsFlexContainerFrame class -- i.e. move the StrutInfo defn to the .h file -- which I'd rather not do, to keep the .h file simple.  Note that this applies to FlexItem, FlexLine, & FlexboxAxisTracker as well -- I think it's cleaner to keep all of those class definitions out of the .h file, and just forward-declare them as-needed for the benefit of nsFlexContainerFrame methods.

So I'll pass on that change, but I applied the other ones:
 https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/426fa0070806

(In reply to Mats Palmgren (:mats) from comment #8)
> For both FlexItem ctors: can they assert that
> !aChildFrame->IsAbsolutelyPositioned() ?

Yes; good call. I'll add that assertion in a separate patch here, since that's not directly connected to any of the patches here.
Here's a "patch 5", introducing the assertion (suggested in comment 8) to verify that there are no abspos FlexItems.

I also made the existing FlexItem assertions slightly more consistent ((a) asserting about mFrame, instead of aChildFrame, since mFrame is the copy of aChildFrame that we're going to be working with; and (b) asserting that mFrame is non-null before dereferencing it, in the new "strut" FlexItem constructor)

I'm taking the liberty of labeling this rs=mats, since this is just implementing a review nit. But if you have review feedback, feel free to chime in.
(In reply to Daniel Holbert [:dholbert] from comment #10)
> I believe that would mean I'd have to actually *define* it inside of the
> nsFlexContainerFrame class -- i.e. move the StrutInfo defn to the .h file

I don't think so.  You just forward-declare it as usual: "struct StrutInfo;"
inside the class and then define it as "struct nsFlexContainerFrame::StrutInfo"
in the .cpp file.  The only problem seems to be the global function
BuildStrutInfoFromCollapsedItems which would need to use
nsFlexContainerFrame::StrutInfo, but you could expose StrutInfo just in
the .cpp file by adding "typedef nsFlexContainerFrame::StrutInfo StrutInfo;"
at the top in the .cpp file for convenience.

That seems to work with clang, and I'd be surprised if other compilers
didn't accept it.

(BTW, BuildStrutInfoFromCollapsedItems should be static?)

> which I'd rather not do, to keep the .h file simple.

I agree we don't want the definition in the header.
(In reply to Mats Palmgren (:mats) from comment #13)
> I don't think so.  You just forward-declare it as usual: "struct StrutInfo;"
> inside the class and then define it as "struct
> nsFlexContainerFrame::StrutInfo"
> in the .cpp file.  The only problem seems to be the global function
> BuildStrutInfoFromCollapsedItems which would need to use
> nsFlexContainerFrame::StrutInfo, but you could expose StrutInfo just in
> the .cpp file by adding "typedef nsFlexContainerFrame::StrutInfo StrutInfo;"
> at the top in the .cpp file for convenience.

Ah, that does seem to work (in both GCC and clang). Thanks!

I thought I'd tried something like that in the past for FlexItem and that it didn't work, but apparently I misremembered. (Or I might've avoided it since I thought it'd mean I'd need extremely-long function signatures in the .cpp file -- e.g. nsFlexContainerFrame::FlexItem::DoWhatever(SomeLongParameterType) -- but some brief local testing shows that the typedef saves us from that. Nice.)

I'll spin off another bug to fix the other forward-declared helper-classes here, but I'll fix StrutInfo in the patch that introduces it here.
OK, here's an updated version of part 2, fixed per comment 13.

I spun off bug 962267 to make similar changes to other helper-classes.
Attachment #8361971 - Attachment is obsolete: true
Attached file testcase
Here's a very simple testcase demonstrating the new behavior, FWIW.
FWIW, Blink (and probably also webkit) seems to be missing this behavior -- I filed http://code.google.com/p/chromium/issues/detail?id=336604 to be sure it's on someone's radar.
Whiteboard: [DocArea=CSS]
Blocks: 1000359
Glad it works, but I have a question. Should it support transition/animation?
No -- not in the way that you're expecting, I think.

CSS transitions/animations get their "smooth animation" effects by moving a property across intermediate values between two endpoints. So, e.g. a CSS transition from "opacity:0" to "opacity:1" will be at "opacity:0.5", when it's halfway complete, since 0.5 is the value that's halfway between 0 and 1.

This can't work for keyword-valued properties like "visibility", because there are no intermediate property-values between two keywords. e.g. there is no "visibility" value for halfway between "visible" and "collapse".
(In reply to Daniel Holbert [:dholbert] from comment #21)
> No -- not in the way that you're expecting, I think.
> 
> CSS transitions/animations get their "smooth animation" effects by moving a
> property across intermediate values between two endpoints. So, e.g. a CSS
> transition from "opacity:0" to "opacity:1" will be at "opacity:0.5", when
> it's halfway complete, since 0.5 is the value that's halfway between 0 and 1.
> 
> This can't work for keyword-valued properties like "visibility", because
> there are no intermediate property-values between two keywords. e.g. there
> is no "visibility" value for halfway between "visible" and "collapse".

Mmm... I see. I thought this would finally solve the ugly hack of animating max-height >_<
Moved the paragraph describing the issue from the main content to a compatibility footnote on https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Flexible_Box_Layout/Using_CSS_flexible_boxes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: