-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Image: Fix layout shift when lightbox is opened and closed #53026
Changes from 9 commits
3a43283
c438e15
4168036
564db4c
2f7af76
2c94042
aa2b495
2e38ab0
59d32e8
057a164
dc7a8b4
734096a
82eb2ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,43 @@ const focusableSelectors = [ | |
'[tabindex]:not([tabindex^="-"])', | ||
]; | ||
|
||
// I've defined the scroll handling outside of the store | ||
// to prevent confusion as to how this logic is called. | ||
// This logic is not referenced directly by the directives | ||
// in the markup because the scroll event we need to listen | ||
// to is triggered on the window; so we define it outside of | ||
// the store to signal that the behavior here is different. | ||
// If we find a compelling reason to move it to the store, | ||
// feel free to do so. | ||
let scrollCallback; | ||
|
||
// We need to track whether a user is touching the screen so we | ||
// can handle the scroll override differently on mobile devices. | ||
let isTouching = false; | ||
let lastTouchTime = 0; | ||
|
||
// Using overflow: hidden to prevent scrolling while the lightbox | ||
// is open causes the content to shift and prevents the zoom | ||
// animation from working in some cases because we're unable to | ||
// account for the layout shift when doing the animation calculations. | ||
// Instead, here we use JavaScript to prevent and reset the scrolling | ||
// behavior. In the future, we may be able to use CSS or overflow: hidden | ||
// instead to not rely on JavaScript, but this seems to be the best approach | ||
// for now that provides the best visual experience. | ||
function handleScroll( context ) { | ||
// We can't override the scroll behavior on mobile devices | ||
// because doing so breaks the pinch to zoom functionality, and we | ||
// want to allow users to zoom in further on the high-res image. | ||
if ( ! isTouching && Date.now() - lastTouchTime > 450 ) { | ||
// We are unable to use event.preventDefault() to prevent scrolling | ||
// because the scroll event can't be canceled, so we reset the position instead. | ||
window.scrollTo( | ||
context.core.image.lastScrollLeft, | ||
context.core.image.lastScrollTop | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess these are already defined and used, but they feel more like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it! While |
||
); | ||
} | ||
} | ||
|
||
store( | ||
{ | ||
state: { | ||
|
@@ -43,54 +80,50 @@ store( | |
|
||
context.core.image.lightboxEnabled = true; | ||
setStyles( context, event ); | ||
// Hide overflow only when the animation is in progress, | ||
// otherwise the removal of the scrollbars will draw attention | ||
// to itself and look like an error | ||
document.documentElement.classList.add( | ||
'wp-has-lightbox-open' | ||
|
||
context.core.image.lastScrollTop = | ||
window.pageYOffset || | ||
document.documentElement.scrollTop; | ||
|
||
// In most cases, this value will be 0, but this is included | ||
// in case a user has created a page with horizontal scrolling. | ||
context.core.image.lastScrollLeft = | ||
window.pageXOffset || | ||
document.documentElement.scrollLeft; | ||
|
||
// We define and bind the scroll callback here so | ||
// that we can pass the context and as an argument. | ||
// We may be able to change this in the future if we | ||
// define the scroll callback in the store instead, but | ||
// this approach seems to tbe clearest for now. | ||
scrollCallback = handleScroll.bind( null, context ); | ||
|
||
// We need to add a scroll event listener to the window | ||
// here because we are unable to otherwise access it via | ||
// the Interactivity API directives. If we add a native way | ||
// to access the window, we can remove this. | ||
window.addEventListener( | ||
'scroll', | ||
scrollCallback, | ||
false | ||
); | ||
}, | ||
hideLightbox: async ( { context, event } ) => { | ||
hideLightbox: async ( { context } ) => { | ||
context.core.image.hideAnimationEnabled = true; | ||
if ( context.core.image.lightboxEnabled ) { | ||
// If scrolling, wait a moment before closing the lightbox. | ||
if ( | ||
context.core.image.lightboxAnimation === 'fade' | ||
) { | ||
context.core.image.scrollDelta += event.deltaY; | ||
if ( | ||
event.type === 'mousewheel' && | ||
Math.abs( | ||
window.scrollY - | ||
context.core.image.scrollDelta | ||
) < 10 | ||
) { | ||
return; | ||
} | ||
} else if ( | ||
context.core.image.lightboxAnimation === 'zoom' | ||
) { | ||
// Disable scroll until the zoom animation ends. | ||
// Get the current page scroll position | ||
const scrollTop = | ||
window.pageYOffset || | ||
document.documentElement.scrollTop; | ||
const scrollLeft = | ||
window.pageXOffset || | ||
document.documentElement.scrollLeft; | ||
// if any scroll is attempted, set this to the previous value. | ||
window.onscroll = function () { | ||
window.scrollTo( scrollLeft, scrollTop ); | ||
}; | ||
// Enable scrolling after the animation finishes | ||
setTimeout( function () { | ||
window.onscroll = function () {}; | ||
}, 400 ); | ||
} | ||
|
||
document.documentElement.classList.remove( | ||
'wp-has-lightbox-open' | ||
); | ||
// The lightbox will close once it detects a scroll event, | ||
// but we want to wait until the close animation is completed | ||
// before allowing a user to scroll again. The duration of this | ||
// animation is defined in the styles.scss and depends on if the | ||
// animation is 'zoom' or 'fade', but in any case we should wait | ||
// a few milliseconds longer than the duration, otherwise a user | ||
// may scroll too soon and cause the animation to look sloppy. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a practical way to cancel the animation upon scroll without causing the page to shift? sorry if someone has already asked about this; it seems like another approach to avoid jitter in the page without actively preventing someone from doing what they're trying to do. asking because it can be irritating on a website when you try to scroll but have to wait for a prolonged period for some animation to complete. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pardon, the comment here was out of date. The lightbox no longer closes on scroll; users needs to click on the overlay or close button to close it. That being said, I was testing, and we could just get rid of the delay. It's less than half a second though, and if we did get rid of it, the lightbox in that case may look sloppy, especially to less technically inclined users. While we could add robust handling and logic to optimize the UX for a canceled zoom animation, I'm not sure that would be worth the time invested. I feel it requires a lot of effort to close the lightbox then immediately scroll and beat the 450 millisecond delay, after which scroll input is recognized immediately anyway. The robust handling could be added in the future, though I think that can be a separate PR. |
||
setTimeout( function () { | ||
window.removeEventListener( | ||
'scroll', | ||
scrollCallback | ||
); | ||
}, 450 ); | ||
|
||
context.core.image.lightboxEnabled = false; | ||
context.core.image.lastFocusedElement.focus( { | ||
|
@@ -139,6 +172,27 @@ store( | |
ref, | ||
} ); | ||
}, | ||
handleTouchStart: () => { | ||
isTouching = true; | ||
}, | ||
handleTouchMove: ( { context, event } ) => { | ||
// On mobile devices, we want to prevent triggering the | ||
// scroll event because otherwise the page jumps around as | ||
// we reset the scroll position. This also means that closing | ||
// the lightbox requires that a user perform a simple tap. This | ||
// may be changed in the future if we find a better alternative | ||
// to override or reset the scroll position during swipe actions. | ||
if ( context.core.image.lightboxEnabled ) { | ||
event.preventDefault(); | ||
} | ||
}, | ||
handleTouchEnd: () => { | ||
// We need to wait a few milliseconds before resetting | ||
// to ensure that pinch to zoom works consistently | ||
// on mobile devices when the lightbox is open. | ||
lastTouchTime = Date.now(); | ||
isTouching = false; | ||
}, | ||
}, | ||
}, | ||
}, | ||
|
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.
if we turn these into JSDoc comments then the explanation will follow the variables and functions. otherwise they are only evident to those who look at this spot where they're created.
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.
Thanks for the heads up! I've converted these all to JSDoc comments.