Closed Bug 1514843 Opened 6 years ago Closed 6 years ago

tall content size "leaks" through size-containment boundary, inside a scrollframe, via min-height:auto and flex-basis:auto

Categories

(Core :: Layout: Scrolling and Overflow, defect, P3)

Product:
Core
Component:
Layout: Scrolling and Overflow
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(5 files, 1 obsolete file)

testcase 1
(deleted), text/html
Details
testcase 2 (should render as a 50px-tall yellow scrollable area)
(deleted), text/html
Details
Bug 1514843 part 1: Slight refactoring of scrollframe sizing, to use a helper variable for child size. r?TYLin
(deleted), text/x-phabricator-request
Details
Bug 1514843 part 2: Honor 'contain:size' for scrollable elements. r?TYLin
(deleted), text/x-phabricator-request
Details
Bug 1514843 part 3: Add more testcases for 'contain:size' on scrollable elements. r?TYLin
(deleted), text/x-phabricator-request
Details
Attached file (wrong testcase) (obsolete) (deleted) —
STR: 1. Enable CSS Containment; set about:config pref layout.css.contain.enabled = true 2. View attached testcase. EXPECTED RESULTS: The yellow area should only be 50px tall. ACTUAL RESULTS: The yellow area is super tall, due to its tall content. This violates the spec; the yellow thing has 'contain:size' so it should size itself as if it had no content (which in this case means it should be 50px due to its "min-height:50px"). It seems that its content size is "leaking" through the containment boundary, to the flex item ancestor, and it's influencing the used (content-size-dependent) values for "flex-basis:auto" and "min-height:auto" on that flex item ancestor. Chrome gives "expected results".
Ah, nsGfxScrollFrame.cpp (i.e. the nsHTMLScrollFrame implementation) doesn't have any "IsContainSize" checsk. Its reflow code needs an update to handle it having contain:size (just like the code we added for blocks in Bug 1467209). I suspect that's all we need here.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Component: Layout → Layout: Scrolling and Overflow

The yellow area should only be 50px tall.

There's no yellow in that testcase...

Oops, you're right. Looks like I attached the wrong testcase.

I reconstructed a functional testcase based on the bug summary & the description in comment 0, and I'll post that shortly. Also, this should clearly block letting containment hit release (bug 1487493), so I'll mark it as such.

Blocks: 1487493
Attachment #9031946 - Attachment description: testcase 1 → (wrong testcase)
Attached file testcase 1 (deleted) —
Attachment #9031946 - Attachment is obsolete: true

This patch doesn't affect behavior. It just makes it easier for the next patch
in this series to implement contain:size for scrollframes (by conditionally
setting the new helper variable to 0,0 if we have contain:size).

Attachment #9054066 - Attachment description: Bug 1514843 part 1: Slight refactoring of scrollframe sizing, to use a helper variable for child size. → Bug 1514843 part 1: Slight refactoring of scrollframe sizing, to use a helper variable for child size. r?TYLin
Attachment #9054067 - Attachment description: Bug 1514843 part 2: Honor 'contain:size' for intrinsically sized scrollable elements (sizing them as if they were empty). → Bug 1514843 part 2: Honor 'contain:size' for scrollable elements. r?TYLin

This patch adds tweaked copies of the testcases from the prior patch,
exercising the other relevant values for the 'overflow' property (for 'hidden'
and 'auto', rather than 'scroll').

Depends on D25155

TYLin: hope you don't mind that I tagged you to review this. So far I think I've been the main reviewer for containment patches (with interns doing the impl), so I need to find another reviewer for my followup work. :)

Fortunately it's pretty straightforward - contain:size should just make us behave as if there were no children when determining the parent size in layout, which in practice means a special case in GetPref/MinISize functions and a special case in Reflow and/or a helper.

Spec is here: https://drafts.csswg.org/css-contain/#containment-size

And it may be useful to look at IsContainSize() usages in our other container types:
https://searchfox.org/mozilla-central/search?q=IsContainSize()
(The strategy in part 2 here is just the scrollframe version of what we've already got in other container types.)

[oops, meant to CC you, TYLin -- see comment 9, and let me know if you have any questions. Thanks!]

(In reply to Daniel Holbert [:dholbert] from comment #9)

TYLin: hope you don't mind that I tagged you to review this. So far I think I've been the main reviewer for containment patches (with interns doing the impl), so I need to find another reviewer for my followup work. :)

No worries. I'm happy to review those patches :)

I landed this on autoland an hour ago via Lando, but pulsebot didn't post for some reason.

Anyway, here are the commits:
https://hg.mozilla.org/integration/autoland/rev/a4552bbd062b
https://hg.mozilla.org/integration/autoland/rev/f1d82b9507f7
https://hg.mozilla.org/integration/autoland/rev/877d344fd292

https://hg.mozilla.org/mozilla-central/rev/a4552bbd062b
https://hg.mozilla.org/mozilla-central/rev/f1d82b9507f7
https://hg.mozilla.org/mozilla-central/rev/877d344fd292

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: