[css-grid] Intristic content with overflow:auto overlaps in grid

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hunboy, Assigned: mats)

Tracking

({testcase})

Trunk
mozilla55
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

Attachments

(6 attachments)

Reporter

Description

2 years ago
Posted file gridoverlap.html
Overflowed contents overlaps each others with grid and scrollbars invisible (behing other content). Testcase attached.
Reporter

Comment 1

2 years ago
The expected result in png - (screenshot from Chromium Canary)
Assignee

Updated

2 years ago
Assignee: nobody → mats
Keywords: testcase
Assignee

Updated

2 years ago
Duplicate of this bug: 1355445
Assignee

Comment 3

2 years ago
Automatic Minimum Size (AMS) only applies when 'overflow' is 'visible'
(and some other conditions):
https://drafts.csswg.org/css-grid/#min-size-auto
but when AMS doesn't apply, 'min-width:auto' makes the minimum size zero,
which in turn enables stretching to shrink it down to zero size if needed.
https://drafts.csswg.org/css-align-3/#valdef-justify-self-stretch

We handle this in most places in our code, but we missed one spot where we
apply AMS to grid items whether the conditions for AMS are true or not.
Assignee

Updated

2 years ago
Duplicate of this bug: 1349798
Assignee

Updated

2 years ago
Duplicate of this bug: 1351557
Comment on attachment 8858120 [details] [diff] [review]
part 1 - [css-grid] Add a bit on GridItemInfo that says if we should apply Automatic Minimum Size or not (idempotent patch).

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

r=me

::: layout/generic/nsGridContainerFrame.cpp
@@ +4150,5 @@
>    iter.Reset();
>    for (; !iter.AtEnd(); iter.Next()) {
>      auto& gridItem = aGridItems[iter.ItemIndex()];
> +
> +    // Check if need to apply "Automatic Minimum Size" and cache it.

s/if need/if we need/
Attachment #8858120 - Flags: review?(dholbert) → review+
Comment on attachment 8858121 [details] [diff] [review]
part 2 - [css-grid] Propagate the ApplyAutoMinSize bit to nsFrame::ComputeSize (idempotent patch).

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

r=me
Attachment #8858121 - Flags: review?(dholbert) → review+
Comment on attachment 8858122 [details] [diff] [review]
part 3 - [css-grid] Only apply Automatic Minimum Size when the ApplyAutoMinSize bit is set, otherwise the minimum size is zero unless specified.

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

r=me
Attachment #8858122 - Flags: review?(dholbert) → review+
Comment on attachment 8858124 [details] [diff] [review]
part 4 - [css-grid] Reftests for grid items that stretch with no Automatic Minimum Size.

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

Ideally these tests (and most/all of their siblings) should be in a subdirectory of w3c-css/submitted so they're shared with other browser vendors.

It looks like we don't have a subdirectory there for grid yet, though it seems like you've got the right w3c metadata in these tests, so they're probably good to be migrated. Could you file a bug on migrating them to that location, if there isn't already a bug on that?

Also: tests 003, 004, and 006 here seem to pass already in current Nightly, to my eye at least (loading testcase/reference side by side). Is that OK/expected?

::: layout/reftests/css-grid/grid-item-overflow-stretch-001-ref.html
@@ +5,5 @@
> +-->
> +<html><head>
> +  <meta charset="utf-8">
> +  <title>CSS Grid Reference: stretching overflow!=visible items</title>
> +  <link rel="author" title="Mats Palmgren" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1319688">

Wrong bug number (throughout this patch, it seems)

@@ +24,5 @@
> +  border:1px solid;
> +  min-width:0;
> +  min-height:0;
> +  -webkit-box-sizing: border-box;
> +  box-sizing: border-box;

Do you really need the redundant -webkit-box-sizing declaration here?

http://caniuse.com/#search=box-sizing suggests that the unprefixed version should be fine everywhere (as does https://developer.mozilla.org/en-US/docs/Web/CSS/box-sizing#Browser_compatibility , if I'm reading it correctly)

(This applies to the first few files in this patch, it seems.)
Attachment #8858124 - Flags: review?(dholbert) → review+

Comment 14

2 years ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8487e949e27
part 1 - [css-grid] Add a bit on GridItemInfo that says if we should apply Automatic Minimum Size or not (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/af48eb90e608
part 2 - [css-grid] Propagate the ApplyAutoMinSize bit to nsFrame::ComputeSize (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6335afb7574
part 3 - [css-grid] Only apply Automatic Minimum Size when the ApplyAutoMinSize bit is set, otherwise the minimum size is zero unless specified.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2e388ae3d1
part 4 - [css-grid] Reftests for grid items that stretch with no Automatic Minimum Size.
Assignee

Comment 15

2 years ago
> Ideally these tests [...] should be in a subdirectory of w3c-css/submitted

I intend to submit most of these to W3C at some point.

> Also: tests 003, 004, and 006 here seem to pass already in current Nightly, to my eye at least (loading testcase/reference
> side by side). Is that OK/expected?

Right, I'm aware of this, I just added as many combinations of the original problem
I could think of to get better test coverage.

> Do you really need the redundant -webkit-box-sizing declaration here?

Removed.
Assignee

Updated

2 years ago
Flags: in-testsuite+
Assignee

Comment 17

2 years ago
Comment on attachment 8858120 [details] [diff] [review]
part 1 - [css-grid] Add a bit on GridItemInfo that says if we should apply Automatic Minimum Size or not (idempotent patch).

Approval Request Comment
[Feature/Bug causing the regression]: CSS Grid feature
[User impact if declined]:broken grid layout
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:we should take 1356820, 1348857, 1350925 in that order to avoid conflicts (they are technically independent though)
[Is the change risky?]:no
[Why is the change risky/not risky?]:only affects grid layout
[String changes made/needed]:none

I'd like to uplift all three of bug 1356820, bug 1348857, bug 1350925
to Beta v54 to fix some Grid layout bugs that web developers have reported.
Attachment #8858120 - Flags: approval-mozilla-beta?
Comment on attachment 8858120 [details] [diff] [review]
part 1 - [css-grid] Add a bit on GridItemInfo that says if we should apply Automatic Minimum Size or not (idempotent patch).

Fix a css grid issue. Beta54+. Should be in 54 beta 2.
Attachment #8858120 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
needs rebasing

grafting 412685:d8487e949e27 "Bug 1348857 part 1 - [css-grid] Add a bit on GridItemInfo that says if we should apply Automatic Minimum Size or not (idempotent patch).  r=dholbert"
merging layout/generic/nsGridContainerFrame.cpp
warning: conflicts while merging layout/generic/nsGridContainerFrame.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #17)
> [Is this code covered by automated tests?]:yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Mats' assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Assignee

Updated

2 years ago
Duplicate of this bug: 1369180
Duplicate of this bug: 1319688
You need to log in before you can comment on or make changes to this bug.