Closed Bug 1279182 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: rego, Assigned: mats)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(5 files)

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.
Attached image Current output
Attached image Expected output
Blocks: css-grid
Component: Untriaged → Layout
Product: Firefox → Core
Thanks Rego.
Assignee: nobody → mats
Blocks: 1266268
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: testcase
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.
Attached patch fixSplinter Review
Attachment #8765670 - Flags: review?(dholbert)
Attached patch testsSplinter Review
[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.]
Blocks: 1281320
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.
Attachment #8765670 - Flags: review?(dholbert) → review+
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. :)
(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).
Pushed by mpalmgren@mozilla.com:
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.
Pushed by mpalmgren@mozilla.com:
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
Depends on: 1298550
Flags: in-testsuite+
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.)
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.
Flags: needinfo?(mats)
s/should/should use/
You need to log in before you can comment on or make changes to this bug.