1330380 - [css-grid] 'max-width' set on grid item causes text to overflow

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Brandon Frohs]

Attachment: testcase.html

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.

Comment 1

[Brandon Frohs]

Attachment: reference case

Comment 2

[Mats Palmgren (inactive)]

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

Comment 3

[Julien Cristau [:jcristau]]

Thanks Mats.  Tracking for 52.

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 1 - Rename a couple of variables.

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

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 2 - We must always pass along a CB size when reflowing grid items, also in MeasuringReflow.

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.)

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 3 - Add a few comments about possible errors in baseline calculations due to percentage padding.

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

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 4 - Add more reftests using percentages in various properties.

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

Comment 8

[Daniel Holbert [:dholbert]]

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)

Comment 9

[Daniel Holbert [:dholbert]]

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

Comment 10

[Daniel Holbert [:dholbert]]

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.

Comment 11

[Daniel Holbert [:dholbert]]

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*

Comment 12

[Mats Palmgren (inactive)]

Attachment: part 2 - We must always pass along a CB size when reflowing grid items, also in MeasuringReflow.

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 14

[Mats Palmgren (inactive)]

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.

Comment 15

[Phil Ringnalda (:philor)]

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

Comment 16

[Pulsebot]

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

Comment 17

[Phil Ringnalda (:philor)]

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

Comment 18

[Julien Cristau [:jcristau]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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