Closed Bug 1481951 Opened Last year Closed Last year

Fix layout containment reftest with floats (contain-layout-overflow-002.html) so that it has valid expectations & passes

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: morgan, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

No description provided.
Priority: -- → P3
Per the spec on layout containment [1] all overflow is supposed to be treated as ink (visual) overflow. This implementation works as of bug 1476495, but fails when a layout-contained element has scrollable overflow produced by floated children. See attached test/reference case.
[1] https://drafts.csswg.org/css-contain/#containment-layout

"
3. If the contents of the element overflow the element, they must be treated as ink overflow.
"
This seems like the sort of thing that's relatively straightforward to debug with rr, by starting from the bad overflow area and working backwards to how it ended up that way.
Actually, I think the behavior here is correct.

I'm assuming that what you thought was the bug was that the second box has a scrollbar.  However, the child of that box, <div class="inner-sm contain border"> has contain:layout, and *it* has a large floating child.

Because of:

>  1. The element establishes an independent formatting context.

the size of the div with class inner-sm will expand to contain its floating descendants.  Then *it* (not its contents) contributes to the scrollable overflow of its parent.


In order to make the test correct, I think there is a need to add something like:

.inner-sm {
  height: 50px;
  width: 50px;
}

(which was, maybe, from the name and from the reference, intended?).


I still don't know what the first box in the reference was intended to match, though.
Yeah, it looks like we match Chrome on the attached testcase, and it's not obvious to me that the float is directly creating any scrollable overflow here (beyond just propping up the height of its contain:layout parent, which *is* allowed to create scrollable overflow).

So maybe this is working as-intended.

> I still don't know what the first box in the reference was intended to match, though.

I think that was left in by mistake - I think the testcase/reference here are shorter versions of other similar testcases, and Morgan just left in a bit in the reference case that she'd removed in the testcase.
[Setting ni=me to land a modified version of the attached patch as a reftest.  Once we've got a test for layout-containment on the parent of a float, for thoroughness/coverage, then I think we can close this out, probably as INVALID.]
Flags: needinfo?(dholbert)
This test is basically a copy of its -001 variant, with some "float:left"
sprinkled around on contained descendants.

Before this patch, this test had an additional arbitrary sizing difference as
compared to the -001 version -- there's one element that arbitrarily has class
"outer" in the -002 test whereas it has class "inner-lg" in the -001 version.
These classes have different sizing characteristics, which makes a difference
to whether scrollbars show up, because this element is not contained (though it
is a layout container itself).

This patch undoes this arbitrary difference and also adds a "float" class to
make it easier to see which elements we've sprinkled float styling onto.

Depends on D3825
Flags: needinfo?(dholbert)
Summary: Layout containment leaks scrollable overflow on floats → ayout containment leaks scrollable overflow on floats
Summary: ayout containment leaks scrollable overflow on floats → Fix layout containment reftest with floats (contain-layout-overflow-002.html) so that it has valid expectations & passes
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Comment on attachment 9002576 [details]
Bug 1481951 part 1: Adjust contain-layout-overflow-* reftests to remove unused rule for nonexistent class "inner-md". r=dbaron

David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 has approved the revision.
Attachment #9002576 - Flags: review+
Comment on attachment 9002577 [details]
Bug 1481951 part 2: Adjust reftest contain-layout-overflow-002.html to be clearer & have accurate expectations. r=dbaron

David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 has approved the revision.
Attachment #9002577 - Flags: review+
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e47a225b907e
part 1: Adjust contain-layout-overflow-* reftests to remove unused rule for nonexistent class "inner-md". r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c3db80981da
part 2: Adjust reftest contain-layout-overflow-002.html to be clearer & have accurate expectations. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/e47a225b907e
https://hg.mozilla.org/mozilla-central/rev/6c3db80981da
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Marking this as qe-verify -, per comment 16.
Flags: qe-verify-

DONTBUILD because this is a comment-only change.

Per bug 1481951 comment 6, it seems our behavior was in fact correct even when
the comment was added, and this "// XXX" comment was just due to a
misunderstanding in what was going on in the testcase.

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f159b8c631d
late-breaking followup: Remove mistaken comment with a reference to this (closed) bug. r=TYLin
You need to log in before you can comment on or make changes to this bug.