Closed
Bug 1348857
Opened 3 years ago
Closed 3 years ago
[css-grid] Intristic content with overflow:auto overlaps in grid
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: hunboy, Assigned: mats)
References
Details
(Keywords: testcase)
Attachments
(6 files)
|
472 bytes,
text/html
|
Details | |
|
4.49 KB,
image/png
|
Details | |
|
7.59 KB,
patch
|
dholbert
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
|
8.07 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
|
1.48 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
|
29.31 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
expected result / png
Overflowed contents overlaps each others with grid and scrollbars invisible (behing other content). Testcase attached.
|
Reporter | |
Comment 1•3 years ago
|
||
The expected result in png - (screenshot from Chromium Canary)
|
Assignee | |
Comment 3•3 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•3 years ago
|
||
Attachment #8858120 -
Flags: review?(dholbert)
|
|
Assignee | |
Comment 5•3 years ago
|
||
Attachment #8858121 -
Flags: review?(dholbert)
|
|
Assignee | |
Comment 6•3 years ago
|
||
Attachment #8858122 -
Flags: review?(dholbert)
|
|
Assignee | |
Comment 7•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89834e552940c9c7be11b81cde495eb76dfab2ab
Attachment #8858124 -
Flags: review?(dholbert)
|
||
Comment 10•3 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). 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 11•3 years ago
|
||
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 12•3 years ago
|
||
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 13•3 years ago
|
||
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•3 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•3 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•3 years ago
|
Flags: in-testsuite+
|
||
Comment 16•3 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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
|
|
Assignee | |
Comment 17•3 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 18•3 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). Fix a css grid issue. Beta54+. Should be in 54 beta 2.
Attachment #8858120 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
|
|
||
Updated•3 years ago
|
status-firefox54:
--- → affected
|
|
||
Comment 19•3 years ago
|
||
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)
|
|
Assignee | |
Comment 20•3 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/0065e54275aa65e9718c7a67db5c002b9605e8af https://hg.mozilla.org/releases/mozilla-beta/rev/20cc389b33bd8a4e80b253a14b7ee27e61f5acfa https://hg.mozilla.org/releases/mozilla-beta/rev/9019477162f6cc7070975ee16327729abeb7eecd https://hg.mozilla.org/releases/mozilla-beta/rev/188cc9e6d18d8dff122b0190e19c1e658b6d0f29
Flags: needinfo?(mats)
|
||
Comment 21•3 years ago
|
||
(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.
Description
•