Stop back-computing percentages for intrinsic sizing

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks 2 bugs)

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 wontfix, firefox61 fixed)

Details

Attachments

(7 attachments, 2 obsolete attachments)

13.34 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
11.49 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
6.08 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
373 bytes, text/html
Details
4.94 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
4.31 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
23.53 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
Assignee

Description

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

Updated

a year ago
Assignee: nobody → mats
[ Triage 2017/02/20: P3 ]
Priority: -- → P3
Assignee

Comment 2

a year ago
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.
Attachment #8954386 - Flags: review?(dholbert)
Assignee

Comment 6

a year ago
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)
Assignee

Comment 7

a year ago
(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?
Attachment #8954386 - Flags: review?(dholbert) → review+
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
Attachment #8954387 - Flags: review?(dholbert) → review+
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.
Attachment #8954389 - Flags: review?(dholbert) → review+
(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.)
Attachment #8954390 - Flags: review?(dholbert) → review-
Assignee

Comment 13

a year ago
(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)
Flags: needinfo?(dholbert)
Assignee

Comment 14

a year ago
(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.
Assignee

Comment 15

a year ago
Posted file Testcase #1
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!
Flags: needinfo?(dholbert)
Assignee

Comment 17

a year ago
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.
Assignee

Comment 20

a year ago
Same as before, but now using ResolveToLength from part 5 instead of
the local AddCoord function.
Attachment #8966232 - Flags: review?(dholbert)
Assignee

Comment 21

a year ago
> 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
Attachment #8966229 - Flags: review?(dholbert) → review+
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.)
Attachment #8966230 - Flags: review?(dholbert) → review+
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."
Attachment #8966232 - Flags: review?(dholbert) → review+
Assignee

Updated

a year ago
Blocks: 1452797

Comment 24

a year ago
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.
Assignee

Comment 25

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

Updated

a year ago
Duplicate of this bug: 1450700
Assignee

Updated

a year ago
Blocks: 1445190
Assignee

Updated

a year ago
Duplicate of this bug: 1458732
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.