Image: Fix layout shift when lightbox is opened and closed

lib/block-supports/behaviors.php

@@ -233,7 +233,9 @@ function gutenberg_render_behaviors_support_lightbox( $block_content, $block ) {
             data-wp-bind--aria-modal="context.core.image.lightboxEnabled"
             data-wp-effect="effects.core.image.initLightbox"
             data-wp-on--keydown="actions.core.image.handleKeydown"
-            data-wp-on--mousewheel="actions.core.image.hideLightbox"
+            data-wp-on--touchstart="actions.core.image.handleTouchStart"
+            data-wp-on--touchmove="actions.core.image.handleTouchMove"
+            data-wp-on--touchend="actions.core.image.handleTouchEnd"
             data-wp-on--click="actions.core.image.hideLightbox"
             >
                 <button type="button" aria-label="$close_button_label" style="fill: $close_button_color" class="close-button" data-wp-on--click="actions.core.image.hideLightbox">

packages/block-library/src/image/index.php

@@ -272,7 +272,9 @@ function block_core_image_render_lightbox( $block_content, $block ) {
             aria-modal="false"
             data-wp-effect="effects.core.image.initLightbox"
             data-wp-on--keydown="actions.core.image.handleKeydown"
-            data-wp-on--mousewheel="actions.core.image.hideLightbox"
+            data-wp-on--touchstart="actions.core.image.handleTouchStart"
+            data-wp-on--touchmove="actions.core.image.handleTouchMove"
+            data-wp-on--touchend="actions.core.image.handleTouchEnd"
             data-wp-on--click="actions.core.image.hideLightbox"
             >
                 <button type="button" aria-label="$close_button_label" style="fill: $close_button_color" class="close-button" data-wp-on--click="actions.core.image.hideLightbox">

packages/block-library/src/image/style.scss

@@ -186,8 +186,8 @@
 
 	.close-button {
 		position: absolute;
-		top: calc(env(safe-area-inset-top) + 12.5px);
-		right: calc(env(safe-area-inset-right) + 12.5px);
+		top: calc(env(safe-area-inset-top) + 20px);
+		right: calc(env(safe-area-inset-right) + 20px);
 		padding: 0;
 		cursor: pointer;
 		z-index: 5000000;
@@ -297,10 +297,6 @@
 	}
 }
 
-html.wp-has-lightbox-open {
-	overflow: hidden;
-}
-
 @keyframes turn-on-visibility {
 	0% {
 		opacity: 0;

packages/block-library/src/image/view.js

@@ -17,6 +17,69 @@ const focusableSelectors = [
 	'[tabindex]:not([tabindex^="-"])',
 ];
 
+/*
+ * Stores a context-bound scroll handler.
+ *
+ * This callback could be defined inline inside of the store
+ * object but it's created externally to avoid confusion about
+ * how its 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 by defining it
+ * outside of the store, we signal that the behavior here is different.
+ * If we find a compelling reason to move it to the store, feel free.
+ *
+ * @type {Function}
+ */
+let scrollCallback;
+
+/*
+ * Tracks whether user is touching screen; used to
+ * differentiate behavior for touch and mouse input.
+ *
+ * @type {boolean}
+ */
+let isTouching = false;
+
+/*
+ * Tracks the last time the screen was touched; used to
+ * differentiate behavior for touch and mouse input.
+ *
+ * @type {number}
+ */
+let lastTouchTime = 0;
+
+/*
+ * Lightbox page-scroll handler: prevents scrolling.
+ *
+ * This handler is added to prevent scrolling behaviors that
+ * trigger content shift while the lightbox is open.
+ *
+ * It would be better to accomplish this through CSS alone, but
+ * using overflow: hidden is currently the only way to do so, and
+ * that causes the layout 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.
+ *
+ * @param {Object} context Interactivity page context?
+ */
+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.scrollLeftReset,
+			context.core.image.scrollTopReset
+		);
+	}
+}
+
 store(
 	{
 		state: {
@@ -43,54 +106,49 @@ 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.scrollTopReset =
+							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.scrollLeftReset =
+							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'
-							);
+							// 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.
+							setTimeout( function () {
+								window.removeEventListener(
+									'scroll',
+									scrollCallback
+								);
+							}, 450 );
 
 							context.core.image.lightboxEnabled = false;
 							context.core.image.lastFocusedElement.focus( {
@@ -139,6 +197,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;
+					},
 				},
 			},
 		},
@@ -266,6 +345,13 @@ store(
 	}
 );
 
+/*
+ * Computes styles for the lightbox and adds them to the document.
+ *
+ * @function
+ * @param {Object} context - An Interactivity API context
+ * @param {Object} event - A triggering event
+ */
 function setStyles( context, event ) {
 	// The reference img element lies adjacent
 	// to the event target button in the DOM.
@@ -434,6 +520,13 @@ function setStyles( context, event ) {
 	`;
 }
 
+/*
+ * Debounces a function call.
+ *
+ * @function
+ * @param {Function} func - A function to be called
+ * @param {number} wait - The time to wait before calling the function
+ */
 function debounce( func, wait = 50 ) {
 	let timeout;
 	return () => {