https://github.com/w3c/csswg-drafts/issues/347 "RESOLVED: percentage margins & paddings compute to 0 in intrinsic sizing and then resolve during layout"

[ Triage 2017/02/20: P3 ]

This basically reverts bug 1302541. I chose to treat calc()-expressions with percentages as zero for now, pending resolution of https://github.com/w3c/csswg-drafts/issues/2297 since that's what we do for padding/margin.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29081130ed0733ba004b14f6b256b2818ea86807 (box-decoration-break-with-outset-box-shadow-1.html fails on Win7, but I suspect it's unrelated since it contains no percentage lengths)

(In reply to Mats Palmgren (:mats) from comment #6) > Created attachment 8954392 [details] [diff] [review] > part 5 - Update tests and enable some previously temporarily disabled Grid > reftests from bug 1427608. Regarding the commented out tests 363858-5a.html/363858-6a.html -- I'll file a follow-up bug to rewrite those if possible. The rendering of these tests is now somewhat compatible with Chrome, so I think they are fine, but it's hard to find a reference that would render exactly the same... Regarding the table rendering change in 403519-2-ref.html -- the test now renders exactly as in Chrome.

Comment on attachment 8954386 [details] [diff] [review] part 1 - [css-grid] Stop back-computing percentage grid gaps when the percentage basis is indefinite. Review of attachment 8954386 [details] [diff] [review]: ----------------------------------------------------------------- r=me, one clarification nit: ::: layout/generic/nsGridContainerFrame.cpp @@ +927,5 @@ > return 1; > } > + // Calculate the max number of tracks that fits without overflow. > + div_t q = div(spaceToFill, repeatTrackSize + gridGap); > + uint32_t numRepeatTracks = q.quot + 1; Why the "+1" here? (I think maybe because we already accounted for one repeat() track, in "sum", before we did any division?) Could you add/extend a comment to make that clearer?

Comment on attachment 8954387 [details] [diff] [review] part 2 - Stop back-computing percentage padding/margin when the percentage basis is indefinite. Review of attachment 8954387 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 8954389 [details] [diff] [review] part 3 - Remove IntrinsicISizeOffsetData::hPctPadding/hPctMargin members since they are now unused. Review of attachment 8954389 [details] [diff] [review]: ----------------------------------------------------------------- r=me, one nit: ::: layout/generic/nsFrame.cpp @@ +5480,1 @@ > return; This clause (AddCoord's case for "eStyleUnit_Percent") is kind of confusing now, since it doesn't do anything. Could you leave behind a code-comment, explaining why we're not doing anything with aStyle.GetPercentValue() here? (when we might be expected to do something, given that we're inside of an Add...() function) Alternately/also: it might be worth putting a code-comment above this function to say that it treats any percent value (or calc-component) in its "aStyle" param as 0.

(In reply to Daniel Holbert [:dholbert] from comment #10) > ::: layout/generic/nsFrame.cpp > @@ +5480,1 @@ > > return; > > This clause (AddCoord's case for "eStyleUnit_Percent") is kind of confusing > now, since it doesn't do anything. > > Could you leave behind a code-comment Oh, never mind - now I see that the next patch adds back some code here, so this doensn't end up empty after all. So, disregard this nit, and consider part 3 "r+". (also: grr @ splinter's decision to print 0 lines of context in my comment there. :) )

Comment on attachment 8954390 [details] [diff] [review] part 4 - Propagate a percentage basis to nsIFrame::IntrinsicISizeOffsets for resolving padding/margin. Review of attachment 8954390 [details] [diff] [review]: ----------------------------------------------------------------- This is r- for the moment, mainly since I'm unsure whether the "val = -val" stuff is correct... (see comment below) ::: layout/base/nsLayoutUtils.cpp @@ +5726,5 @@ > PhysicalAxis ourInlineAxis = > aFrame->GetWritingMode().PhysicalAxis(eLogicalAxisInline); > nsIFrame::IntrinsicISizeOffsetData offsets = > ourInlineAxis == aAxis ? aFrame->IntrinsicISizeOffsets() > + : aFrame->IntrinsicBSizeOffsets(); // XXX propagate percent basis from grid layout (This XXX comment maybe wants to be on its own line, so it doesn't wrap off the end of people's editors? Also, does it need a followup bug or anything like that?) ::: layout/generic/nsFrame.cpp @@ +5479,5 @@ > "unexpected negative value"); > + if (aPercentageBasis == NS_UNCONSTRAINEDSIZE) { > + return; > + } > + *aCoord += aStyle.GetPercentValue() * aPercentageBasis; You need to wrap the multiplication in NSToCoordRound() here, to get reasonable rounding behavior. @@ +5486,5 @@ > case eStyleUnit_Calc: { > + const nsStyleCoord::Calc* calc = aStyle.GetCalcValue(); > + if (calc->mHasPercent && aPercentageBasis == NS_UNCONSTRAINEDSIZE) { > + return; > + } In this early return, we're dropping calc->mLength on the floor, simply because there's also a percent value alongside it that we can't resolve. I think that's what you're intending for now (per comment 2)... but even so, it's very counterintuitive. Could you at least add a code-comment here, to clarify that that *is* indeed what's happening (and that this is known-perhaps-suboptimal & may change, pending CSSWG action)? @@ +5487,5 @@ > + const nsStyleCoord::Calc* calc = aStyle.GetCalcValue(); > + if (calc->mHasPercent && aPercentageBasis == NS_UNCONSTRAINEDSIZE) { > + return; > + } > + nscoord val = calc->mLength + calc->mPercent * aPercentageBasis; As above, we need NSToCoordRound() around the multiplication here. @@ +5489,5 @@ > + return; > + } > + nscoord val = calc->mLength + calc->mPercent * aPercentageBasis; > + if (aClampNegativeToZero && val < 0) { > + val = -val; Wait, why are we inverting val's sign here? Shouldn't we just be doing val = std::max(0,val) or something like that? (If we have aStyle = "calc(-10px)", it looks like previously we'd clamp that to 0, whereas this patch is making us interpret that as if it were calc(10px)... Is that a mistake? Or if it really makes sense, it could probably use a brief explanatory comment.)

(In reply to Daniel Holbert [:dholbert] from comment #8) > Why the "+1" here? (I think maybe because we already accounted for one > repeat() track, in "sum", before we did any division?) Correct. I'll add a comment, like so: - // Note that the repeat() track size is included in |sum| in this loop. + // Note that one repeat() track size is included in |sum| in this loop. ... + // The +1 here is for the one repeat track we already accounted for above. uint32_t numRepeatTracks = q.quot + 1; Does that make it clearer? (let me know if you have a better suggestion)

(In reply to Daniel Holbert [:dholbert] from comment #12) > (This XXX comment maybe wants to be on its own line, so it doesn't wrap off > the end of people's editors? Sure. > Also, does it need a followup bug or anything like that?) Yeah, my feeling is that we must already have a bug here if propagating a basis is necessary, at least after we changed block-axis padding/margin to resolve against the inline-axis. But I haven't seen any obvious errors in reftests so far, so maybe I'm wrong... I'll try to investigate this more before landing and file a bug if necessary... > > + *aCoord += aStyle.GetPercentValue() * aPercentageBasis; > > You need to wrap the multiplication in NSToCoordRound() here, to get > reasonable rounding behavior. Yes, that sounds reasonable. > In this early return, we're dropping calc->mLength on the floor, simply > because there's also a percent value alongside it that we can't resolve. > > I think that's what you're intending for now (per comment 2)... but even > so, it's very counterintuitive. Could you at least add a code-comment here, > to clarify that that *is* indeed what's happening (and that this is > known-perhaps-suboptimal & may change, pending CSSWG action)? Yeah, it's intentional. I'll add a comment referencing the github issue I filed about grid gaps. (I'm pretty sure they can't change this for web-compat reasons. fantasai probably just didn't realize this is how padding/margin work already.) > > + nscoord val = calc->mLength + calc->mPercent * aPercentageBasis; > > As above, we need NSToCoordRound() around the multiplication here. OK > > + nscoord val = calc->mLength + calc->mPercent * aPercentageBasis; > > + if (aClampNegativeToZero && val < 0) { > > + val = -val; > > Wait, why are we inverting val's sign here? Shouldn't we just be doing val > = std::max(0,val) or something like that? Oops, that's definitely wrong. I have no idea what I was thinking there, LOL. I'll just do an early return there I think.

Here's a testcase that hits that "XXX propagate percent basis" case. It's definitely already wrong in Nightly, but it's still not correct with these patches -- the row size ends up being the height of the "x" as if the padding-bottom was zero, but layout gets it right so the item overflows with 100px. (I suspect Chrome has the correct rendering) It's the "min-size-contribution" case, where min-height is definite and height is auto: https://drafts.csswg.org/css-grid/#min-size-contribution It looks like an easy fix so I'll fix this in part 4 and write some tests for this...

(In reply to Mats Palmgren (:mats) from comment #13) > Does that make it clearer? (let me know if you have a better suggestion) Yup, that is great. Thanks!

After testing how Chrome resolves calc()-percentages a bit more, it turns out that they resolve padding and margin *differently*. Sigh. https://github.com/w3c/csswg-drafts/issues/2297#issuecomment-369756141 Anyway, since we're coming up to a soft-freeze, and there's no rush to fix this, I'll wait to after we branch to minimize the risks of this change. Hopefully, we'll have a clear answer from the CSSWG by then.

Same as before, but now using ResolveToLength from part 5 instead of the local AddCoord function.

> Same as before, but now using ResolveToLength from part 5 instead of > the local AddCoord function. IOW, you only need to re-review layout/generic/nsFrame.cpp

Comment on attachment 8966230 [details] [diff] [review] part 5 - Create nsLayoutUtils::ResolveToLength for resolving CSS <length-percentage> (idempotent patch) Review of attachment 8966230 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.h @@ +3064,5 @@ > + } > + MOZ_ASSERT(!clampNegativeResultToZero || aCoord.GetPercentValue() >= 0, > + "This value should have been rejected by the style system"); > + return NSToCoordFloorClamped(aPercentageBasis * > + aCoord.GetPercentValue()); Nit: hypothetically if we receive a negative value for aPercentageBasis, then this function will gladly return something negative with no warnings, despite having clampNegativeResultToZero == true! This is a bit surprising. Probably not worth adding special-case code to handle this, but perhaps you should add an up-front NS_WARNING_ASSERTION to document & verify the assumption that aPercentageBasis is non-negative? (Probably better not to MOZ_ASSERT that one, because nscoord overflow is possible with silly content, and can produce huge --> negative-sized boxes, which we'd prefer not to cause a needless abort when debugging unrelated + more-concerning issues with fuzzer testcases.)

Comment on attachment 8966232 [details] [diff] [review] part 6 - Propagate a percentage basis to nsIFrame::IntrinsicISizeOffsets for resolving padding/margin Review of attachment 8966232 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: layout/base/nsLayoutUtils.h @@ +1452,5 @@ > * calculates the result as if the 'min-' computed value is zero. > * Otherwise, return NS_UNCONSTRAINEDSIZE. > * > + * @param aPercentageBasis the percentage basis (in aFrame's WM). > + * Pass NS_UNCONSTRAINEDSIZE if the basis is indefinite in either/both axes. This probably needs a slight rewording. The directive to "Pass NS_UNCONSTRAINEDSIZE" doesn't quite make sense here, since this param has type LogicalSize, so you can't literally "pass NS_UNCONSTRAINEDSIZE" here. Maybe something like: "If the basis is indefinite in a given axis, pass a size with NS_UNCONSTRAINEDSIZE in that component."

Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/48d0890ec465 part 1 - [css-grid] Stop back-computing percentage grid gaps when the percentage basis is indefinite. Treat them as zero sized instead. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/18a97ac055fe part 2 - Stop back-computing percentage padding/margin when the percentage basis is indefinite. Treat them as zero sized instead. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/03cf11e34045 part 3 - Remove IntrinsicISizeOffsetData::hPctPadding/hPctMargin members since they are now unused. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/96f308028ff1 part 4 - Factor out constants like NS_UNCONSTRAINEDSIZE so they can be used in headers without needing nsIFrame.h (idempotent patch). r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/dcc12436f58f part 5 - Create nsLayoutUtils::ResolveToLength for resolving CSS <length-percentage> (idempotent patch). r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e12045f233 part 6 - Propagate a percentage basis to nsIFrame::IntrinsicISizeOffsets for resolving padding/margin. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/a3f15a7f2914 part 7 - Update tests and enable some previously temporarily disabled Grid reftests from bug 1427608.

A warning for negative aPercentageBasis seems reasonable, I added: NS_WARNING_ASSERTION(aPercentageBasis >= nscoord(0), "nscoord overflow?");

https://hg.mozilla.org/mozilla-central/rev/48d0890ec465 https://hg.mozilla.org/mozilla-central/rev/18a97ac055fe https://hg.mozilla.org/mozilla-central/rev/03cf11e34045 https://hg.mozilla.org/mozilla-central/rev/96f308028ff1 https://hg.mozilla.org/mozilla-central/rev/dcc12436f58f https://hg.mozilla.org/mozilla-central/rev/e2e12045f233 https://hg.mozilla.org/mozilla-central/rev/a3f15a7f2914