-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Bento Lightbox: scrollable
feature
#32145
Conversation
id="lightbox" | ||
layout="nodisplay" | ||
animation={animation} | ||
scrollable={scrollable} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the attribute mapping anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean in amp-lightbox.js
? It was already there:
'scrollable': {attr: 'scrollable', type: 'boolean'}, |
Merging since Circle CI and Travis are green, and the failing |
* Fix type * Support `scrollable` in Preact * Support scrollable in AMP layer * Update tests * Add comments * Remove root css * Replace overflow: scroll with overflow: auto
This PR implements the
scrollable
feature forLightbox
/amp-lightbox
. Related to #31052.Scrolling is allowed within the
Lightbox
with application ofoverflow: scroll
as opposed tooverflow: hidden
. It also prevents scrolling beyond theLightbox
in the outer document with application ofoverscroll-behavior: none
andoverflow: scroll
. It appears thatoverflow: hidden
in combination withoverscroll-behavior: none
does not yield scroll containment due to over-scrolling not being possible given the lack of overflow.Unfortunately,
overscroll-behavior
is only partially supported, so we may need to have more robust approaches such as adding an event listener to the element's root document which callspreventDefault
onscroll
when theLightbox
isopen
.