Skip to content

Commit

Permalink
Image: Fix layout shift when lightbox is opened and closed (#53026)
Browse files Browse the repository at this point in the history 
* Replace overflow: hidden with JavaScript callback to prevent scrolling

* Disable scroll callback on mobile; add comments; fix scrim styles

The page jumps around when trying to override the scroll behavior
on mobile, so I disabled it altogether. I've also added comments
to clarify this and other decisions made around the scroll handling.

While testing, I realized that the scrim was completely opaque during
the zoom animation, which does not match the design, so I added a new
animation specifically for the scrim to fix that.

* Add handling for horizontally oriented pages

* Move close button so that it's further from scrollbar on desktop

* Fix call to handleScroll() and add touch callback to new render method

* Improve lightbox experience on mobile

To ensure pinch to zoom works as expected when the lightbox
is open on mobile, I added logic to disable the scroll override
when touch is detected. Without this, the scroll override kicks
in whenever one tries to pinch to zoom, and it breaks the experience.

I also revised the styles for the scrim to make it opaque, as having
content visible outside of the lightbox is distracting when
pinching to zoom on a mobile device, and I think having a consistent
lightbox across devices will make for the best user experience.

* Fix spacing

* Add touch directives to block supports

* Fix spacing in block supports

* Rename attribute for clarity

* Update comment

* Update comments

* Fix spacing

---------

Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com>
  • Loading branch information
@cbravobernal @artemiomorales
cbravobernal and artemiomorales authored Sep 28, 2023
1 parent 25d7b32 commit 9b2f4bf
Show file tree
Hide file tree
Showing 4 changed files with 145 additions and 52 deletions.
4 changes: 3 additions & 1 deletion lib/block-supports/behaviors.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Expand Down
4 changes: 3 additions & 1 deletion packages/block-library/src/image/index.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Expand Down
8 changes: 2 additions & 6 deletions packages/block-library/src/image/style.scss
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -297,10 +297,6 @@
}
}

html.wp-has-lightbox-open {
overflow: hidden;
}

@keyframes turn-on-visibility {
0% {
opacity: 0;
Expand Down
181 changes: 137 additions & 44 deletions packages/block-library/src/image/view.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand All @@ -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( {
Expand Down Expand Up @@ -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;
},
},
},
},
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 () => {
Expand Down

1 comment on commit 9b2f4bf

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flaky tests detected in 9b2f4bf.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6342106360
📝 Reported issues:

Please sign in to comment.