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

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hunboy, Assigned: mats)

Tracking

({testcase})

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

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

Attachments

(6 attachments)

(Reporter)

Description

2 years ago
Created attachment 8849132 [details]
gridoverlap.html

Overflowed contents overlaps each others with grid and scrollbars invisible (behing other content). Testcase attached.
(Reporter)

Comment 1

2 years ago
Created attachment 8849133 [details]
expected result / png

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)

Comment 4

2 years ago
Created 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).
Attachment #8858120 - Flags: review?(dholbert)
(Assignee)

Comment 5

2 years ago
Created attachment 8858121 [details] [diff] [review]
part 2 - [css-grid] Propagate the ApplyAutoMinSize bit to nsFrame::ComputeSize (idempotent patch).
Attachment #8858121 - Flags: review?(dholbert)
(Assignee)

Comment 6

2 years ago
Created 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.
Attachment #8858122 - Flags: review?(dholbert)
(Assignee)

Comment 7

2 years ago
Created attachment 8858124 [details] [diff] [review]
part 4 - [css-grid] Reftests for grid items that stretch with no Automatic Minimum Size.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89834e552940c9c7be11b81cde495eb76dfab2ab
Attachment #8858124 - Flags: review?(dholbert)
(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+

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d8487e949e27
https://hg.mozilla.org/mozilla-central/rev/af48eb90e608
https://hg.mozilla.org/mozilla-central/rev/a6335afb7574
https://hg.mozilla.org/mozilla-central/rev/ee2e388ae3d1
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(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+
status-firefox54: --- → affected
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.