Closed Bug 1000957 Opened 11 years ago Closed 8 years ago

Clamp flex line's height (clamping stretched flex items), in single-line auto-height flex container w/ max-height (spec change)

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: dholbert, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files, 1 obsolete file)

Bug 1000957: Uncomment some flexbox warnings referencing fixed bugs.
(deleted), text/x-review-board-request
dholbert
: review+
Details
Bug 1000957: Whitespace cleanup in nsFlexContainerFrame::Reflow.
(deleted), text/x-review-board-request
dholbert
: review+
Details
Bug 1000957: For single-line flex containers, clamp the flex line to container's min/max cross size.
(deleted), text/x-review-board-request
dholbert
: review+
Details
Bug 1000957: Reftests for flexbox single-line size clamping.
(deleted), text/x-review-board-request
dholbert
: review+
Details
Quoting CSSWG minutes: TabAtkins: If flex is indefinite but has a max height and the content is bigger than max, does flex item grow to contain or is content constrained? TabAtkins: The flex item gets set to 150px, to size of content, and overflows the constrained flex container. TabAtkins: Blink is different and makes flex item stay the size of flex container. TabAtkins: This is same as if you set a height instead of max height. RESOLVED: Change the spec's (flexbox) max-height implementation to match Blink's implementation. http://lists.w3.org/Archives/Public/www-style/2014Apr/0373.html (This was originally brought up in http://lists.w3.org/Archives/Public/www-style/2014Apr/0292.html )
Depends on: css3-flexbox
Summary: Clamp stretched flex item's height, in single-line auto-height max-height-clamped flex container (spec change) → Clamp flex line's height (clamping stretched flex items), in single-line auto-height flex container w/ max-height (spec change)
This is in the ED of the spec now: https://dvcs.w3.org/hg/csswg/rev/9b2355f9ccf3
Blocks: flexbox-spec-changes
:dholbert: Is this the same as this bug, were changing the flow-direction does not inherit max-height? https://jsfiddle.net/ux6gL466/1/ See the difference Chrome vs Firefox. It does inherit the height with height: 100px, but not with max-height: 100px. I'm asking because I'm writing a patch for treeherder and stumbled upon this issue there.
Nevermind, I've had a look at fiddle in the linked mail and am pretty sure that this is the same issue.
Yup, it is the same issue. Here's a simpler version of your testcase, w/ only one flex container: http://jsfiddle.net/ux6gL466/8/ Per the new spec-text, we should be clamping the line height to be inside the container's [min-height,max-height] range, since our flex-container has only one line. I'm not focusing on implementing flexbox spec changes at the moment, but hopefully I'll get some cycles to circle back to this & other dependencies of the "flexbox-spec-changes" metabug. Unassigning for the moment to reflect reality, though (& so someone else can pick this up if they like).
Assignee: dholbert → nobody
Blocks: 1251374
Comment on attachment 8778748 [details] Bug 1000957: Whitespace cleanup in nsFlexContainerFrame::Reflow. https://reviewboard.mozilla.org/r/69910/#review67240 Thanks for taking this! A few quick notes (haven't reviewed thoroughly yet): ::: layout/reftests/flexbox/flexbox-single-line-clamp-1.html:4 (Diff revision 1) > +<!doctype html> > +<style> > +.flexrow { > + max-height: 100px; We need a few more testcases here: (1) a testcase for min-height clamping (along with this max-height clamping testcase). (The min-height version would have flex items that are small, but get stretched (via default `align-items:stretch`) to the container's min-height.) (2) a testcase for the opposite orientation of flex container -- i.e. a min/max-width clamped element with `flex-direction:column`. ::: layout/reftests/flexbox/flexbox-single-line-clamp-1.html:25 (Diff revision 1) > + background: green; > +} > +</style> > +<div class="flexrow"> > + <div class="flexcolumn"> > + <div class="flexrowscroll"> This is a bit more complex than it needs to be (with 3 nested flex containers, scrolling, etc.) Probably better to make this simpler -- just a single flex container with a child (or two) with tall contents, like so: https://jsfiddle.net/04o1kwfd/5/
Comment on attachment 8778747 [details] Bug 1000957: Uncomment some flexbox warnings referencing fixed bugs. https://reviewboard.mozilla.org/r/69908/#review67520 This patch doesn't really have much to do with this bug, but I suppose it's fine to land it here -- thanks for noticing these stale comments and cleaning them up! r=me with two nits fixed: First, the commit message: > Bug 1000957: layout: Uncomment some flexbox warnings referencing fixed bugs. r?dholbert Probably best to drop "layout:" there (maybe that's a servo convention, but it's not common in gecko commit messages). ::: layout/generic/nsFlexContainerFrame.cpp (Diff revision 1) > { > if (aAxisTracker.IsRowOriented()) { > // Row-oriented --> our main axis is the inline axis, so our main size > // is our inline size (which should already be resolved). > - // XXXdholbert ISize may be (wrongly) unconstrained right now: bug 1163238 > - // Uncomment when that's fixed: Second nit: there's one more comment further down that *also* needs removing (since it points back to this comment that you're removing here) -- search this file for this string: "mentions of bug 1163238 in GetMainSizeFromReflowInput" ...and please delete that XXX comment.
Attachment #8778747 - Flags: review?(dholbert) → review+
Comment on attachment 8778748 [details] Bug 1000957: Whitespace cleanup in nsFlexContainerFrame::Reflow. https://reviewboard.mozilla.org/r/69910/#review67522 r- on this part for now, because I'd like to see the final updated tests (per my previous comments) -- but the code changes look good! A few stylistic nits, too: First, as noted for the other part, probably drop "layout" from the commit message. Secondly, the commit message text ("Clamp the first line's size to the flex container's size always") isn't actually correct. A more accurate description would be something like this: "For single-line flex containers, clamp the flex line to container's min/max cross size" ::: layout/generic/nsFlexContainerFrame.cpp:2775 (Diff revision 1) > - aFirstLine->SetLineCrossSize(aContentBoxCrossSize); > + aFirstLine->SetLineCrossSize(aContentBoxCrossSize); > - return; > + return; > - } > + } > > + // "If the flex container is single-line, then clamp the line’s > + // cross-size to be within the container’s computed min and max cross-size You're using a fancy UTF single-quote character here (in "line’s" and "container’s"). Please replace that with an ASCII single-quote character (s/’/'/). ::: layout/generic/nsFlexContainerFrame.cpp:2782 (Diff revision 1) > + aFirstLine->SetLineCrossSize(NS_CSS_MINMAX(aFirstLine->GetLineCrossSize(), > + aReflowInput.ComputedMinBSize(), > + aReflowInput.ComputedMaxBSize())); > + } > + > + Nit: you're adding a double blank line here, which is out-of-place (uncommon in this file I think, and a bit excessive in general). Please collapse these two lines together. ::: layout/generic/nsFlexContainerFrame.cpp:3812 (Diff revision 1) > } > } > > void > -nsFlexContainerFrame::Reflow(nsPresContext* aPresContext, > - ReflowOutput& aDesiredSize, > +nsFlexContainerFrame::Reflow(nsPresContext* aPresContext, > + ReflowOutput& aDesiredSize, This whitespace cleanup is good, but unrelated to this patch, and as-such it makes it harder to see what's actually changing in the patch. Please revert this part of the patch, and make it in a separate change if you like (either before or after this one, or as a "no bug" change). r=me in advance on that standalone patch.
Attachment #8778748 - Flags: review?(dholbert) → review-
Comment on attachment 8778748 [details] Bug 1000957: Whitespace cleanup in nsFlexContainerFrame::Reflow. https://reviewboard.mozilla.org/r/69910/#review67692 r+ on the whitespace-cleanup change. Thanks!
Attachment #8778748 - Flags: review?(dholbert) → review+
Comment on attachment 8778748 [details] Bug 1000957: Whitespace cleanup in nsFlexContainerFrame::Reflow. https://reviewboard.mozilla.org/r/69910/#review67522 > You're using a fancy UTF single-quote character here (in "line’s" and "container’s"). Please replace that with an ASCII single-quote character (s/’/'/). Heh, spec text copy-pasta, fixed :)
Comment on attachment 8779428 [details] Bug 1000957: For single-line flex containers, clamp the flex line to container's min/max cross size. https://reviewboard.mozilla.org/r/70422/#review67706 r+ on the code-changes patch here - thanks!
Attachment #8779428 - Flags: review?(dholbert) → review+
Comment on attachment 8779429 [details] Bug 1000957: Reftests for flexbox single-line size clamping. https://reviewboard.mozilla.org/r/70424/#review67708 The reftests need a bit more tweaking/simplifying, I thing - see below. Also: ideally these should live in layout/reftests/w3c-css/submitted/flexbox so we can share them easily with other vendors, and they should include the appropriate w3c header tags (link rel="author", link rel="match", link rel="help") for w3c-submitted tests. Could you move them there and add those headers? ::: layout/reftests/flexbox/flexbox-single-line-clamp-1-ref.html:7 (Diff revision 3) > +<style> > +.container { > + display: block; > + background: gray; > + max-height: 200px; > + overflow: hidden; This reference case can be simplified a good deal. In particular, I think you can & should: - drop one of the panels. (as suggested above) - drop the `float:left` (which you won't need anymore if there's only one panel) - drop both instances of `overflow: hidden` (on the container and the panel). (you won't need it on the container anymore, if the panels aren't floated.) ALTERNATELY, it might be simplest to just make this reference case identical to the testcase except with an explicit "height" instead of "max-height" on the container. The way you've got it (using block instead of flex) is fine too, though; it just makes them a little harder to compare. ::: layout/reftests/flexbox/flexbox-single-line-clamp-1.html:12 (Diff revision 3) > +} > +.panel { > + background: lightblue; > + width: 150px; > + border: 1px solid purple; > + overflow: hidden; Probably drop `overflow:hidden` here -- I don't think it adds anything useful to the testcase. ::: layout/reftests/flexbox/flexbox-single-line-clamp-1.html:25 (Diff revision 3) > +<div class="container"> > + <div class="panel"> > + <div class="tall-child"></div> > + </div> > + <div class="panel"> > + <div class="tall-child"></div> I don't think there's any benefit to having 2 panels here (particularly since they're the same). So, maybe get rid of the second one? (That'll let the reference case be a bit simpler, too.) ::: layout/reftests/flexbox/flexbox-single-line-clamp-2-ref.html:17 (Diff revision 3) > + box-sizing: border-box; > + width: 100%; > + height: 102px; > +} > +.panel:last-child { > + height: 52px; The 102px & 52px sizes here are kinda magical (it takes a second to see where they come from in the testcase), and this :last-child selector feels a bit magical/fragile as well. (given that there's no such selector in the testcase) Could you: (a) drop the `box-sizing:border-box` if you don't actually need it (that'll let these elements just have the same heights as their content in the testcase, which makes things easier to map) (b) Make these elements have a consistent height so that you can do away with this :last-child selector. (since this testcase is testing widths anyway -- not heights) That will mean giving .small-box and .big-box the same height in the testcase, I think. ::: layout/reftests/flexbox/flexbox-single-line-clamp-2.html:13 (Diff revision 3) > + max-width: 250px; > +} > +.panel { > + background: lightblue; > + border: 1px solid purple; > + overflow: hidden; As in the first testcase, can we get rid of `overflow:hidden` here, to simplify this a little? ::: layout/reftests/flexbox/flexbox-single-line-clamp-2.html:20 (Diff revision 3) > +} > +.big-box { > + width: 1000px; > + height: 50px; > +} > +.small-box { .small-box and .big-box are in the opposite order here, with respect to the content order. Please rearrange them so they're consistent. (That makes this easier to follow.)
Attachment #8779429 - Flags: review?(dholbert) → review-
Comment on attachment 8779430 [details] Bug 1000957: Use argument instead of member variable InitAxesFromModernProps. https://reviewboard.mozilla.org/r/70426/#review67728 This change doesn't seem correct. As you note in the extended commit message, aWM and mWM seem to be redundant; we should just remove aWM. I'll file a separate bug for that, since bugs are cheap and the change doesn't really have anything to do with this bug.
Attachment #8779430 - Flags: review?(dholbert) → review-
I filed bug 1293738 on simplifying InitAxesFromModernProps, BTW. Feel free to take that (though no pressure). I might also mark it a "good first bug" or just fix it myself soonish, since it's just a bit of code-cleanup.
Comment on attachment 8779429 [details] Bug 1000957: Reftests for flexbox single-line size clamping. https://reviewboard.mozilla.org/r/70424/#review67782 Thanks for fixing these up! r=me with the following: ::: layout/reftests/w3c-css/submitted/flexbox/flexbox-single-line-clamp-1.html:7 (Diff revision 4) > +<!-- > + Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ > +--> > +<meta charset="utf-8"> > +<title>CSS Test: Single-line flexbox containers should clamp their first line height to the container's computed min and max cros-size.</title> Three nits (which apply to each of the test files here, here since they all share this title I think): (1) typo: s/cros/cross/ (2) s/flexbox container/flex container/ (3) s/their first line/their line's/ ("first line" is unnecessarily specific, since you've already said "single-line flex container") ::: layout/reftests/w3c-css/submitted/flexbox/flexbox-single-line-clamp-3-ref.html:1 (Diff revision 4) > +<!DOCTYPE html> extreme nit: this file has uppercase "DOCTYPE", whereas its corresponding testcase has lowercase "doctype". Please make those consistent between the two files, one way or another, so that's not a spurious difference between the testcase & its reference case.
Attachment #8779429 - Flags: review?(dholbert) → review+
Attachment #8779430 - Attachment is obsolete: true
Attachment #8779430 - Flags: review?(dholbert)
Comment on attachment 8779429 [details] Bug 1000957: Reftests for flexbox single-line size clamping. https://reviewboard.mozilla.org/r/70424/#review67708 > Probably drop `overflow:hidden` here -- I don't think it adds anything useful to the testcase. Yep, leftovers from when I did it with text.
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc0d979b2c8 Uncomment some flexbox warnings referencing fixed bugs. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/a6c64727f714 Whitespace cleanup in nsFlexContainerFrame::Reflow. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/4f2073faf7df For single-line flex containers, clamp the flex line to container's min/max cross size. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6907edbc42 Reftests for flexbox single-line size clamping. r=dholbert
Depends on: 1295514
Depends on: 1295249
No longer depends on: 1295514
Assignee: nobody → emilio
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: