Should Dialog let contents overflow outside?

[siddharthkp]

When an element inside <details-dialog> is longer than the dialog, it results in a scroll on the dialog which isn't ideal:

overflow-auto.mov

This can be reproduced in the Dialog with <include-fragment> example: https://github.github.io/details-dialog-element/example/index.html

Screen.Recording.2022-03-16.at.2.36.28.PM.mov

The cause can be narrowed down to a overflow:auto index.css#L12

Removing that line, does make it behave nicely. But, I'm not sure what would break instead OR if there are any tradeoffs.

without-overflow-auto.mov

More screenshots via @dipree:

[dipree]

The overflow: auto was introduced a while back and the reason is documented here #63. But the version with this change (3.1.3) was only recently (9 days ago) introduced by https://github.com/github/github/commit/77a0d5b478d346e830cec5f9284effa156f834dd.

overflow: auto will solve the problem of allowing to access the content when the viewport is too small but it will also cause elements that should overflow, like a dropdown or select menu, to be swallowed by the dialog.

[dipree]

What triggered the version bump https://github.com/github/codespaces/issues/6834

[pqt]

Something I want to note here is that in your third video demonstrating the overflow, you scroll but not so much as to scroll the page, in #63 that is the problem being targeted (and solved) by the overflow: auto in the PR.

I'd personally be advocating for the contents to not overflow by default, however, in the case where an unknown number of options can appear in a dropdown it would make sense to reset the overflow to visible (.overflow-visible) on a case-by-case basis.

[siddharthkp]

@pqt Good shout!

Looking at #63, feels like we might have a regression?:

(changed the height of the dialog with developer tools)

regression.mov

---

Scrolling on the transfer issue dialog:

1. with overflow:auto (this is on production today)

   You can scroll all the sections on the page:

   everything.scrolls.mov

2. without overflow: auto

   one.less.scroll.mov