1338108 - Inflating a zero size with 100% results in nscoord_MAX in intrinsic sizing

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[romaric.pascal]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

I'm trying to set up a grid of squares using the new CSS grid layout, like in this example: http://codepen.io/rhumaric/pen/xgyrEJ. Contents of the grids are square through `width: 100%; padding-bottom: 100%;`. They're sized according to the width of the screen by dividing the grids in N sections of `1fr`. 


Actual results:

Setting a `grid-templates-row` of `auto` makes the height of the squares null, preventing content from being seen. 


Expected results:

Each item of the grid should have a height respecting the `padding-bottom` instruction, allowing for its content to be visible.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Reporter's testcase

Comment 2

[Mats Palmgren (inactive)]

Attachment: Reduced testcase

Comment 3

[Mats Palmgren (inactive)]

This is essentially a bug in Chrome which still resolves vertical percentages
against the width (as the reduced testcase shows):
https://bugs.chromium.org/p/chromium/issues/detail?id=229568

Comment 4

[Mats Palmgren (inactive)]

Still, I'm not completely happy with how we calculate the intrinsic size
for the grid item here - it ends up being nscoord_MAX due to:
http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/layout/base/nsLayoutUtils.h#1455

This is not a Grid bug per se though, it happens in all intrinsic sizing that
involves a percentage sum > 100%.  I think zero would be a more reasonable result
when aCurrent==0 there, otherwise there is a discontinuity when going from
99.99% to 100%.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Reduced testcase with 99.99% instead of 100%

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix

I think this would lead to better results in general, but it's a bit scary change...  WDYT?

Comment 7

[Mats Palmgren (inactive)]

Sorry, I should have said "a percentage sum >= 100%" in comment 4 (not > 100%).

Comment 8

[Daniel Holbert [:dholbert]]

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

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

::: layout/base/nsLayoutUtils.h
@@ +1450,5 @@
>     * is greater than or equal to 1.0 the value nscoord_MAX is returned.
>     */
>    static nscoord AddPercents(nscoord aCurrent, float aPercent)
>    {
> +    if (aPercent > 0.0f && aCurrent > 0) {

This seems like a reasonable change to me.  Probably worth mentioning this edge-case behavior in the documentation above this function, though.

Maybe just tweak this sentence in the existing documentation...
   * [...]  If 'aPercent' is less than
   * or equal to zero the original 'aCurrent' value is returned.

...and just replace like so:
  s/'aPercent' is/'aPercent' or 'aCurrent' are/

Comment 9

[Robert Utasi [:hunboy]]

Attachment: grid testcase infinity scrollbar at position relative element with padding-top: 99.99999%

Comment 10

[Mats Palmgren (inactive)]

Attachment: fix

(updated the comment)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a90ff46b78a4931953ef9818e765a23db2879ac1

Comment 11

[Daniel Holbert [:dholbert]]

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

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

r=me

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4df8d4e384ef
The result of adding any percentage factor to a size that is zero should also be zero.  r=dholbert

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for unexpectedly passing 367185-1.xhtml:

https://hg.mozilla.org/integration/mozilla-inbound/rev/76de74bcbf1a38838b8c619a2832b50ec368ddf9

Push with unexpected passes: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4df8d4e384ef90e16acd6cfd30a01168b905138f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=89257755&repo=mozilla-inbound

> [task 2017-04-06T18:52:23.376308Z] 18:52:23     INFO - REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/reftest/tests/layout/xul/crashtests/367185-1.xhtml | assertion count 0 is less than expected 24 assertions

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b13d3c03b5ad
The result of adding any percentage factor to a size that is zero should also be zero.  r=dholbert

Comment 15

[Mats Palmgren (inactive)]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #13)
> Backed out for unexpectedly passing 367185-1.xhtml

Right, it makes sense that it fixed the assertions in that test (bug 1220345):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=843613d5b9bbfafeafc7caafefa3831d0d6beec7

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b13d3c03b5ad