1279182 - [css-grid] Percentage gutters are wrong calculated on grid containers with indefinite sizes

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Manuel Rego Casasnovas]

Attachment: percentage-gaps-indefinite-sizes.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.24 Safari/537.36

Steps to reproduce:

In a floated grid container with auto height use a percentage value for grid-gap.

Open the attached example to see the issue in the 3rd grid container.


Actual results:

The percentages are resolved wrong, and you get a huge grid.


Expected results:

On the inline axis, the percentage gap for the columns should be resolved against the intrinsic size of the grid container (200px in the attached example). That is a gap of 20px.

On the block axis, the percentage gap for the rows should resolve as 0px as the height is indefinite.

Comment 1

[Manuel Rego Casasnovas]

Attachment: Current output

Comment 2

[Manuel Rego Casasnovas]

Attachment: Expected output

Comment 3

[Mats Palmgren (inactive)]

Thanks Rego.

Comment 4

[Mats Palmgren (inactive)]

We use nsRuleNode::ComputeCoordPercentCalc to resolve these values.
I thought it dealt with aPercentageBasis == NS_UNCONSTRAINEDSIZE for
percent styles and returned zero in that case, but I was mistaken.

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix

Comment 6

[Mats Palmgren (inactive)]

Attachment: tests

Comment 7

[Daniel Holbert [:dholbert]]

[Looks like bug 1281320 adds some calls to "AbsoluteSize" function that's added in this bug's patch -- adding an explicit bug-dependency, to reflect that.]

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8765670 [details] [diff] [review]
fix

Review of attachment 8765670 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with one one possible change to the logic (modulo your thoughts) and with one documentation clarification.

::: layout/generic/nsGridContainerFrame.cpp
@@ +976,5 @@
>          if (!coord->IsCoordPercentCalcUnit()) {
>            return 1;
>          }
>        }
> +      nscoord trackSize = ::AbsoluteSize(*coord, aSize);

I'm not 100% sure this change is correct.  There's a spec-quote above this saying:
  "treating each track as its max track sizing function if that is
   definite or as its minimum track sizing function otherwise"

Right now, that spec-quote is followed by some checks for "IsCoordPercentCalcUnit()", which serve as our proxies for "is this definite".  But perhaps we should lump in "percent of unresolved size" as indefinite, as well. (rather than treating it as definite and then collapsing it to zero later on, as this patch does right now)

Specifically: perhaps should instead replace both of the nested "if (!coord->IsCoordPercentCalcUnit())" checks above this with the following:
  if (!coord->IsCoordPercentCalcUnit() ||
      IsPercentOfIndefiniteSize(*coord, aSize)) {

...and then we can revert the ::AbsoluteSize call on this line here, since at this point we'll known that |coord| is resolvable.

::: layout/style/nsRuleNode.h
@@ +1004,5 @@
>    // (Values that don't require aPercentageBasis should be handled
>    // inside nsRuleNode rather than through this API.)
> +  // @note the caller is expected to handle percentage of an indefinite size
> +  // and NOT call this method with aPercentageBasis == NS_UNCONSTRAINEDSIZE.
> +  // @note the return value may be negative

This second @note is a little mysterious. (It's not superficially clear what sorts of scenarios you're warning about... My first guesses were:
 - integer overflow due to scaling by huge percent values? [but that's not worth caring much about]
 - The arguments themselves being negative? [but I think the CSS parser should clamp or filter those out]

After a bit more thought, I'm *guessing* you're really talking about calc expressions with subtraction, like so:
  calc(10px - 50%)
which can indeed totally produce negative values depending on the percent basis.

So, consider mentioning an example to make this point clearer -- maybe:
  // @note the return value may be negative, e.g. for "calc(a-b%)"

@@ +1012,5 @@
>    // Compute the value of an nsStyleCoord that is either a coord, a
>    // percent, or a calc expression.
> +  // @note the caller is expected to handle percentage of an indefinite size
> +  // and NOT call this method with aPercentageBasis == NS_UNCONSTRAINEDSIZE.
> +  // @note the return value may be negative

Same here.

Comment 9

[Daniel Holbert [:dholbert]]

Also, I'm not 100% sold on the naming of "AbsoluteSize" function -- it sounds a bit too similar to "absolute value", which means something completely different when applied to potentially-negative quantities. So, looking at calls to this function, I half-expect that AbsoluteSize will give me the absolute value of the size, or something like that.

I don't have any great suggestions for a better name ("ResolveToDefiniteLength" is the best I can do right now), so this isn't really actionable I suppose -- but if you come up with a better name, it might be good to replace this throughout your patches before they land. :)

Comment 10

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> > +      nscoord trackSize = ::AbsoluteSize(*coord, aSize);
> 
> I'm not 100% sure this change is correct.

That's a good point.  I should have pointed out that I'm aware of it.
This is in the CalculateRepeatFillCount method - I will address this
in bug 1280798 instead because there are more issues here for
intrinsic sizing, so this code needs a bit refactoring.

> ::: layout/style/nsRuleNode.h
> So, consider mentioning an example to make this point clearer -- maybe:
>   // @note the return value may be negative, e.g. for "calc(a-b%)"

Fixed.


(In reply to Daniel Holbert [:dholbert] from comment #9)
> Also, I'm not 100% sold on the naming of "AbsoluteSize" function

I renamed it ResolveToDefiniteSize.  I prefer Size rather than Length
here since it clamps negative values to zero, which is appropriate for
sizes (width/height), but not for (CSS) lengths in general (margin).

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8f999c0a3f
[css-grid] Resolve a <percentage> grid-gap of an indefinite CB size to zero.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7cf357d19a
[css-grid] Reftests for <percentage> grid-gap.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8f1c3166fa2
[css-grid] Follow-up: comment out part of the test that failed on Win 8 for now.  r=me DONTBUILD

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ba8f999c0a3f
https://hg.mozilla.org/mozilla-central/rev/6e7cf357d19a
https://hg.mozilla.org/mozilla-central/rev/b8f1c3166fa2

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8765670 [details] [diff] [review]
fix

Review of attachment 8765670 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +108,5 @@
> +AbsoluteSize(const nsStyleCoord& aCoord, nscoord aPercentBasis)
> +{
> +  MOZ_ASSERT(aCoord.IsCoordPercentCalcUnit());
> +  if (::IsPercentOfIndefiniteSize(aCoord, aPercentBasis)) {
> +    return nscoord(0);

Late-breaking review question... when aPercentBasis is unconstrained, it looks to me like this^^ would resolve "calc(100px + 1%)" to 0, which seems wrong.  It seems like perhaps we should be resolving that sort of calc() value to 100px (its pixel component), treating the percentage basis as 0 but still honoring the px value.

Was there a reason we did it this way, though?  (Asking because bug 1265342 is cribbing from this code, and I want to be sure I understand if there's a good reason it's the way it is here, & whether that good reason applies over on bug 1265342 as well.)

(For reference, this code lives here in mozilla-central:
https://searchfox.org/mozilla-central/rev/2ce99e8054b0ff6ed1adf484aeaacacf2fea084c/layout/generic/nsGridContainerFrame.cpp#116-121
... and its name was updated to "ResolveToDefiniteSize()" in the final patch that landed here.)

Comment 15

[Mats Palmgren (inactive)]

Yeah, that function is only meant to be used for track sizing values
which should be treated as 'auto' per:
https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-length-percentage
(bug 1434478 removes other uses)

I've updated bug 1434478 with a generic nsLayoutUtils::ResolveToLength
function that we should for other things: margin, padding, grid-gap etc.
For 'shape-margin' I think you want nsLayoutUtils::ResolveToLength<true>
to clamp negative values.

Comment 16

[Mats Palmgren (inactive)]

s/should/should use/