Last Comment Bug 783470 - implement "visibility: collapse" behavior on flexbox
: implement "visibility: collapse" behavior on flexbox
Status: RESOLVED FIXED
[DocArea=CSS]
: dev-doc-complete
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla29
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
http://dev.w3.org/csswg/css-flexbox/#...
Depends on: css3-flexbox
Blocks: 1000359 783409
  Show dependency treegraph
 
Reported: 2012-08-16 22:22 PDT by Daniel Holbert [:dholbert]
Modified: 2015-12-01 14:25 PST (History)
7 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: split most of flex layout into a helper function (14.59 KB, patch)
2014-01-17 15:47 PST, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Review
part 2: Generate a "StrutInfo" for each strut that we need to build (and restart flex layout if there are any) (7.05 KB, patch)
2014-01-17 15:55 PST, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Review
part 3: Act on StrutInfo to make struts (specialized frozen 0-main-size FlexItems) (12.64 KB, patch)
2014-01-17 15:57 PST, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Review
part 4: reftests (23.00 KB, patch)
2014-01-17 16:23 PST, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Review
part 5: assertion to ensure we don't create FlexItems for abspos children (2.14 KB, patch)
2014-01-21 12:27 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
part 2: Generate a "StrutInfo" for each strut that we need to build (now forward-declared inside nsFlexContainerFrame{}) (7.92 KB, patch)
2014-01-21 14:10 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
testcase (586 bytes, text/html)
2014-01-21 15:03 PST, Daniel Holbert [:dholbert]
no flags Details

Description Daniel Holbert [:dholbert] 2012-08-16 22:22:59 PDT
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.
Comment 1 Daniel Holbert [:dholbert] 2014-01-17 15:36:04 PST
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.
Comment 2 Daniel Holbert [:dholbert] 2014-01-17 15:47:42 PST
Created attachment 8361970 [details] [diff] [review]
part 1: split most of flex layout into a helper function

(Non-functional patch -- just splitting some code into a helper function, and renaming variables that have been converted into arguments.)
Comment 3 Daniel Holbert [:dholbert] 2014-01-17 15:55:39 PST
Created 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)

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.)
Comment 4 Daniel Holbert [:dholbert] 2014-01-17 15:57:30 PST
Created attachment 8361974 [details] [diff] [review]
part 3: Act on StrutInfo to make struts (specialized frozen 0-main-size FlexItems)
Comment 5 Daniel Holbert [:dholbert] 2014-01-17 16:01:34 PST
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.
Comment 6 Daniel Holbert [:dholbert] 2014-01-17 16:23:40 PST
Created attachment 8361985 [details] [diff] [review]
part 4: reftests

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".)
Comment 7 Mats Palmgren (:mats) 2014-01-20 02:14:53 PST
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?
Comment 8 Mats Palmgren (:mats) 2014-01-20 02:17:11 PST
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.
Comment 9 Mats Palmgren (:mats) 2014-01-20 02:17:22 PST
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".
Comment 10 Daniel Holbert [:dholbert] 2014-01-21 12:13:23 PST
(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.
Comment 11 Daniel Holbert [:dholbert] 2014-01-21 12:27:56 PST
Created attachment 8363168 [details] [diff] [review]
part 5: assertion to ensure we don't create FlexItems for abspos children

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.
Comment 12 Daniel Holbert [:dholbert] 2014-01-21 12:29:13 PST
Try run: https://tbpl.mozilla.org/?tree=Try&rev=b61ff828cfd7
Comment 13 Mats Palmgren (:mats) 2014-01-21 13:15:56 PST
(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.
Comment 14 Daniel Holbert [:dholbert] 2014-01-21 14:01:31 PST
(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.
Comment 15 Daniel Holbert [:dholbert] 2014-01-21 14:10:01 PST
Created attachment 8363243 [details] [diff] [review]
part 2: Generate a "StrutInfo" for each strut that we need to build (now forward-declared inside nsFlexContainerFrame{})

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.
Comment 17 Daniel Holbert [:dholbert] 2014-01-21 15:03:59 PST
Created attachment 8363275 [details]
testcase

Here's a very simple testcase demonstrating the new behavior, FWIW.
Comment 18 Daniel Holbert [:dholbert] 2014-01-21 15:14:03 PST
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.
Comment 20 danielolanoc 2015-04-11 07:32:24 PDT
Glad it works, but I have a question. Should it support transition/animation?
Comment 21 Daniel Holbert [:dholbert] 2015-04-11 09:08:00 PDT
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".
Comment 22 danielolanoc 2015-04-12 08:40:50 PDT
(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 >_<
Comment 23 Ryan S. Scheel [:havvy] 2015-12-01 14:25:31 PST
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

Note You need to log in before you can comment on or make changes to this bug.