Closed Bug 1467209 Opened 6 years ago Closed 6 years ago

Implement contain:size for FlexContainerFrame, HTMLButtonControlFrame, and BlockFrame

Categories

(Core :: Layout, enhancement, P3)

Product:
Core
Component:
Layout
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: morgan, Assigned: morgan)

References

Details

Attachments

(3 files, 9 obsolete files)

Bug 1467209 - Implement contain:size for HTMLButtonControlFrame.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1467209 - Implement contain:size for blockFrame.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1467209 - Implement contain:size for flexContainerFrame.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Helper bug to allow patch landing for some frame implementations.
Attachment #8984268 - Flags: review?(dholbert)
Attachment #8984269 - Flags: review?(dholbert)
Attachment #8984270 - Flags: review?(dholbert)
Attachment #8984271 - Flags: review?(dholbert)
Comment on attachment 8984268 [details] Bug 1467209 - Update some baseline-querying utility functions to bail on frames that have 'contain:size'. https://reviewboard.mozilla.org/r/250058/#review256380 r=me with a clearer commit message, per below: ::: commit-message-b75ac:1 (Diff revision 1) > +Bug 1467209 - Base changes needed to implement contain:size for all affected frames. Best-practice is for commit messages to "describe the change", and this is a bit too vague right now - it's not clear what (if anything) is changing, behavior-wise. Maybe update to something like the following, which seems to be the relevant behavior-change in this patch: "Update some baseline-querying utility functions to bail on frames that have 'contain:size'."
Attachment #8984268 - Flags: review?(dholbert) → review+
Comment on attachment 8984269 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/250060/#review256382 ::: layout/forms/nsHTMLButtonControlFrame.cpp:147 (Diff revision 2) > nscoord result; > DISPLAY_MIN_WIDTH(this, result); > + if (StyleDisplay()->IsContainSize()) { > + return 0; > + } In order for this DISPLAY_MIN_WIDTH(...) macro to continue working, you unfortunately have to use "result" as your return variable here. (which, granted, is kind of annoying/clumsy) (Under the hood, DISPLAY_MIN_WIDTH sets up an object that gets a reference to |result|, and that pretty-prints out its value when this function returns, if you have certain debugging features enabled. That happens here, through a few layers of indirection: https://dxr.mozilla.org/mozilla-central/rev/f7fd9b08c0156be5b5cd99de5ed0ed0b98d93051/layout/generic/nsFrame.cpp#12454-12469 ) So your changes here (and to GetPrefISize, and similar functions in other files) need to be restructured to do "result = 0", and probably to share the existing return statement to the extent that that's possible. (though you could add an early "return result" in more-complex functions if that ends up being cleaner). But for this one, probably best to structure things like: if (is-contain-size) { result = 0; } else { [old code to compute result] } return result; ::: layout/forms/nsHTMLButtonControlFrame.cpp:267 (Diff revision 2) > - if (aButtonReflowInput.ComputedBSize() != NS_INTRINSICSIZE) { > + if (StyleDisplay()->IsContainSize()) { > + buttonContentBox.BSize(wm) = aButtonReflowInput.ComputedMinBSize(); > + contentsDesiredSize.BSize(wm) = 0; > + } else if (aButtonReflowInput.ComputedBSize() != NS_INTRINSICSIZE) { (1) Use aButtonReflowInput.mStyleDisplay in your changes here (rather than StyleDisplay()). It'll be the same pointer value, but just pre-looked-up for you (since StyleDisplay() has a small nonzero lookup cost) (2) This assignment is only correct if we happen to have an auto-height buttton, I think... If we have a button with "height: 30px" (assuming height is the b-size), then we should be using that as the button's bsize -- not its min-height (unless its min-height is larger). I think you probably want to make this into a special case inside of the "Button is intrinsically sized" scenario -- (the final "else" of this if/else/else branch). That's the case where, if we're contain:size, then we should use our min-bsize. (3) The clobbering of |contentsDesiredSize| is somewhat problematic, because it means our FinishReflowChild() call (further down) will be telling the child that it only gets to have a 0-sized frame. And that's not correct. Our own frame may be small, but our child doesn't have to be. Generally, we shouldn't be messing with *child frames'* desired size objects as part of this feature, I think -- we just shouldn't be using it for our *own* desired-size decisions. So: bottom line, I don't think you need to zero it out here... If you do, I'm curious which calculation needs it to be 0. (and maybe we should be targeting that calculation more directly) In particular, the "extraSpace" calculation probably does need to know the actual child desiredSize, because if we have a contain:size <button> with a fixed width & height, then I think we do need to know the child's desiredSize in order to correctly align it within the (fixed-size and technically size-contained) button. ::: layout/forms/nsHTMLButtonControlFrame.cpp:288 (Diff revision 2) > - if (aButtonReflowInput.ComputedISize() != NS_INTRINSICSIZE) { > + if (StyleDisplay()->IsContainSize()) { > + buttonContentBox.ISize(wm) =aButtonReflowInput.ComputedMinISize(); > + contentsDesiredSize.ISize(wm) = 0; > + } else if (aButtonReflowInput.ComputedISize() != NS_INTRINSICSIZE) { > buttonContentBox.ISize(wm) = aButtonReflowInput.ComputedISize(); > } else { The above 3 comments all apply here as well. And, nit: you're missing a space after the "=" on the ISize(wm) =aButton[...] assignment here.
Attachment #8984269 - Flags: review?(dholbert) → review-
Comment on attachment 8984270 [details] Bug 1467209 - Implement contain:size for blockFrame, add reftests. https://reviewboard.mozilla.org/r/250062/#review256388 Haven't looked at the tests closely yet, but a couple small nits on the code: ::: layout/generic/nsBlockFrame.cpp:690 (Diff revision 3) > /* virtual */ nscoord > nsBlockFrame::GetMinISize(gfxContext *aRenderingContext) > { > + > nsIFrame* firstInFlow = FirstContinuation(); stray new blank line at the beginning of the function here -- let's revert that. ::: layout/generic/nsBlockFrame.cpp:705 (Diff revision 3) > + if (this->StyleDisplay()->IsContainSize()) { > + return 0; > + } As for the button patch, you can't do this or DISPLAY_MIN_WIDTH will stop working. Really, you probably want to do: mMinWidth = 0; return mMinWidth; and similar for GetPrefISize below.
Comment on attachment 8984271 [details] Bug 1467209 - Implement contain:size for flexContianerFrame, add reftests. https://reviewboard.mozilla.org/r/250064/#review256390 ::: commit-message-404f0:1 (Diff revision 3) > +Bug 1467209 - Implement contain:size for flexContianerFrame, add reftests. typo: s/tian/tain/ ::: layout/generic/nsFlexContainerFrame.cpp:3925 (Diff revision 3) > MOZ_ASSERT(aFirstLine, "null first line pointer"); > > + > if (aAxisTracker.IsRowOriented()) { Nit: this is introducing a (second) blank line separating the assertion from the code here -- let's revert that change. ::: layout/generic/nsFlexContainerFrame.cpp:3983 (Diff revision 3) > MOZ_ASSERT(aIsDefinite, "outparam pointer must be non-null"); > > + > + > if (aAxisTracker.IsColumnOriented()) { > // Column-oriented --> our cross axis is the inline axis, so our cross size Let's revert these blank line introductions. ::: layout/generic/nsFlexContainerFrame.cpp:5157 (Diff revision 3) > DISPLAY_MIN_WIDTH(this, mCachedMinISize); > + if (this->StyleDisplay()->IsContainSize()) { > + return 0; > + } > + As for the other patches, you'll need to update this to set the same variable that we handed to DISPLAY_MIN_WIDTH (and similar for GetPrefISize) ::: layout/reftests/w3c-css/submitted/contain/contain-size-flex-001.html:8 (Diff revision 3) > +<head> > + <meta charset="utf-8"> > + <title>CSS Test: 'contain: layout' on flexbox with no intrinsic > + height should cause it to have zero height.</title> > + <link rel="author" title="Kyle Zentner" href="mailto:zentner.kyle@gmail.com"> > + <link rel="help" href="http://www.w3.org/TR/css-containment-1/#containment-layout"> It looks like this "help" link is now a broken link. (Back when Kyle wrote the original versions of these tests, this was the correct hypothetical link based on the draft spec, but the TR hadn't been published yet. And I guess stuff changed around when the TR was published.) Nowadays I think we typically link to the draft spec (that used to be frowned upon by the test-linting tools, but it's I'm pretty sure it's fine now). So this and other tests should probably all link to https://drafts.csswg.org/css-contain/#size-containment for their "help" link.
Attachment #8984270 - Attachment is obsolete: true
Attachment #8984270 - Flags: review?(dholbert)
Attachment #8984271 - Attachment is obsolete: true
Attachment #8984271 - Flags: review?(dholbert)
Comment on attachment 8984269 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/250060/#review256592 This is nearly there! Just a couple stylistic nits on the code. (Also: I think MozReview obsoleted your later patches, probably because you did "hg push review" with just this patch in your outgoing, or something like that. MozReview expects the full series to be pushed each time, even if you've only got changes to one part).) Also: ideally this should land with some reftests for <button>, too (maybe some are coming in a later patch - but ideally it's great to include them with the patch that adds the code they're testing). We'd want to be sure we're testing buttons with & without sizing properties (width/height), as well as testing that baseline-alignment has no effect. (For the baseline-alignment, you could craft a testcase with a "contain:size" button that has transparent text, like data:text/html,Outside<button style="contain:size"><span style="color:transparent">Inside</span</button> -- and then in the reference case, have an identical button without contain:size and without any contents. By definition, they should align/render the same, I think.) ::: layout/forms/nsHTMLButtonControlFrame.cpp:268 (Diff revision 4) > } else { > + if (aButtonReflowInput.mStyleDisplay->IsContainSize()) { Let's fold this else/if together to simplify the structure a bit and save on (re)indentation. So this should be: if (...) { old code; } else if (contain-size) { your code; } else { old code } ::: layout/forms/nsHTMLButtonControlFrame.cpp:292 (Diff revision 4) > } else { > + if (aButtonReflowInput.mStyleDisplay->IsContainSize()) { Same here.
Attachment #8984269 - Flags: review?(dholbert) → review-
Attachment #8984269 - Flags: review?(dholbert)
Attachment #8984529 - Flags: review?(dholbert)
Attachment #8984567 - Flags: review?(dholbert)
Comment on attachment 8984269 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/250060/#review256718 Keeping r- for the moment, since this still needs tests, but I think this looks pretty much done with the following two nits addressed: ::: layout/forms/nsHTMLButtonControlFrame.cpp:269 (Diff revision 7) > + // Button is intrinsically sized and has size containment > + // it should have a BSize that is either zero or the minimum > + // specified BSize. Nit: please add a period after "containment" and capitalize "It", to make it clearer that line 1 vs. line 2-3 are two separate sentences. ::: layout/forms/nsHTMLButtonControlFrame.cpp:332 (Diff revision 7) > // * Button's ascent is its child's ascent, plus the child's block-offset > // within our frame... unless it's orthogonal, in which case we'll use the > // contents inline-size as an approximation for now. > // XXX is there a better strategy? should we include border-padding? > if (aButtonDesiredSize.GetWritingMode().IsOrthogonalTo(wm)) { > aButtonDesiredSize.SetBlockStartAscent(contentsDesiredSize.ISize(wm)); > + } else if (aButtonReflowInput.mStyleDisplay->IsContainSize()) { > + aButtonDesiredSize.SetBlockStartAscent(childPos.B(wm)); > } else { > aButtonDesiredSize.SetBlockStartAscent(contentsDesiredSize.BlockStartAscent() + > childPos.B(wm)); Hmm, the first case (for the IsOrthogonalTo case) needs a small tweak. For context, that case gets triggered for content like the following: data:text/html,abc<button style="writing-mode:vertical-rl">def With the patch as it stands right now: if we add contain:size to the button in that ^ testcase, this code would enter the IsOrthogonalTo case, and wouldn't do any special IsContainSize() stuff -- it'd continue using the "def" size to determine the button's baseline, and that would break the concept of size containment. So -- I think that first case should do something like the following: nscoord orthogAscent = aButtonReflowInput.mStyleDisplay->IsContainSize() ? 0 : contentsDesiredSize.ISize(wm); aButtonDesiredSize.SetBlockStartAscent(orthogAscent); That ensures that our baseline behavior is consistent (for this orthogonal writing-mode scenario) between a case where we have no content vs. a case where we have contain:size.
Attachment #8984269 - Flags: review-
Comment on attachment 8984529 [details] Bug 1467209 - Implement contain:size for blockFrame, add reftests. https://reviewboard.mozilla.org/r/250400/#review256730 ::: layout/generic/nsBlockFrame.cpp:1636 (Diff revision 3) > + else if (aReflowInput.mStyleDisplay->IsContainSize()) { > + // If we're size-containing and we don't have a specified size, then our > + // final size should actually be computed from only our border and padding, > + // as though we were empty. > + // Hence this case is a simplified version of the case below. > + nscoord contentBSize = 0; > + nscoord autoBSize = aReflowInput.ApplyMinMaxBSize(contentBSize); > + aMetrics.mCarriedOutBEndMargin.Zero(); > + autoBSize += borderPadding.BStartEnd(wm); This looks good, but I don't think it handles setting our ascent (which our parent may use to baseline-align us with siblings, if we're a "display:inline-block" element). So we might still be leaking that ascent information about our children. You can test this by viewing (for example) the last line of the testcase that I posted on the chrome button issue[1]: https://jsfiddle.net/mz2Lf4wj/ Chrome gets that jsfiddle's last line correct, for inline-block at least -- it does baseline-alignment as if there were no content inside of the inline-block. I'm guessing we might not get it correct, though (I don't see where we clear the ascent, at least, so I assume we'll be doing the same thing we normally would do & propagating it from our children). Please add a testcase for this (maybe called contain-size-inline-block-003) -- testing cases with & without a specified height on the inline-block -- and make whatever tweaks are needed in nsBlockFrame (if any) to make the testcase pass. [1] (that chrome button issue is https://bugs.chromium.org/p/chromium/issues/detail?id=850169 ) ::: layout/reftests/w3c-css/submitted/contain/contain-size-inline-block-001-ref.html:13 (Diff revision 3) > + body { > + margin: 0; > + } > + .container-ref { > + display: inline; > + background: blue; Let's use "teal" or "aqua" for the background here (or some other lighter color), for the benefit of humans viewing the testcase & trying to read the text that's overlaid on this background. ("blue" is quite dark, and it's dark enough to make black-text-on-blue-background basically unreadable, at least on my monitor.) ::: layout/reftests/w3c-css/submitted/contain/contain-size-inline-block-001-ref.html:19 (Diff revision 3) > + border: 1em solid green; > + } > + </style> > +</head> > +<body> > + <div class="container-ref">This text should remain visable with a blue background.</div> typo: s/visable/visible/ (in both reference + testcase here) ::: layout/reftests/w3c-css/submitted/contain/contain-size-inline-block-001.html:5 (Diff revision 3) > + <title>CSS Test: 'contain: size' on inline element should not cause the element > + to become an inline-block with zero height and width.</title> Let's phrase this more about what the spec *does* require. (The current phrasing feels a bit oddly specific & out-of-left-field, for a reader who's not familiar with the older/deprecated spec text that may've originally required this zero-sized-inline-block conversion. :)) How about: <title> CSS Test: 'contain: size' should not affect the rendering of a non-atomic inline element </title> (That seems like it's pretty directly what you're testing here -- your testcase and reference case are basically identical, aside from the presence of 'contain:size', so you're really just testing that there's no rendering effect.) ::: layout/reftests/w3c-css/submitted/contain/contain-size-inline-block-001.html:7 (Diff revision 3) > + <link rel="author" title="Morgan Rae Reschenberg" href="mailto:mreschenberg@berkeley.edu"> > + <link rel="match" href="contain-size-inline-block-001-ref.html"> > + <style> Be sure to add a <link> pointing back to spec text to each testcase file, like so: <link rel="help" href="https://drafts.csswg.org/css-contain/#containment-size"> (This won't be needed in reference cases -- only in testcases, as a backup for whatever the title/testcase is claiming to be required.) I think you're already planning on doing this -- just mentioning to be sure it's on your radar.
Attachment #8984529 - Flags: review?(dholbert) → review-
Comment on attachment 8984567 [details] Bug 1467209 - Implement contain:size for flexContainerFrame, add reftests. https://reviewboard.mozilla.org/r/250442/#review256746 ::: layout/generic/nsFlexContainerFrame.h:362 (Diff revision 2) > + nscoord ResolveFlexContainerMainSize(const ReflowInput& aReflowInput, > + const FlexboxAxisTracker& aAxisTracker, > + nscoord aTentativeMainSize, > + nscoord aAvailableBSizeForContent, > + const FlexLine* aFirstLine, > + nsReflowStatus& aStatus); > + Let's revert this, I think. (See my comments for this function in nsFlexContainerFrame.cpp -- I don't think you need to promote this to be a nsFlexContainerFrame method.) ::: layout/generic/nsFlexContainerFrame.cpp:14 (Diff revision 2) > #include "nsLayoutUtils.h" > #include "nsPlaceholderFrame.h" > #include "nsPresContext.h" > #include "mozilla/ComputedStyle.h" > #include "mozilla/CSSOrderAwareFrameIterator.h" > #include "mozilla/Logging.h" > #include <algorithm> > #include "gfxContext.h" > #include "mozilla/LinkedList.h" > #include "mozilla/FloatingPoint.h" > #include "mozilla/UniquePtr.h" > #include "WritingModes.h" > +#include "nsLayoutUtils.h" Let's revert this, I think. (You don't need to add this #include -- it's already #included further up, at the top of the section that I'm highlighting in this review-comment.) ::: layout/generic/nsFlexContainerFrame.cpp:3917 (Diff revision 2) > -static nscoord > -ResolveFlexContainerMainSize(const ReflowInput& aReflowInput, > +nscoord > +nsFlexContainerFrame::ResolveFlexContainerMainSize(const ReflowInput& aReflowInput, Let's revert this, I think. (It doesn't look like you need to make this change anymore... I'm guessing you (or Kyle) originally did this to read StyleDisplay() off of the "this" object. But now it looks like your changes in this function are only reading data off of "aReflowInput", so there's no need to be a nsFlexContainerFrame method, because you're not using the |this| pointer at all). ::: layout/generic/nsFlexContainerFrame.cpp:3960 (Diff revision 2) > + if (aReflowInput.mStyleDisplay->IsContainSize()) { > + return aReflowInput.ComputedMinBSize(); > + } > + > // Column-oriented case, with auto BSize: > // Resolve auto BSize to the largest FlexLine length, clamped to our > // computed min/max main-size properties. > // XXXdholbert Handle constrained-aAvailableBSizeForContent case here. > nscoord largestLineOuterSize = GetLargestLineMainSize(aFirstLine); > return NS_CSS_MINMAX(largestLineOuterSize, > aReflowInput.ComputedMinBSize(), Add a comment above this to make it a bit clearer what we're doing and how it fits in to the flow around it. Something like: // Column-oriented case, with auto BSize and size containment: // Behave as if we had no content: just use our min BSize. ::: layout/generic/nsFlexContainerFrame.cpp:4022 (Diff revision 2) > + if (aReflowInput.mStyleDisplay->IsContainSize()) { > + return aReflowInput.ComputedMinBSize(); > + } > + > // Row-oriented case (cross axis is block axis), with auto BSize: > // Shrink-wrap our line(s), subject to our min-size / max-size As above, let's add a comment here (similar to the suggested comment above but s/column/row/) ::: layout/generic/nsFlexContainerFrame.cpp:5154 (Diff revision 2) > DISPLAY_MIN_WIDTH(this, mCachedMinISize); > - if (mCachedMinISize == NS_INTRINSIC_WIDTH_UNKNOWN) { > + if (this->StyleDisplay()->IsContainSize()) { > + mCachedMinISize = 0; > + } else if (mCachedMinISize == NS_INTRINSIC_WIDTH_UNKNOWN) { > mCachedMinISize = IntrinsicISize(aRenderingContext, > nsLayoutUtils::MIN_ISIZE); Two nits: (1) Drop the "this->" -- it's implied. (2) Let's reorder the logic a bit. Right now, this patch would make us start having to call StyleDisplay() unconditionally every time we run this function, and there's a (small) cost to that new function-call. Really, we don't need to do that most of the time. Let's optimize, based on the assumption that mCachedMinISize has probably already been set to something other than the sentinel value most of the time (because when this function is called multiple times, we reuse the cached value) i.e. let's do this instead: if (mCachedMinISize == NS_INTRINSIC_WIDTH_UNKNOWN) { mCachedMinISize = StyleDisplay()->IsContainSize() ? 0 : IntrinsicISize(aRenderingContext, nsLayoutUtils::MIN_ISIZE); } (Just modifying the preexisting code structure, to set mCachedMinISize to 0 in the size-contained scenario.) ::: layout/generic/nsFlexContainerFrame.cpp:5169 (Diff revision 2) > - if (mCachedPrefISize == NS_INTRINSIC_WIDTH_UNKNOWN) { > + if (this->StyleDisplay()->IsContainSize()) { > + mCachedPrefISize = 0; > + } else if (mCachedPrefISize == NS_INTRINSIC_WIDTH_UNKNOWN) { > mCachedPrefISize = IntrinsicISize(aRenderingContext, > nsLayoutUtils::PREF_ISIZE); Above comments apply here too. ::: layout/reftests/w3c-css/submitted/contain/contain-size-flex-001.html:5 (Diff revision 2) > +<!DOCTYPE HTML> > +<html> > +<head> > + <meta charset="utf-8"> > + <title>CSS Test: 'contain: layout' on flexbox with no intrinsic s/layout/size/ ::: layout/reftests/w3c-css/submitted/contain/contain-size-flex-001.html:21 (Diff revision 2) > + flex-direction: column; > + border: 8px solid green; > + contain: size; > + } > + .inline { > + display: inline-block; Let's get rid of this "display: inline-block" value. It actually has zero effect, so it adds unnecessary complexity to the markup. (This was probably from Kyle's old version of the test... Not sure why he had it here. :)) (A flex container forces all of its children to have "blockified" display values, i.e. inline and inline-block become block, inline-table becomes table, etc. So these divs will end up being forced to display:block anyway, since they are children of a flex container, in spite of this attempt to make them inline-block.) ::: layout/reftests/w3c-css/submitted/contain/contain-size-flex-001.html:27 (Diff revision 2) > +<body> > + <div class="container"> > + <div class="inline"></div> > + <div class="inline"></div> > + </div> This test is good, but could stand to be a good deal more thorough. I'd suggest just treating it as a starting point for inspiration, and adding your own stuff to it. And really, I imagine a lot of the things you'll want to be testing a group of many very similar things, across all affected frame types. So it might be wise to invest some time right now in creating a single thorough chunk of boilerplate for one frame type (maybe this one), and then plan on copying it around with small tweaks (e.g. to the "display" value, or with s/<div>/<button>/, etc.) for other frames as you add support for them. Here are some scenarios worth testing (regardless of frame type) which would be good to include in this boilerplate "megatest": (0) contain:size with unspecified sizing properties (which you're already testing here) (1) contain:size with specified 'min-height' (2) contain:size with specified 'height' (3) contain:size with float:left and unspecified (auto) width (4) contain:size with float:left and specified width (e.g. width:20px) (5) contain:size with display:inline-flex and unspecified (auto) width (6) contain:size with display:inline-flex and specified width (e.g. width:20px) (7) contain:size with baseline-alignment across a containment boundary, e.g. abc<div class="container">inside</div>def (The text inside the container shouldn't do any baseline alignment with the text outside; we should align as if container were empty.) Also: for flexbox in particular, you probably want to test all of the above with both "flex-direction:row", as well as for "flex-direction: column" (which flip the flex container's internal axes). It probably makes sense to write a test that only bothers with one of these (e.g. the default, which is flex-direction:row), and then just create a copy of it to test the other one (by adding one line of CSS in the copy). For this test, I'd suggest styling all of the containers with CSS like the following: .container { contain: size; border: 2px solid black; color: transparent; margin-bottom: 2px; display: [whatever-frame-type-you're-testing]; } Note in particular the "color: transparent". This means you can throw some text inside of it (e.g. just "abc"), and you can be sure that the text *won't paint*, but will be involved in layout [i.e. it'll exercise your containment sizing/positioning logic]. Then, you can make the reference case have pretty much the *exact same style and content* as the testcase, except with no text inside the container elements, and without any need for contain:size and color:transparent. This setup lets you very directly validate that, indeed, "contain:size" does make the element lay out as if it had no contents, in all of the above scenarios (0 - 7)
Attachment #8984567 - Flags: review?(dholbert) → review-
(In reply to Daniel Holbert [:dholbert] from comment #28) > I'd suggest just treating [...] And really, I imagine a lot of the things > you'll want to be testing a group of many very similar things, across all > affected frame types. Er, sorry, this sentence doesn't entirely make sense... I messed up the wording when editing it. :) If it wasn't clear, I meant to say: "And really, I imagine you'll want to be testing a group of many very-similar things, across all affected frame types."
Comment on attachment 8984269 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/250060/#review256718 > Hmm, the first case (for the IsOrthogonalTo case) needs a small tweak. > > For context, that case gets triggered for content like the following: > data:text/html,abc<button style="writing-mode:vertical-rl">def > > With the patch as it stands right now: if we add contain:size to the button in that ^ testcase, this code would enter the IsOrthogonalTo case, and wouldn't do any special IsContainSize() stuff -- it'd continue using the "def" size to determine the button's baseline, and that would break the concept of size containment. > > So -- I think that first case should do something like the following: > nscoord orthogAscent = aButtonReflowInput.mStyleDisplay->IsContainSize() ? > 0 : contentsDesiredSize.ISize(wm); > aButtonDesiredSize.SetBlockStartAscent(orthogAscent); > > > That ensures that our baseline behavior is consistent (for this orthogonal writing-mode scenario) between a case where we have no content vs. a case where we have contain:size. Why do we want the ascent to be zero in the case that it's both orthogonal and size-contained? Is that still accurate in the case that we've got a border/padding/etc? The reason I have the second case returning childPos.B is because that value gets adjusted for the weird ComputedLogicalBorderPadding "convenient" value the function already has set up.
Comment on attachment 8984269 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/250060/#review256718 > Why do we want the ascent to be zero in the case that it's both orthogonal and size-contained? Is that still accurate in the case that we've got a border/padding/etc? The reason I have the second case returning childPos.B is because that value gets adjusted for the weird ComputedLogicalBorderPadding "convenient" value the function already has set up. We need the ascent to be whatever it would've been, if there were no children. (which may or may not be 0 -- you're right that border/padding may need to factor into it, and it may be something more subtle than 0) The second case (with childPos.B) looks reasonable here -- it's the first case (where we return contentsDesiredSize.ISize) that I'm concerned about. Maybe we should just swap the order of these first two cases, and return childPos.B(wm) unconditionally if we're size-contained?
Comment on attachment 8984269 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/250060/#review257944 ::: layout/forms/nsHTMLButtonControlFrame.cpp:269 (Diff revision 9) > + // Button is intrinsically sized and has size containment > + // it should have a BSize that is either zero or the minimum > + // specified BSize. This reads like a run-on sentence -- let's make it more clearly 2 different statements by adding a period at the end of the first line, and capitalizing "It" at the beginning of the second line. ::: layout/forms/nsHTMLButtonControlFrame.cpp:336 (Diff revision 9) > if (aButtonDesiredSize.GetWritingMode().IsOrthogonalTo(wm)) { > aButtonDesiredSize.SetBlockStartAscent(contentsDesiredSize.ISize(wm)); > + } else if (aButtonReflowInput.mStyleDisplay->IsContainSize()) { > + aButtonDesiredSize.SetBlockStartAscent(childPos.B(wm)); I'm still concerned about this, per comment 31. If we take the first clause here, we'll be leaking sizing information even if we're contain:size. ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001-ref.html:11 (Diff revision 9) > + .ib { > + font: 20px monospace; > + border: 3px solid green; > + display: inline-block; > + } > + .ib2 { > + font: 20px monospace; > + height: 100px; > + width: 0; > + border: 3px solid green; > + display: inline-block; > + } > + .ib3 { > + font: 20px monospace; > + height: 0; > + width: 100px; > + border: 3px solid green; > + display: inline-block; > + } These styles are all mostly repeated. It's probably cleaner & more maintainable to group the common ones in a single style rule like: button { ...common stuff... } And then put specific customizations in more clearly named rules. Perhaps... .w { width: 100px; } .h { height: 100px; } ...to match what you've got in the testcase? Also: as noted below for the testcase, I'm not sure "display:inline-block" is a useful/necessary thing here, so let's drop that and the "ib" naming if we can. Also: are the "height: 0" / "width: 0" declarations necessary? I suspect not (those should be the defaults), so it's probably cleaner & more understandable if you remove those. (We get the effect of "height:0"/"width:0" by virtue of simply having no content inside the buttons in this file.) ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001-ref.html:40 (Diff revision 9) > +outside before<button class="ib"></button>outside after > +<br> > +outside before<button class="ib contain"></button>outside after > +<br> > +<button class="ib2 contain"></button> > +<br> > +<button class="ib3 contain"></button> > +<br> > +<button class="ib4 contain"></button> > +<br> > +outside before<button class="ib contain" style="writing-mode: vertical-lr;"></button>outside after There's no "contain" selector defined in this file, so let's drop the class="... contain" from the buttons here. ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:10 (Diff revision 9) > + body { > + margin: 0; > + } Let's get rid of this body { margin: 0 } rule. It was present in some of Kyle's tests, but we recently got pushpack from someone involved with w3c tests, (correctly) noting that it's unnecessary & unrelated to what's being tested: https://bugzilla.mozilla.org/show_bug.cgi?id=1170781#c60 So for new tests, let's not bother with that. ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:14 (Diff revision 9) > + .ib { > + font: 20px monospace; > + border: 3px solid green; > + color: transparent; > + display: inline-block; Is "display: inline-block" actually necessary for the buttons here? I wouldn't think it is. (And the reference case's rendering doesn't seem to be influenced by it.) In the interests of simplicity, probably better to remove that, unless it's needed. Also: It looks like every single button in this test has class="ib contain" -- so maybe you really want to just fold all the "ib" / "contain" styling into a button { ... } rule? ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:20 (Diff revision 9) > + .h { > + height: 100px; > + } > + .w { > + width: 100px; > + } We should test <button> with min-width and min-height, as well. (At least one button with each, just to be sure that we honor those.) ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:35 (Diff revision 9) > + > +<br> > + > +outside before<button class="ib contain" style="background:red;"> > + <div> > + CSS Test: Buttons with 'contain:size' should not let intneral content affect their baseline alignment. typo: s/intneral/internal/ ::: layout/reftests/w3c-css/submitted/contain/reftest.list:15 (Diff revision 9) > +== contain-size-flex-001.html contain-size-flex-001-ref.html > +== contain-size-block-001.html contain-size-block-001-ref.html > +== contain-size-inline-block-001.html contain-size-inline-block-001-ref.html > +== contain-size-button-001.html contain-size-button-001-ref.html Most of these reftest.list changes belong in later patches, where the corresponding files are added.
Attachment #8984269 - Flags: review?(dholbert) → review-
Sorry, I'm trying to fix my pushed files again. It was all fine locally but pushing messed things up and I lost a bunch of work... I'm not sure why this is so difficult for me to do :(
Comment on attachment 8984529 [details] Bug 1467209 - Implement contain:size for blockFrame, add reftests. https://reviewboard.mozilla.org/r/250400/#review257976 ::: layout/generic/nsBlockFrame.cpp:519 (Diff revision 5) > nscoord > nsBlockFrame::GetCaretBaseline() const > { > nsRect contentRect = GetContentRect(); > nsMargin bp = GetUsedBorderAndPadding(); > - > if (!mLines.empty()) { > ConstLineIterator line = LinesBegin(); Unnecessary/unrelated whitespace change -- please revert this. ::: layout/generic/nsBlockFrame.cpp:706 (Diff revision 5) > CheckIntrinsicCacheAgainstShrinkWrapState(); > > if (mMinWidth != NS_INTRINSIC_WIDTH_UNKNOWN) > return mMinWidth; > > + if (this->StyleDisplay()->IsContainSize()) { Drop "this->" -- it's implied. (Here and in GetPrefISize) ::: layout/reftests/w3c-css/submitted/contain/contain-size-block-001-ref.html:8 (Diff revision 5) > + body { > + margin: 0; > + } As noted for the previous part, let's remove this "margin: 0" styling, to the extent possible. (I don't think this should make an impact on any of the tests' renderings, as long as we remove it from both testcase & reference case.) ::: layout/reftests/w3c-css/submitted/contain/contain-size-block-002.html:5 (Diff revision 5) > + <title>CSS Test: 'contain: size' on auto-sized block element should cause > + the element to have zero height.</title> The contain-size-block-002 / 003 (and inline-block-002 / 003) testcases need to be listed in reftest.list in order for our reftest harness to run them. (It looks like the -001 versions are added to the reference case in the previous patch right now, which should happen in this patch, and then we should add the 002 & 003 ones as well.)
Attachment #8984529 - Flags: review?(dholbert) → review-
Comment on attachment 8984529 [details] Bug 1467209 - Implement contain:size for blockFrame, add reftests. https://reviewboard.mozilla.org/r/250400/#review257976 > The contain-size-block-002 / 003 (and inline-block-002 / 003) testcases need to be listed in reftest.list in order for our reftest harness to run them. > > (It looks like the -001 versions are added to the reference case in the previous patch right now, which should happen in this patch, and then we should add the 002 & 003 ones as well.) I was working on combining 002 and 003 into 001 so I can use 001 as my 'generic' boilerplate test and just modify the frame type. Locally I removed 002 and 003 from reftest.list and my working directory. Should I restore them and keep their testing functionality separate from 001?
(In reply to Morgan Reschenberg [:mreschenberg] from comment #42) > I was working on combining 002 and 003 into 001 so I can use 001 as my > 'generic' boilerplate test and just modify the frame type. Locally I removed > 002 and 003 from reftest.list and my working directory. Should I restore > them and keep their testing functionality separate from 001? Your plan sounds good! No need to restore them - I was just noting the mismatch between reftest.list and test files that I was seeing on mozreview.
Summary: Implement contain:size for FlexConainerFrame, HTMLButonControlFrame, and BlockFrame → Implement contain:size for FlexContainerFrame, HTMLButtonControlFrame, and BlockFrame
Attachment #8984268 - Flags: checkin?(mreschenberg)
Attachment #8984268 - Flags: checkin?(mreschenberg) → checkin?(dholbert)
Comment on attachment 8984269 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/250060/#review258922 ::: layout/forms/nsHTMLButtonControlFrame.cpp:336 (Diff revision 12) > if (aButtonDesiredSize.GetWritingMode().IsOrthogonalTo(wm)) { > - aButtonDesiredSize.SetBlockStartAscent(contentsDesiredSize.ISize(wm)); > + nscoord orthogAscent = aButtonReflowInput.mStyleDisplay->IsContainSize() > + ? childPos.B(wm) > + : contentsDesiredSize.ISize(wm); > + aButtonDesiredSize.SetBlockStartAscent(orthogAscent); > + } else if (aButtonReflowInput.mStyleDisplay->IsContainSize()) { > + aButtonDesiredSize.SetBlockStartAscent(childPos.B(wm)); > } else { Nit: this works, but I don't have a good mental model for why it makes sense (with the childPos.B(wm) vs. childDesiredSize.ISize(wm) being the two options) This is kinda my own fault because I'd suggested this structure (with the ternary expression) up above, when I was thinking that childDesiredSize.ISize() would be 0 when there are no contents -- at that point I was thinking it'd be "0 or ISize()", not "B() or ISize()". Now that it's B() or ISize(), it feels kinda haphazard (a position in one axis vs. a length in the other axis). To avoid this awkwardness, I think we should instead swap the order of the first two cases here, to share code/logic better and to preserve the original code better & make this more sensible. So, I think that lets this simplify to: if (aButtonReflowInput.mStyleDisplay->IsContainSize()) { aButtonDesiredSize.SetBlockStartAscent(childPos.B(wm)); } else if (aButtonDesiredSize.GetWritingMode().IsOrthogonalTo(wm)) { ... } else { ... } (here, "..." are the contents of the original if/else cascade -- i.e. what exists in mozilla-central right now) How does that sound? I think it's more understandable, from my perspective. ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:15 (Diff revision 12) > + .minHeight { > + /* Some info stil leaks from text contents, so this is hack-y for right now */ > + vertical-align: top; > + min-height: 50px; > + background: lightblue; > + } > + .height { > + /* Some info stil leaks from text contents, so this is hack-y for right now */ > + vertical-align: top; Typo: s/stil/still/ But: is this vertical-align styling still needed? (It doesn't seem to affect the rendering of this testcase for me, in a local build with your patches and with the pref enabled. There might be some subtle linux/mac difference though.) If it is still needed, we should either: - investigate & fix the issue here (and fix the test to remove hackiness) - ...or spin off a followup bug and mention the bug number here. If we take the latter route (punting to a followup bug), then we probably shouldn't put this testcase in this w3c-css/submitted directory right now, at least not with the hacky css/comment. We don't really want to be submitting tests upstream to other browsers with Firefox-specific hackarounds for known bugs. You could either put it somewhere else within layout/reftests (with its current hacks), *or* keep it where it is and remove the hacks, and annotate it as "fails" in reftest.list. But, again, maybe this isn't needed anymore anyway? ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:87 (Diff revision 12) > + <br> > + > + <button class="width">CSS Test: A size-contained button with specified width should render at given width and zero height regardless of content.</button> > + <br> > + > + s<button class="orthog">CSS Test: A size-contained button with vertical text should perform baseline alignment as if the container were empty. </button>endtext Nit: trailing space here. ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:89 (Diff revision 12) > + <button class="width">CSS Test: A size-contained button with specified width should render at given width and zero height regardless of content.</button> > + <br> > + > + s<button class="orthog">CSS Test: A size-contained button with vertical text should perform baseline alignment as if the container were empty. </button>endtext > +</body> > +</html> Nit: please add a newline character at the end of this file and the other new test files, as well as at the end of reftest.list. Right now, this file ends immediately after the final ">" of "</html>", but our coding style guidelines say[1] you should add a final newline character after that. (Not necessarily an actual trailing blank line, but just a final newline character.) Otherwise, the raw text view of the patch complains that there is "No newline at end of file" (visible via commands like "hg export" that output the raw text, e.g. here[2] at the mozreview-hosted version of the raw text patch) (Sadly MozReview doesn't show this in its graphical review UI.) This matters because if you end a file without a newline character, and then someone else comes along and adds an additional line of content to the end of the file, then HG / searchfox / etc. will show their patch as modifying your (previously-)final line, even though they made no substantial changes to that line. Their change will inadvertantly be editing your final line by adding a newline character at the end of it. Hence, it's nice to prevent this confusing scenario by just ending the file with a newline in the first place. [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#General_C.2FC.2B.2B_Practices [2] https://reviewboard-hg.mozilla.org/gecko/raw-rev/e2395e4e08eb25a104f63eca8c2d6e98e9f13ae5
Attachment #8984269 - Flags: review?(dholbert) → review-
Comment on attachment 8984268 [details] Bug 1467209 - Update some baseline-querying utility functions to bail on frames that have 'contain:size'. Canceling the checkin-requested flag -- that per-attachment flag is kind of a "wart" in bugzilla that we used in the past but don't really use right now. MozReview/Autoland only support landing all of a bug's patches together at once, so even if this first patch is ready to land, it would get a bit messy to check it in now without the rest. (If you indeed *would* like to land the first patch now, ahead of the rest (e.g. to reduce the number of patches you're juggling in your local repo), we could still hypothetically do that, but you'd want to spin it off to its own bug probably - which is totally fine if you'd like. As we say sometimes, "bugs are cheap". :))
Attachment #8984268 - Flags: checkin?(dholbert)
Comment on attachment 8984529 [details] Bug 1467209 - Implement contain:size for blockFrame, add reftests. https://reviewboard.mozilla.org/r/250400/#review258950 ::: layout/generic/nsBlockFrame.cpp:475 (Diff revision 8) > nscoord > nsBlockFrame::GetLogicalBaseline(WritingMode aWM) const > { > + if (StyleDisplay()->IsContainSize()) { > + return BSize(aWM); > + } > auto lastBaseline = > BaselineBOffset(aWM, BaselineSharingGroup::eLast, AlignmentContext::eInline); > return BSize(aWM) - lastBaseline; I'm not sure this is the best place to add this -- I suspect we'd prefer to share code here by burying this check a few levels down (inside of BaselineBOffset() implementation, closer to where we consider the actual content. (To the extent possible, we should try to make it clear why we're doing an IsContainSize() specific thing, and how that's directly analogous to having no content, in the context of the given function. This function doesn't really consider the content directly, which is why it feels to me like the wrong spot to be inserting this check.) (And actually, it looks like BaselineBOffset is implemented in terms of GetNaturalBaselineBOffset() [1], so maybe if you adjust that as I'm suggesting below, then this will Just Work... [1] https://dxr.mozilla.org/mozilla-central/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/layout/generic/nsIFrameInlines.h#146-160 ::: layout/generic/nsBlockFrame.cpp:491 (Diff revision 8) > - if (aBaselineGroup == BaselineSharingGroup::eFirst) { > + if (aBaselineGroup == BaselineSharingGroup::eFirst || StyleDisplay()->IsContainSize()) { > return nsLayoutUtils::GetFirstLineBaseline(aWM, this, aBaseline); > } > > for (ConstReverseLineIterator line = LinesRBegin(), line_end = LinesREnd(); > line != line_end; ++line) { From the principle of "behave as if we have no content", I think we'd really want to add: if (StyleDisplay()->IsContainSize()) { return false; } ...as a second, distinct, early-return clause here. That's a more direct short-circuit to behave as if we have no content. (It basically makes us behave the same as if we skipped the "for" loop below, due to having no content.) ::: layout/reftests/w3c-css/submitted/contain/contain-size-block-001.html:4 (Diff revision 8) > + <meta charset="utf-8"> > + <title>CSS Test: 'contain: size' on block elements should zero the height if unspecified, and should ensure baseline alignment consistency with empty (0x0) objects of the same type. If max-width is specified, 'contain: size' should constrain the block to the specified size.</title> > + <link rel="author" title="Morgan Rae Reschenberg" href="mailto:mreschenberg@berkeley.edu"> > + <link rel="match" href="contain-size-block-001-ref.html"> In each of the testcases (not the reference cases) in all of your patches here, you need one more <link> tag, to point at the bit of spec text that the test is testing, roughly. (This is used to automagically insert links into w3c specs, to point to tests for each chunk of spec text. It's also used to help people figure out the rationale for what a test is asserting.) You could use the following in all of your tests, I think: <link rel="help" href="https://drafts.csswg.org/css-contain/#containment-size"> ::: layout/reftests/w3c-css/submitted/contain/contain-size-block-001.html:39 (Diff revision 8) > + .iFlexBasic { > + display: inline-flex; > + width: auto; > + } > + .iFlexWidth { > + display: inline-flex; > + width: 50px; > + } This doesn't really make sense in a contain-size-block-* testcase. (It made sense in a button testcase, because we can style a button with "display:inline-flex". But it doesn't make sense in a block testcase, because by definition, a block cannot be display:inline-flex. A block would have display:block or inline-block.) ::: layout/reftests/w3c-css/submitted/contain/contain-size-inline-block-001.html:9 (Diff revision 8) > + div { > + display: inline-block; > + contain:size; > + border: 1em solid green; > + background: red; > + color: transparent; > + } > + .minHeight { > + min-height: 50px; > + } > + .height { > + height: 50px; > + } > + .minWidth { > + min-width: 50px; > + } > + .width { > + width: 50px; > + } > + .floatLBasic { > + float: left; > + width: auto; Nit: you could probably get rid of the "float" chunks from this inline-block testcase (and from other inline-$WHATEVER testcases down the line). By definition, "float:left" will promote any computed "display:inline-$FOO" element to instead just have "display:$FOO". So in fact, the chunk of the test with floats here is really just re-testing contain:size;float:left;display:block -- it's not testing anything inline-block specific. ::: layout/reftests/w3c-css/submitted/contain/contain-size-inline-block-001.html:36 (Diff revision 8) > + .iFlex{ > + display: inline-flex; > + width: auto; > + } > + .iFlexWidth { > + display: inline-flex; > + width: 50px; > + } As above, this display:inline-flex stuff probably wants to be removed from this inline-block testcase.
Attachment #8984529 - Flags: review?(dholbert) → review-
Comment on attachment 8984567 [details] Bug 1467209 - Implement contain:size for flexContainerFrame, add reftests. https://reviewboard.mozilla.org/r/250442/#review258952 ::: layout/generic/nsFlexContainerFrame.cpp:3998 (Diff revision 7) > return aAvailableBSizeForContent; > } > return std::min(aTentativeMainSize, largestLineOuterSize); > } > > + // Column-orientated case, with size-containment: s/orientated/oriented/ ::: layout/generic/nsFlexContainerFrame.cpp:4062 (Diff revision 7) > return aAvailableBSizeForContent; > } > return std::min(effectiveComputedBSize, aSumLineCrossSizes); > } > > + // Row-orientated case, with size-containment: s/orientated/oriented/ ::: layout/generic/nsFlexContainerFrame.cpp:4900 (Diff revision 7) > NS_WARNING_ASSERTION( > lines.getFirst()->IsEmpty(), > "Have flex items but didn't get an ascent - that's odd (or there are " > "just gigantic sizes involved)"); This NS_WARNING_ASSERTION needs to include... || aReflowInput.mStyleDisplay->IsContainSize() ...in its asserted condition. Otherwise, it gets triggered (and spams some warning output to the terminal) for any contain:size flex container I think. I noticed this happening in my terminal, at least. ::: layout/generic/nsFlexContainerFrame.cpp:5195 (Diff revision 7) > { > DISPLAY_MIN_WIDTH(this, mCachedMinISize); > - if (mCachedMinISize == NS_INTRINSIC_WIDTH_UNKNOWN) { > +if (mCachedMinISize == NS_INTRINSIC_WIDTH_UNKNOWN) { This patch is accidentally deindenting the "if" all the way to the left here. (shown as a faint pink "<<" on the left side of MozReview) Please revert that change and restore the indentation. ::: layout/generic/nsFlexContainerFrame.cpp:5198 (Diff revision 7) > - mCachedMinISize = IntrinsicISize(aRenderingContext, > - nsLayoutUtils::MIN_ISIZE); > + mCachedMinISize = StyleDisplay()->IsContainSize() > + ? 0 > + : IntrinsicISize(aRenderingContext, nsLayoutUtils::MIN_ISIZE); The indentation is arbitrarily large here. Just indent 2 spaces with respect to the line above it. mCachedMinISize = StyleDisplay()->IsContainSize() ? stuff : otherstuff; ::: layout/generic/nsFlexContainerFrame.cpp:5211 (Diff revision 7) > - mCachedPrefISize = IntrinsicISize(aRenderingContext, > - nsLayoutUtils::PREF_ISIZE); > + mCachedPrefISize = StyleDisplay()->IsContainSize() > + ? 0 > + : IntrinsicISize(aRenderingContext, nsLayoutUtils::PREF_ISIZE); Here as well - only 2 spaces of indentation beyond the line above, please. ::: layout/reftests/w3c-css/submitted/contain/contain-size-flex-001-ref.html:62 (Diff revision 7) > + <div class="floatLWidth-ref"></div> > + <br style="clear:both;"> > + > + <div class="iFlexBasic-ref"></div> > + <br style="clear:both;"> You don't need this second "clear:both" - there are no floats for it to clear. The first one (up a few lines) makes sense because it's directly after a floated thing, but then there are no more floats after that point for the second one to clear. ::: layout/reftests/w3c-css/submitted/contain/contain-size-flex-001.html:38 (Diff revision 7) > + .iFlexBasic { > + /* This is also hack-y for the same reason as the button tests. > + Some info about text is leaking causing weird positioning/margins. */ > + vertical-align: top; As noted for the button testcase: it's somewhat problematic to put hackarounds in testcases that we're sharing with other vendors. Needs a followup bug, and either the hackaround should be removed (with the test marked as "fails" in reftest.list if it fails), or the test should be moved (e.g. to layout/reftests/bugs) until we've addressed the hypothetical followup bug. ::: layout/reftests/w3c-css/submitted/contain/contain-size-flex-001.html:54 (Diff revision 7) > + > + outside before<div>CSS Test: A size-contained flex element should perform baseline alignment as if the container were empty.</div>outside after > + <br> This part should probably be removed (here as well as in the related "contain-size-block-001.html" test). It's confusing/misleading right now because it says it's testing baseline alignment, but it's really not. Baseline only happens between adjacent *inline-level* things (e.g. raw text and elements with display:inline-block or display:inline-flex). Not between adjacent *block-level* things (display:block, display:flex). Still: you can test baseline alignment lower down in this test on the "iFlexBasic" etc. pieces, as noted below. ::: layout/reftests/w3c-css/submitted/contain/contain-size-flex-001.html:73 (Diff revision 7) > + <div class="floatLWidth">CSS Test: A size-contained floated flex element with specified width and no specified height should render at given width and 0 height regardless of content.</div> > + <br style="clear:both;"> > + > + <div class="iFlexBasic">CSS Test: A size-contained inline-flex element with auto width and no specified height should render at 0px by 0px regardless of content.</div> > + <br style="clear:both;"> As noted for the reference case: you don't need this second "clear:both" here, so let's remove it to simplify. ::: layout/reftests/w3c-css/submitted/contain/contain-size-flex-001.html:76 (Diff revision 7) > + <div class="iFlexBasic">CSS Test: A size-contained inline-flex element with auto width and no specified height should render at 0px by 0px regardless of content.</div> > + <br style="clear:both;"> > + > + <div class="iFlexWidth">CSS Test: A size-contained inline-flex element with specified width and no specified height should render at given width and 0 height regardless of content.</div> For these "iFlex" tests (for display:inline-flex), you probably want to put a character or two before / after them, so that you're testing their baseline alignment. (The test title mentions baseline alignment but I think this is the only part that would really be testing that.) ::: layout/reftests/w3c-css/submitted/contain/reftest.list:15 (Diff revision 7) > -== contain-size-block-001.html contain-size-block-001-ref.html > +== contain-size-flex-001.html contain-size-flex-001-ref.html > -== contain-size-inline-block-001.html contain-size-inline-block-001-ref.html > -== contain-size-button-001.html contain-size-button-001-ref.html Right now this commit is removing the other tests' lines from reftest.list - you probably didn't mean for that to happen. :)
Attachment #8984567 - Flags: review?(dholbert) → review-
Attachment #8984269 - Attachment is obsolete: true
Attachment #8984529 - Attachment is obsolete: true
Attachment #8984567 - Attachment is obsolete: true
Comment on attachment 8984268 [details] Bug 1467209 - Update some baseline-querying utility functions to bail on frames that have 'contain:size'. https://reviewboard.mozilla.org/r/250058/#review259400 ::: layout/style/nsStyleStruct.h:2421 (Diff revision 4) > > bool IsRubyDisplayType() const { > return IsRubyDisplayType(mDisplay); > } > > - bool IsOutOfFlowStyle() const { > + bool IsInternalRubyDisplayType() const { This, along with the above changes, are helper methods Yusuf added for isContainPaint that I also need; they shouldn't cause merge conflicts if landing together.
Comment on attachment 8984269 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/250060/#review258922 > Typo: s/stil/still/ > > But: is this vertical-align styling still needed? (It doesn't seem to affect the rendering of this testcase for me, in a local build with your patches and with the pref enabled. There might be some subtle linux/mac difference though.) > > If it is still needed, we should either: > - investigate & fix the issue here (and fix the test to remove hackiness) > - ...or spin off a followup bug and mention the bug number here. > > If we take the latter route (punting to a followup bug), then we probably shouldn't put this testcase in this w3c-css/submitted directory right now, at least not with the hacky css/comment. We don't really want to be submitting tests upstream to other browsers with Firefox-specific hackarounds for known bugs. You could either put it somewhere else within layout/reftests (with its current hacks), *or* keep it where it is and remove the hacks, and annotate it as "fails" in reftest.list. > > But, again, maybe this isn't needed anymore anyway? In your local build did you pull everything and run the reftests from the head (ie. the flexContainer commit) or did you run them just from here?
Comment on attachment 8984269 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/250060/#review258922 > In your local build did you pull everything and run the reftests from the head (ie. the flexContainer commit) or did you run them just from here? (I'm pretty sure I used the "hg pull" command from the final patch here.)
BTW -- it looks like you used the "discard" option on MozReview (it says "This change has been discarded"), and you don't typically need to do that. Stuff should just work if you re-push to review (and you'll get handy interdiffs between revisions, etc). Let me know if it's not working, though! (If it shows a way to "un-discard" [possibly called "reopen" on mozreview], that might be a good idea & might make it easier to review subsequent revisions with the old version & interdiffs from it handy -- but no big deal if that's not easy/obvious.)
(Oh, maybe the "discarded" thing happened automatically as a result of pushing the first patch without the other ones. I think there's a way to manually discard and I was thinking you'd done that, but maybe not. :))
Yeah, that was an accident (mostly because the changes I made between the monolithic patch and these things can't be pulled onto each other at the moment, but I need the 'baseline'/utility patch as the root to test both :p) I think I'm going to spin off a helper bug that we can land for that first patch so I don't have to keep messing with pulling/rebasing when switching between bugs if that's okay (?)
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #60) > Comment on attachment 8984269 [details] > Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. > > https://reviewboard.mozilla.org/r/250060/#review258922 > > > In your local build did you pull everything and run the reftests from the head (ie. the flexContainer commit) or did you run them just from here? > > (I'm pretty sure I used the "hg pull" command from the final patch here.) Hmm okay. I know the button functionality pulls from the blockFrame functionality re: baseline alignment so maybe that's why they pass if you pull the whole patch set. Either way (ie. if I pull from just the button patch or if I pull the whole stack), I can't seem to get the test to pass locally without specifying 'vertical-align:top' in both the ref and test case. Like you said, this doesn't really work because it means my baseline code is broken somehow. Is there a way I can push my reftests to try to see if they pass on other platforms/if its an isolated macos issue?
> I think I'm going to spin off a helper bug that we can land for that first patch Sounds good! > Is there a way I can push my reftests to try to see if they pass on other platforms/if its an isolated macos issue? You can use "./mach try [args]" to get a try run of whatever commit you're currently updated to. (So e.g. you could make an experimental tweak to a test or whatever, and then run ./mach try to push that directly to try, bypassing reviewboard.) (Side note: "./mach try" will generate a temporary empty child-commit which *only* exists to encapsulate the test/platform-selection syntax. It removes this commit before it completes, so your tree should end up the way it started, but in the unlikely scenario that you interrupt it and end up with a weird "try: ..." commit in your history, that's what happened. :)) You can figure out the right ./mach try command at https://mozilla-releng.net/trychooser/ -- e.g. if I click "debug only", "linux" and "reftest", then I end up with ./mach try -b d -p linux -u reftest -t none
Flags: needinfo?(dholbert)
Comment on attachment 8984567 [details] Bug 1467209 - Implement contain:size for flexContainerFrame, add reftests. https://reviewboard.mozilla.org/r/250442/#review258952 > Right now this commit is removing the other tests' lines from reftest.list - you probably didn't mean for that to happen. :) I wasn't sure how much overlap the patches were supposed to have since I was buidling them as disjoint pieces. Should I build up the reftest file from patch to patch within the same bug?
(In reply to Morgan Reschenberg [:morgan] from comment #66) > I wasn't sure how much overlap the patches were supposed to have since I was > buidling them as disjoint pieces. Should I build up the reftest file from > patch to patch within the same bug? The ideal option is (a) for each commit to add its own tests, and add lines for those tests to reftest.list. As you note, though, that can cause some merge trouble. So: less-ideal, but sometimes saner from avoiding merge trouble, you're welcome to: (b) add all the tests in a final standalone commit ...or: (c) add all the reftest.list lines in the final commit (It seemed like your patches were going with the option (a), and they seemed to be doing that successfully, except that my review comment was observing that your final commit was accidentally *deleting* some lines from reftest.list, which makes less sense than the above options.)
Attachment #8987878 - Flags: review?(dholbert)
Attachment #8987879 - Flags: review?(dholbert)
Attachment #8987880 - Flags: review?(dholbert)
Depends on: 1471267, 1471274
Those (^) are two spun-off bugs for the basic utility functions needed so I can move on with size-contianment on other frames and the issue with flex/button containers being affected by inner text (respectively).
Attachment #8984268 - Attachment is obsolete: true
In the process of rebasing these to match the current copy of central. Please hold off on review until they get re-pushed so feedback doesn't get thrown away in the process :)
Attachment #8987878 - Attachment is obsolete: true
Attachment #8987878 - Flags: review?(dholbert)
Attachment #8987879 - Attachment is obsolete: true
Attachment #8987879 - Flags: review?(dholbert)
Attachment #8987880 - Attachment is obsolete: true
Attachment #8987880 - Flags: review?(dholbert)
Attachment #8988857 - Flags: review?(dholbert)
Attachment #8988858 - Flags: review?(dholbert)
Attachment #8988859 - Flags: review?(dholbert)
Rebase finished
Comment on attachment 8988857 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/253980/#review261160 ::: layout/forms/nsHTMLButtonControlFrame.cpp:303 (Diff revision 2) > } > > // Center child in the block-direction in the button > // (technically, inside of the button's focus-padding area) > - nscoord extraSpace = buttonContentBox.BSize(wm) - > + nscoord extraSpace; > + if (aButtonReflowInput.mStyleDisplay->IsContainSize()) { I think this needs a comment, but I'm not sure exactly how to phrase it. Maybe something to the effect of: If we are size-contained, we don't want our inner content to contribute to our ascent (which extraSpace influences later on). To avoid that, we consider only our content box BSize.
Comment on attachment 8988857 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/253980/#review261160 > I think this needs a comment, but I'm not sure exactly how to phrase it. Maybe something to the effect of: > > If we are size-contained, we don't want our inner content to contribute to our ascent (which extraSpace influences later on). To avoid that, we consider only our content box BSize. Something like this maybe: "If we're size-contained, pretend our contents had 0 height (as they would, if we had no children)" However, having said that -- I just realized we *do* care about `extraSpace` taking the child into account here, because we still want to draw the text in the right place, which requires us to have extraSpace (and childPos) set correctly. So really, instead of neutralizing the child's contribution to extraSpace, we probably need to do something more targeted at the ascent. Maybe the final "if (aButtonReflowInput.mStyleDisplay->IsContainSize()) { ...}" case should just contain some arithmetic rather than trusting childPos.B(wm) -- i.e. in that clause, maybe we should just back-compute what childPos.B(wm) *would have been* if we had no contents, or something like that.
Comment on attachment 8988857 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/253980/#review261160 > Something like this maybe: > "If we're size-contained, pretend our contents had 0 height (as they would, if we had no children)" > > However, having said that -- I just realized we *do* care about `extraSpace` taking the child into account here, because we still want to draw the text in the right place, which requires us to have extraSpace (and childPos) set correctly. > > So really, instead of neutralizing the child's contribution to extraSpace, we probably need to do something more targeted at the ascent. Maybe the final "if (aButtonReflowInput.mStyleDisplay->IsContainSize()) { ...}" case should just contain some arithmetic rather than trusting childPos.B(wm) -- i.e. in that clause, maybe we should just back-compute what childPos.B(wm) *would have been* if we had no contents, or something like that. e.g. maybe something like this (I'm skimming the math so I might've missed something): nscoord bPosOfHypotheticalEmptyChild = (buttonContentBox.BSize(wm) / 2) + clbp.BStart(wm); aButtonDesiredSize.SetBlockStartAscent(bPosOfHypotheticalEmptyChild);
Comment on attachment 8988857 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/253980/#review261400 ::: layout/forms/nsHTMLButtonControlFrame.cpp:336 (Diff revision 3) > - if (aButtonDesiredSize.GetWritingMode().IsOrthogonalTo(wm)) { > + if (aButtonReflowInput.mStyleDisplay->IsContainSize()) { > + // If we're size-contained, pretend our contents had 0 height > + // (as they would, if we had no children) > + nscoord containAscent = (buttonContentBox.BSize(wm) / 2) + clbp.BStart(wm); > + aButtonDesiredSize.SetBlockStartAscent(containAscent); > + } else if (aButtonDesiredSize.GetWritingMode().IsOrthogonalTo(wm)) { > aButtonDesiredSize.SetBlockStartAscent(contentsDesiredSize.ISize(wm)); > } else { > aButtonDesiredSize.SetBlockStartAscent(contentsDesiredSize.BlockStartAscent() + > childPos.B(wm)); > } Please tweak/extend this comment to make it clear that the arithmetic here is meant to produce the ascent that we would get in the general (non-orthogonal) "else" clause, if we had no contents. ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001-ref.html:21 (Diff revision 3) > + .floatLWidth-ref { > + float: left; > + width: 50px; > + height: 0; > + } > + .iFlexBasic-ref { > + display: inline-flex; > + } > + .iFlexWidth-ref { > + display: inline-flex; > + width: 50px; > + height: 0; > + } Let's get rid of the two "height: 0" usages here. Those are unnecessary/redundant, I think. (The point here is to verify that buttons should size themselves *as if they had no contents*. So the key difference in this file is that you've removed their contents. You don't need height:0 on top of that; the emptiness is sufficient on its own to produce the expected rendering.) ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:5 (Diff revision 3) > + <title>CSS Test: 'contain: size' on buttons should zero the height and/or width if unspecified, and should ensure baseline alignment consistency with empty (0x0) objects of the same type.</title> > + <link rel="author" title="Morgan Rae Reschenberg" href="mailto:mreschenberg@berkeley.edu"> > + <link rel="match" href="contain-size-button-001-ref.html"> This needs a link rel="help" tag. This should work: <link rel="help" href="https://drafts.csswg.org/css-contain/#containment-size"> ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:5 (Diff revision 3) > +<!DOCTYPE HTML> > +<html> > +<head> > + <meta charset="utf-8"> > + <title>CSS Test: 'contain: size' on buttons should zero the height and/or width if unspecified, and should ensure baseline alignment consistency with empty (0x0) objects of the same type.</title> Let's refine this title a little -- the mentions of "zero" / "0x0" are kinda iffy here, since default-sized buttons actually occupy a nonzero amount of space (even if they have style="width:0;height:0"), because of their default borders & padding. Suggested replacement wording: CSS Test: 'contain: size' on buttons should cause them to be sized & baseline-aligned as if they had no contents ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:31 (Diff revision 3) > + .floatLBasic { > + float: left; > + width: auto; > + } > + .floatLWidth { > + float: left; > + width: 50px; > + } > + .iFlexBasic { > + display: inline-flex; > + width: auto; Drop the two "width: auto" usages here -- it's unnecessary, because width:auto is the default value.
Attachment #8988857 - Flags: review?(dholbert) → review+
Comment on attachment 8988857 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/253980/#review261412 ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:1 (Diff revision 3) > +<!DOCTYPE HTML> (general test request, not about any line in particular): We also need to test that a button with *visible* contents (and a generous specified size) renders normally, regardless of whether it has contain:size -- i.e. a test that would've caught the extraSpace issue noted in comment 85. e.g. comparing this, with & without size containment (and with "a" being black or anything nontransparent): <button style="height:50px; width:50px">a</button> This could be part of this test, or in a separate test - either way. (If you want to make it a separate test, then feel free to put it in its own commit at the end of the patch queue, if that makes your life easier rebasing-wise.)
Comment on attachment 8988857 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/253980/#review261400 > Let's refine this title a little -- the mentions of "zero" / "0x0" are kinda iffy here, since default-sized buttons actually occupy a nonzero amount of space (even if they have style="width:0;height:0"), because of their default borders & padding. > > Suggested replacement wording: > CSS Test: 'contain: size' on buttons should cause them to be sized & baseline-aligned as if they had no contents I was using this standardised wording across block/flex tests, too, so I'll make sure they get updated as well. I wasn't sure how specific to be, since in some frame types containment zeroes both height and width (and in some it zeroes in only one direction). I think the reword is more accurate though (and descriptive on future behaviour, should these tests need modifying).
Comment on attachment 8988857 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/253980/#review261438 ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:21 (Diff revision 3) > + min-width: 50px; > + background: lightblue; > + } > + .width { > + width: 50px; > + background: lightblue; I'm going to remove this for both .width and .minWidth; the height should automatically be zero without content (which is what we're trying to emulate), so if any height renders and the background shows, the background should be red to indicate an error.
Comment on attachment 8988857 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/253980/#review261438 > I'm going to remove this for both .width and .minWidth; the height should automatically be zero without content (which is what we're trying to emulate), so if any height renders and the background shows, the background should be red to indicate an error. sounds good!
Comment on attachment 8988858 [details] Bug 1467209 - Implement contain:size for blockFrame. https://reviewboard.mozilla.org/r/253982/#review261444 r=me on this patch, with nits addressed ::: layout/reftests/w3c-css/submitted/contain/contain-size-block-001-ref.html:19 (Diff revision 3) > + .floatLBasic-ref { > + float: left; > + width: 0; > + } Like in the button patch: let's remove this "width:0" here -- this element should end up with width:0 anyway because it's floated & empty. ::: layout/reftests/w3c-css/submitted/contain/contain-size-block-001.html:5 (Diff revision 3) > +<!DOCTYPE HTML> > +<html> > +<head> > + <meta charset="utf-8"> > + <title>CSS Test: 'contain: size' on block elements should zero the height if unspecified, and should ensure baseline alignment consistency with empty (0x0) objects of the same type. If max-width is specified, 'contain: size' should constrain the block to the specified size.</title> (I'm assuming this'll be tweaked/shortened per comment 92.) ::: layout/reftests/w3c-css/submitted/contain/contain-size-block-001.html:32 (Diff revision 3) > + .floatLBasic { > + float: left; > + width: auto; ...and let's remove "width:auto" here, because it's the default value so it's unnecessary/redundant to explicitly specify. ::: layout/reftests/w3c-css/submitted/contain/contain-size-inline-block-001.html:5 (Diff revision 3) > +<!DOCTYPE HTML> > +<html> > +<head> > + <meta charset="utf-8"> > + <title>CSS Test: 'contain: size' on inline- elements should zero the height and/or width if unspecified, and should ensure baseline alignment consistency with empty (0x0) objects of the same type.</title> "inline-" here should say "inline-block". (and I think you'll also be shortening this for consistency per comment 92, which sounds good.)
Attachment #8988858 - Flags: review?(dholbert) → review+
Comment on attachment 8988859 [details] Bug 1467209 - Implement contain:size for flexContainerFrame. https://reviewboard.mozilla.org/r/253984/#review261446 r- since this one will probably merit one final skim after the rebase & test tweaks. ::: layout/generic/nsFlexContainerFrame.cpp:4544 (Diff revision 3) > - if (lines.getFirst()->IsEmpty() && > - !lines.getFirst()->getNext()) { > + if ((lines.getFirst()->IsEmpty() && !lines.getFirst()->getNext()) || > + aReflowInput.mStyleDisplay->IsContainSize()) { This needs 2 more spaces of indentation. ("aReflowInput" should be aligned just inside the open-paren of the "if" condition; i.e. the "a" should be directly under the second open-paren.) (Also -- just a heads-up -- this part of the patch doesn't apply cleanly & needs a rebase to current mozilla-central. This should be fixable with trivial copypaste -- the change is just a new arg in the "GenerateFlexLinex" call above this, introduced by a patch from Mihir.) ::: layout/generic/nsFlexContainerFrame.cpp:4897 (Diff revision 3) > // children (or if our children are huge enough that they have nscoord_MIN > // as their baseline... in which case, we'll use the wrong baseline, but no > // big deal) > + // this should also happen if we want to pretend we have no > + // children (e.g. contain:size) > NS_WARNING_ASSERTION( Please revert this chunk (i.e. remove these two lines) -- these are from back when this patch was making its baseline changes here, but we're not doing that anymore. ::: layout/reftests/w3c-css/submitted/contain/contain-size-flex-001-ref.html:19 (Diff revision 3) > + .floatLBasic-ref { > + float: left; > + width: 0; > + } > + .floatLWidth-ref { > + float: left; > + width: 50px; > + } > + .iFlexBasic-ref { > + display: inline-flex; > + width: 0; As in the block/button tests, let's get rid of "width: 0" and "width:auto" lines in the test files here, since they don't have any effect in these cases. (reference case has 2 "width:0", testcase has 2 corresponding "width:auto") ::: layout/reftests/w3c-css/submitted/contain/contain-size-flex-001.html:4 (Diff revision 3) > + <meta charset="utf-8"> > + <title>CSS Test: 'contain: size' on flex elements should zero the height and/or width if unspecified, and should ensure baseline alignment consistency with empty (0x0) objects of the same type.</title> > + <link rel="author" title="Morgan Rae Reschenberg" href="mailto:mreschenberg@berkeley.edu"> > + <link rel="match" href="contain-size-flex-001-ref.html"> This needs a <link rel="help"> tag. ::: layout/reftests/w3c-css/submitted/contain/contain-size-flex-001.html:9 (Diff revision 3) > + body { > + /* Note: Because the text still gets painted (albiet, transparently), this is necessary > + to ensure we don't add a scroll bar and fail the reftest for that reason */ > + overflow: hidden; This feels kinda hacky for a shared test. This might be a reason not to use the "bunch of text as the contents" paradigm for these tests... :-/ I worry that this might trigger failures in Chrome/Edge as well, if they run these reftests in a smaller viewport than we do (e.g. 400x400, which is smaller than what we use, but which as I recall is the viewport that all of the "painted" content is supposed to fit into to avoid scrollbars... For now, though, I think we should avoid relying on this "overflow:hidden" ourselves, and shorten the text as-needed so that we can pass the test without it. (We might also want to convert the text into HTML comments and use something smaller like a fixed-size div with a single text character as the "contained contents"... but that part could potentially be a followup bug and doesn't need to block this from landing, in my opinion.) ::: layout/reftests/w3c-css/submitted/contain/contain-size-flex-001.html:75 (Diff revision 3) > + aa<div class="iFlexBasic">CSS Test: A size-contained inline-flex element with auto width and no specified height should render at 0px by 0px regardless of content, and it should ensure baseline alignment behaviour matches that of an empty object of the same type.</div>bb > + <br> > + > + aa<div class="iFlexWidth">CSS Test: A size-contained inline-flex element with specified width and no specified height should render at given width and 0 height regardless of content, and it should ensure baseline alignment behaviour matches that of an empty object of the same type.</div>bb On my machine, this last element is at y-position of 437 pixels[1]. So this testcase / reference case is a bit too tall for the w3c reftest guidelines right now. Perhaps it'd be worth splitting the "inline-flex" piece into a separate testcase (matching the division that you've got for block vs. inline-block)? [1] (I found its offset by right-clicking it and doing Inspect Element, and then clicking "Console" devtools tab, and typing "$0.offsetTop". ($0 = most recently selected element in the devtools inspector tab.)
Attachment #8988859 - Flags: review?(dholbert) → review-
Comment on attachment 8988857 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/253980/#review261956 ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:99 (Diff revision 5) > + <!--CSS Test: A size-contained button with vertical text should perform baseline alignment as if the container were empty.--> > + s<button class="orthog"><div class="innerContents">inner</div></button>endtext > + <br> > + > + <!--CSS Test: A size-contained button with inner text should layout the text in the same manner as a container of the same type with identical contents.--> > + <button class="height width" style="color:black;">inside</button> Let's remove the style="color:black" attribute here. It's unnecessary[1] and it causes the test to fail[2] right now. I'm pretty sure the test should still render the text & will pass more reliably without it. [1] unnecessary because you don't need to override "color:transparent" in this case, because that's set on innerContents now, which is not a class that's applied in this part of the test. [2] color:black causes the test to fail because the reference case doesn't have a color specified, so it just uses the default button-text color, which can vary depending on platform (on my platform it's a dark grey color)
Comment on attachment 8988858 [details] Bug 1467209 - Implement contain:size for blockFrame. https://reviewboard.mozilla.org/r/253982/#review261964 ::: layout/reftests/w3c-css/submitted/contain/contain-size-block-001.html:72 (Diff revisions 3 - 5) > - outside before<div>CSS Test: A size-contained block element should perform baseline alignment as if the container were empty.</div>outside after > + <!--CSS Test: A size-contained block element should perform baseline alignment as if the container were empty.--> > + outside before<div class="contain"><div class="innerContents">inner</div></div>outside after This chunk says it's testing baseline alignment, but it actually isn't, because the text (correctly) ends up on a different line than the contained block here. (This happens regardless of contain:size.) To test baseline alignment here (probably a good idea), you can throw this whole line into a flex container with align-items:baseline. Just add this style rule to the end of your <style> block: .flexBaselineChecker { display: flex; align-items: baseline; } ...and then wrap this whole line (from outside before up to outside after) in a <div class="flexBaselineChecker">, in both the testcase and the reference case. (The test still passes for me, after I make these edits -- so it looks like we are indeed doing baseline alignment correct. Hooray!)
Comment on attachment 8988857 [details] Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. https://reviewboard.mozilla.org/r/253980/#review262040 Last nit here, I think: ::: layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html:58 (Diff revision 6) > + <!--CSS Test: A size-contained floated button with auto width and no specified height should render at 0px by 0px regardless of content.--> > + <button class="floatLBasic"><div class="innerContents">inner</div></button> > + <br> > + > + <!--CSS Test: A size-contained floated button with specified width and no specified height should render at given width and 0 height regardless of content.--> > + <button class="floatLWidth"><div class="innerContents">inner</div></button> > + <br> > + > + <!--CSS Test: A size-contained inline-flex button with auto width and no specified height should render at 0px by 0px regardless of content.--> > + <button class="iFlexBasic"><div class="innerContents">inner</div></button> > + <br> This is kind of confusing phrasing, in the first and last quoted comment here: "...auto width and no specified height..." Here, "unspecified" and "auto" are really the same thing, for width and height, since auto is the initial value of these properties. But it sounds like this comment is implying that auto != unspecified. Really, you probably want to say "with no specified size" like you do for the very first element here. (This feedback applies to the main test in each of the 3 patches here -- they all have at least one instance of this "auto width and no specified height" language.)
Comment on attachment 8988859 [details] Bug 1467209 - Implement contain:size for flexContainerFrame. https://reviewboard.mozilla.org/r/253984/#review262042 r=me (with the "auto width and no specified height" code-comment adjusted in contain-size-flex-001.html, per comment 108)
Attachment #8988859 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 342a815fc96ac6917a01de4a63579579b392fe7d -d 44861ed17925: rebasing 471559:342a815fc96a "Bug 1467209 - Implement contain:size for HTMLButtonControlFrame. r=dholbert" merging layout/reftests/w3c-css/submitted/contain/reftest.list warning: conflicts while merging layout/reftests/w3c-css/submitted/contain/reftest.list! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Looks like this needs a rebase for reftest.list (looks like it clashes with your reftest.list change from bug 1470329)
Flags: needinfo?(mreschenberg)
Hmmm. Importing on top of a clean pull from central seemed to be okay on my end (no merge conflicts). I have the multicol test in reftest.list now, so I re-pushed in case that's all it needed.
Flags: needinfo?(mreschenberg)
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4bd2d2037953 Implement contain:size for HTMLButtonControlFrame. r=dholbert https://hg.mozilla.org/integration/autoland/rev/b5c2aea05d7a Implement contain:size for blockFrame. r=dholbert https://hg.mozilla.org/integration/autoland/rev/cb898a5e2d09 Implement contain:size for flexContainerFrame. r=dholbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4bd2d2037953 https://hg.mozilla.org/mozilla-central/rev/b5c2aea05d7a https://hg.mozilla.org/mozilla-central/rev/cb898a5e2d09
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: