Closed Bug 1330380 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect, P1)

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- unaffected
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: brandon, Assigned: mats)

References

(Blocks 2 open bugs)

Details

(Keywords: testcase)

Attachments

(5 files, 2 obsolete files)

Attached file 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.
Attached file reference case
Blocks: css-grid
Component: Untriaged → Layout
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Summary: 'max-width' set on grid item causes text to overflow → [css-grid] 'max-width' set on grid item causes text to overflow
Assignee: nobody → mats
Keywords: testcase
[Tracking Requested - why for this release]: Bug fix for CSS Grid release.  I expect to have a patch for review later today or tomorrow.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Blocks: 1330866
Thanks Mats.  Tracking for 52.
Using "cb" in these variable names is a slight misnomer, and I'll add a true CB
size in the next patch...
Attachment #8826623 - Flags: review?(dholbert)
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.)
Attachment #8826631 - Flags: review?(dholbert)
Just adding a few XXX comments about the MeasuringReflow call associated
with measuring baselines.  I'll address that separately in bug 1330866.
Attachment #8826633 - Flags: review?(dholbert)
Attachment #8825888 - Attachment description: reftest.html → reference case
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)
Attachment #8826631 - Flags: review?(dholbert) → review-
Oh, maybe that's what part 3 is about; sorry if so. Taking a look now.
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.
Attachment #8826631 - Flags: review- → review+
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*
Attachment #8826633 - Flags: review?(dholbert) → review+
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
Attachment #8826631 - Attachment is obsolete: true
Attachment #8826633 - Attachment is obsolete: true
Attachment #8826734 - Flags: review?(dholbert)
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.
Flags: in-testsuite+
Priority: -- → P1
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.
Attachment #8826734 - Flags: approval-mozilla-aurora?
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
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
Attachment #8826734 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.