Closed
Bug 783470
Opened 12 years ago
Closed 11 years ago
implement "visibility: collapse" behavior on flexbox
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: dholbert, Assigned: dholbert)
References
()
Details
(Keywords: dev-doc-complete, Whiteboard: [DocArea=CSS])
Attachments
(6 files, 1 obsolete file)
14.59 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
12.64 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
23.00 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
2.14 KB,
patch
|
Details | Diff | Splinter Review | |
7.92 KB,
patch
|
Details | Diff | Splinter Review | |
586 bytes,
text/html
|
Details |
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.
Updated•12 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
(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)
Assignee | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
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)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8361974 -
Flags: review?(matspal)
Assignee | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8361970 -
Flags: review?(matspal) → review+
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
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 9•11 years ago
|
||
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+
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Assignee | ||
Comment 11•11 years ago
|
||
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.
Assignee | ||
Comment 12•11 years ago
|
||
Flags: in-testsuite?
Comment 13•11 years ago
|
||
(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.
Assignee | ||
Comment 14•11 years ago
|
||
(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.
Assignee | ||
Comment 15•11 years ago
|
||
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
Assignee | ||
Comment 16•11 years ago
|
||
Landed:
part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/137e02fe6f55
part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1794e78469
part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/31e897da75da
part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/a3651e87da6c
part 5: https://hg.mozilla.org/integration/mozilla-inbound/rev/da32dcdc4121
Flags: in-testsuite? → in-testsuite+
Assignee | ||
Comment 17•11 years ago
|
||
Here's a very simple testcase demonstrating the new behavior, FWIW.
Assignee | ||
Comment 18•11 years ago
|
||
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 19•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/137e02fe6f55
https://hg.mozilla.org/mozilla-central/rev/2a1794e78469
https://hg.mozilla.org/mozilla-central/rev/31e897da75da
https://hg.mozilla.org/mozilla-central/rev/a3651e87da6c
https://hg.mozilla.org/mozilla-central/rev/da32dcdc4121
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•11 years ago
|
Whiteboard: [DocArea=CSS]
Comment 20•10 years ago
|
||
Glad it works, but I have a question. Should it support transition/animation?
Assignee | ||
Comment 21•10 years ago
|
||
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•10 years ago
|
||
(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•9 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•