Closed
Bug 1348857
Opened 8 years ago
Closed 8 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: MatsPalmgren_bugz)
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 |
Overflowed contents overlaps each others with grid and scrollbars invisible (behing other content). Testcase attached.
Reporter | ||
Comment 1•8 years ago
|
||
The expected result in png - (screenshot from Chromium Canary)
Assignee | ||
Comment 3•8 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•8 years ago
|
||
Attachment #8858120 -
Flags: review?(dholbert)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8858121 -
Flags: review?(dholbert)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8858122 -
Flags: review?(dholbert)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8858124 -
Flags: review?(dholbert)
Comment 10•8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
Flags: in-testsuite+
Comment 16•8 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: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 17•8 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•8 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•8 years ago
|
status-firefox54:
--- → affected
Comment 19•8 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•8 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•8 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
•