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

RESOLVED FIXED in Firefox 51

Status

()

Core
Layout
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Manuel Rego Casasnovas, Assigned: mats)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {testcase})

Trunk
mozilla51
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(5 attachments)

(Reporter)

Description

2 years ago
Created attachment 8761511 [details]
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8761512 [details]
Current output
(Reporter)

Comment 2

2 years ago
Created attachment 8761513 [details]
Expected output
(Reporter)

Updated

2 years ago
Blocks: 616605
Component: Untriaged → Layout
Product: Firefox → Core
(Assignee)

Comment 3

2 years ago
Thanks Rego.
Assignee: nobody → mats
Blocks: 1266268
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: testcase
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 5

2 years ago
Created attachment 8765670 [details] [diff] [review]
fix
Attachment #8765670 - Flags: review?(dholbert)
(Assignee)

Comment 6

2 years ago
Created attachment 8765671 [details] [diff] [review]
tests
[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. :)
(Assignee)

Comment 10

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

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

Comment 12

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

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba8f999c0a3f
https://hg.mozilla.org/mozilla-central/rev/6e7cf357d19a
https://hg.mozilla.org/mozilla-central/rev/b8f1c3166fa2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Updated

a year ago
Depends on: 1298550
(Assignee)

Updated

a year ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.