Brandon Frohs Reporter
	Description • 7 years ago

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161209095719

Steps to reproduce:

Set a percentage 'max-width' that was smaller than the column width on a grid item.


Actual results:

The grid item with the most text was sized vertically as if no 'max-width' was specified. That is, if you were to remove 'max-width' completely, the box would be the same size, but the text would fit inside of it (since it didn't wrap before the end of the box).

Text overflows the box and overlaps grid items below it.

Lengths such as px or em work properly.


Expected results:

The grid item should fit all of the text within it, and push the next row down accordingly.

	Mats Palmgren (inactive) Assignee
	Comment 2 • 7 years ago

[Tracking Requested - why for this release]: Bug fix for CSS Grid release.  I expect to have a patch for review later today or tomorrow.

	Julien Cristau [:jcristau]
	Comment 3 • 7 years ago

Thanks Mats.  Tracking for 52.

	Mats Palmgren (inactive) Assignee
	Comment 4 • 7 years ago

Using "cb" in these variable names is a slight misnomer, and I'll add a true CB
size in the next patch...

	Mats Palmgren (inactive) Assignee
	Comment 5 • 7 years ago

So here's the bug:
-  ReflowInput childRI(pc, *rs, aChild, aAvailableSize, nullptr, riFlags);

passing nullptr for the CB when reflowing a grid item is pretty much always
an error since it triggers ReflowInput::ComputeContainingBlockRectangle
which will use the grid container as the CB, which is wrong.

(We actually had a couple of tests that covered this, but unfortunately
they use a single track so the grid area was the same as the container's
content box so we got the expected result anyway.)

	Mats Palmgren (inactive) Assignee
	Comment 6 • 7 years ago

Just adding a few XXX comments about the MeasuringReflow call associated
with measuring baselines.  I'll address that separately in bug 1330866.

	Mats Palmgren (inactive) Assignee
	Comment 7 • 7 years ago

Green on Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eb8613c015f356bb99186f42a9a9370f3bc233a

	Daniel Holbert [:dholbert]
	Comment 8 • 7 years ago

Comment on attachment 8826631 [details] [diff] [review]
part 2 - We must always pass along a CB size when reflowing grid items, also in MeasuringReflow.

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +3677,5 @@
>  MeasuringReflow(nsIFrame*           aChild,
>                  const ReflowInput*  aReflowInput,
>                  nsRenderingContext* aRC,
>                  const LogicalSize&  aAvailableSize,
> +                const LogicalSize*  aCBSize = nullptr,

The commit message says "We must always pass along a CB size", but that doesn't match what we actually do here -- we have this variable *default* to null, and there's one callsite (in InitializeItemBaselines) that does use that default value.  So, for now, the code doesn't match the commit message w.r.t. "always".

Does the other callsite need some fixup?  And if the intent is really that we should be passing something non-null (per comment 5), then perhaps we shouldn't use null as a default value for this argument here?  (force callers to explicitly request null if they *really* want that behavior, but don't make it the default)

	Daniel Holbert [:dholbert]
	Comment 9 • 7 years ago

Oh, maybe that's what part 3 is about; sorry if so. Taking a look now.

	Daniel Holbert [:dholbert]
	Comment 10 • 7 years ago

Comment on attachment 8826631 [details] [diff] [review]
part 2 - We must always pass along a CB size when reflowing grid items, also in MeasuringReflow.

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

Yup, sorry, I shoulda looked at Part 3 first. :)

So r=me, though could you remove the default value of aCBSize?  Default values are kind of footgunny, and we shouldn't have the default behavior be something undesirable.  Seems like here, any caller (including future callers) that want nullptr should have to *explicitly* explain/hand-wring about their nullptr usage in a code comment. (as you do for the one other caller in part 3) 

You might also want to just fold part 3 into this part, since if there's no default argument, you'll have to touch that baseline code anyway to add the "nullptr" to that function call.  So might as well add the explanatory XXX comments at the same time.

	Daniel Holbert [:dholbert]
	Comment 11 • 7 years ago

Comment on attachment 8826633 [details] [diff] [review]
part 3 - Add a few comments about possible errors in baseline calculations due to percentage padding.

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

This might want to just be folded into part 2, if you're already touching this code there anyway due to removing the default aCBSize=null assignment, as suggested above.

::: layout/generic/nsGridContainerFrame.cpp
@@ +4207,5 @@
> +      // XXX (see ::ContentContribution and how it deals with percentages)
> +      //
> +      // XXX What if the true baseline after line-breaking differs from this
> +      // XXX hypothetical baseline based on an infinite inline size?
> +      // XXX Maybe we should just call ::ContentContribution here instead?

Perhaps this should be clearer about how it actually ties in to the adjacent code?  e.g. maybe start this comment out with:
 "Note that we pass MeasuringReflow() a null aCBSize below, which is problematic per bug 1330380 comment 5."
or something like that. *shrug*

	Mats Palmgren (inactive) Assignee
	Comment 12 • 7 years ago

OK, I merged part 2 and 3, and made aCBSize mandatory and just pass 0,0
for the baseline measurement for now.

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

	Pulsebot
	Comment 13 • 7 years ago

Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6f5417cf3f2
part 1 - Rename a couple of variables.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/12789bb38582
part 2 - We must always pass along a CB size when reflowing grid items, also in MeasuringReflow.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/442504607f46
part 3 - Add more reftests using percentages in various properties.

	Mats Palmgren (inactive) Assignee
	Comment 14 • 7 years ago

Comment on attachment 8826734 [details] [diff] [review]
part 2 - We must always pass along a CB size when reflowing grid items, also in MeasuringReflow.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: broken CSS Grid layout in some cases involving percentages
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: only touches nsGridContainerFrame.cpp thus restricted to display:grid/inline-grid layout, fairly minor change
[String changes made/needed]: none

Request is for all three parts.  For CSS Grid release in v52.

	Phil Ringnalda (:philor)
	Comment 15 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a6f5417cf3f2
https://hg.mozilla.org/mozilla-central/rev/12789bb38582
https://hg.mozilla.org/mozilla-central/rev/442504607f46

	Pulsebot
	Comment 16 • 7 years ago

Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14f88bc4d6b3
Follow-up: remove debug console.log script from a few CSS Grid reftests.  r=me

	Phil Ringnalda (:philor)
	Comment 17 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/14f88bc4d6b3

	Julien Cristau [:jcristau]
	Comment 18 • 7 years ago

Comment on attachment 8826734 [details] [diff] [review]
part 2 - We must always pass along a CB size when reflowing grid items, also in MeasuringReflow.

css grid fix for aurora52

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/77badc363a32
https://hg.mozilla.org/releases/mozilla-aurora/rev/98f09679b016
https://hg.mozilla.org/releases/mozilla-aurora/rev/81b03e9bbcc1
https://hg.mozilla.org/releases/mozilla-aurora/rev/b68fa747093c