Closed Bug 1348857 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: hunboy, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: testcase)

Attachments

(6 files)

gridoverlap.html
472 bytes, text/html
Details
expected result / png
4.49 KB, image/png
Details
part 1 - [css-grid] Add a bit on GridItemInfo that says if we should apply Automatic Minimum Size or not (idempotent patch).
7.59 KB, patch
dholbert
: review+
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review
part 2 - [css-grid] Propagate the ApplyAutoMinSize bit to nsFrame::ComputeSize (idempotent patch).
8.07 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 3 - [css-grid] Only apply Automatic Minimum Size when the ApplyAutoMinSize bit is set, otherwise the minimum size is zero unless specified.
1.48 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 4 - [css-grid] Reftests for grid items that stretch with no Automatic Minimum Size.
29.31 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
Attached file gridoverlap.html — 
Overflowed contents overlaps each others with grid and scrollbars invisible (behing other content). Testcase attached.
Attached image expected result / png — 
The expected result in png - (screenshot from Chromium Canary)
Assignee: nobody → mats
Keywords: testcase
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.
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+
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.
> 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.
Flags: in-testsuite+
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-
You need to log in before you can comment on or make changes to this bug.