Skip to content
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

Merged
merged 13 commits into from
Sep 28, 2023

Conversation

cbravobernal
Copy link
Contributor

@cbravobernal cbravobernal commented Jul 27, 2023

What?

Currently, opening and closing the lightbox causes the website layout to shift, which can cause the zoom animation to appear broken, particularly on mobile. It is also possible to scroll the webpage while the lightbox is open by directly manipulating the scrollbar or using a mobile device, which is not in line with web standards.

This PR improves the lightbox implementation by removing the layout shift and updating the scroll handling so that the lightbox behaves reliably whenever a scroll is performed.

Why?

We want to offer a polished lightbox experience, and the current experience appears broken in some cases.

How?

This PR removes overflow: hidden from the styles and updates the callback for the scroll handling.

Note: Expected behavior is that scrolling will be disabled on both desktop and mobile.

Testing Instructions

  1. Insert an image in a post and add enough content to ensure that the scrollbar will be visible.
  2. Enable the lightbox using the block settings.
  3. Publish and view the post on desktop.
  4. Click on the image to open and close the lightbox — make sure that the scrollbar remains intact and that there is no layout shift.
  5. Attempt scrolling while the lightbox is open using the mousewheel, up and down arrow keys, the PageUp and PageDown keys, and by directly clicking and dragging the scrollbar — verify that the lightbox stays open and doesn't scroll when performing any of those actions.
  6. On mobile devices, test that scrolling is disabled and that the lightbox remains open when attempting to scroll using touch input. Test also that pinch to zoom while the lightbox is open works reasonably well.

Screenshare

Layout Shift Demonstration

Before the Fix

Lightbox.layout.shift.-.broken.mp4

After the Fix

Lightbox.layout.shift.-.fixed.mp4

@cbravobernal cbravobernal added [Type] Bug An existing feature does not function as intended [Block] Image Affects the Image Block labels Jul 27, 2023
@cbravobernal cbravobernal self-assigned this Jul 27, 2023
@github-actions
Copy link

github-actions bot commented Jul 27, 2023

Size Change: +812 B (0%)

Total Size: 1.62 MB

Filename Size Change
build/block-directory/index.min.js 7.07 kB +15 B (0%)
build/block-editor/index.min.js 218 kB +51 B (0%)
build/block-library/blocks/image/style-rtl.css 1.4 kB -18 B (-1%)
build/block-library/blocks/image/style.css 1.39 kB -18 B (-1%)
build/block-library/blocks/image/view.min.js 1.83 kB +2 B (0%)
build/block-library/blocks/search/style-rtl.css 613 B +9 B (+1%)
build/block-library/blocks/search/style.css 613 B +9 B (+1%)
build/block-library/index.min.js 207 kB +111 B (0%)
build/block-library/style-rtl.css 14 kB -13 B (0%)
build/block-library/style.css 14 kB -14 B (0%)
build/blocks/index.min.js 51.5 kB +7 B (0%)
build/commands/index.min.js 15.5 kB +6 B (0%)
build/components/index.min.js 248 kB +80 B (0%)
build/components/style-rtl.css 11.8 kB +28 B (0%)
build/components/style.css 11.8 kB +28 B (0%)
build/compose/index.min.js 12.7 kB +1 B (0%)
build/core-commands/index.min.js 2.72 kB +101 B (+4%)
build/core-data/index.min.js 70.5 kB +8 B (0%)
build/customize-widgets/index.min.js 12 kB +11 B (0%)
build/data/index.min.js 8.87 kB +9 B (0%)
build/edit-post/index.min.js 35.7 kB +29 B (0%)
build/edit-site/index.min.js 185 kB +193 B (0%)
build/edit-widgets/index.min.js 17 kB +18 B (0%)
build/editor/index.min.js 45.9 kB +10 B (0%)
build/format-library/index.min.js 7.79 kB +36 B (0%)
build/keyboard-shortcuts/index.min.js 1.76 kB +18 B (+1%)
build/list-reusable-blocks/index.min.js 2.21 kB +11 B (0%)
build/nux/index.min.js 2.01 kB +8 B (0%)
build/patterns/index.min.js 3.57 kB +8 B (0%)
build/plugins/index.min.js 1.81 kB +8 B (0%)
build/preferences/index.min.js 1.26 kB +3 B (0%)
build/react-i18n/index.min.js 631 B +7 B (+1%)
build/reusable-blocks/index.min.js 2.73 kB +9 B (0%)
build/rich-text/index.min.js 10.2 kB +14 B (0%)
build/router/index.min.js 1.79 kB +8 B (0%)
build/server-side-render/index.min.js 1.96 kB +11 B (+1%)
build/viewport/index.min.js 967 B -1 B (0%)
build/widgets/index.min.js 7.18 kB +9 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 964 B
build/annotations/index.min.js 2.71 kB
build/api-fetch/index.min.js 2.29 kB
build/autop/index.min.js 2.11 kB
build/blob/index.min.js 461 B
build/block-directory/style-rtl.css 1.04 kB
build/block-directory/style.css 1.04 kB
build/block-editor/content-rtl.css 4.28 kB
build/block-editor/content.css 4.27 kB
build/block-editor/default-editor-styles-rtl.css 403 B
build/block-editor/default-editor-styles.css 403 B
build/block-editor/style-rtl.css 15.6 kB
build/block-editor/style.css 15.6 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 90 B
build/block-library/blocks/archives/style.css 90 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/audio/theme-rtl.css 138 B
build/block-library/blocks/audio/theme.css 138 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 104 B
build/block-library/blocks/avatar/style.css 104 B
build/block-library/blocks/block/editor-rtl.css 305 B
build/block-library/blocks/block/editor.css 305 B
build/block-library/blocks/button/editor-rtl.css 587 B
build/block-library/blocks/button/editor.css 587 B
build/block-library/blocks/button/style-rtl.css 633 B
build/block-library/blocks/button/style.css 632 B
build/block-library/blocks/buttons/editor-rtl.css 337 B
build/block-library/blocks/buttons/editor.css 337 B
build/block-library/blocks/buttons/style-rtl.css 332 B
build/block-library/blocks/buttons/style.css 332 B
build/block-library/blocks/calendar/style-rtl.css 239 B
build/block-library/blocks/calendar/style.css 239 B
build/block-library/blocks/categories/editor-rtl.css 113 B
build/block-library/blocks/categories/editor.css 112 B
build/block-library/blocks/categories/style-rtl.css 124 B
build/block-library/blocks/categories/style.css 124 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/style-rtl.css 121 B
build/block-library/blocks/code/style.css 121 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 421 B
build/block-library/blocks/columns/style.css 421 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 199 B
build/block-library/blocks/comment-template/style.css 198 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 840 B
build/block-library/blocks/comments/editor.css 839 B
build/block-library/blocks/comments/style-rtl.css 637 B
build/block-library/blocks/comments/style.css 636 B
build/block-library/blocks/cover/editor-rtl.css 647 B
build/block-library/blocks/cover/editor.css 650 B
build/block-library/blocks/cover/style-rtl.css 1.7 kB
build/block-library/blocks/cover/style.css 1.69 kB
build/block-library/blocks/details/editor-rtl.css 65 B
build/block-library/blocks/details/editor.css 65 B
build/block-library/blocks/details/style-rtl.css 98 B
build/block-library/blocks/details/style.css 98 B
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/embed/theme-rtl.css 138 B
build/block-library/blocks/embed/theme.css 138 B
build/block-library/blocks/file/editor-rtl.css 316 B
build/block-library/blocks/file/editor.css 316 B
build/block-library/blocks/file/style-rtl.css 311 B
build/block-library/blocks/file/style.css 312 B
build/block-library/blocks/file/view.min.js 321 B
build/block-library/blocks/footnotes/style-rtl.css 201 B
build/block-library/blocks/footnotes/style.css 199 B
build/block-library/blocks/freeform/editor-rtl.css 2.61 kB
build/block-library/blocks/freeform/editor.css 2.61 kB
build/block-library/blocks/gallery/editor-rtl.css 957 B
build/block-library/blocks/gallery/editor.css 962 B
build/block-library/blocks/gallery/style-rtl.css 1.55 kB
build/block-library/blocks/gallery/style.css 1.55 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 654 B
build/block-library/blocks/group/editor.css 654 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 189 B
build/block-library/blocks/heading/style.css 189 B
build/block-library/blocks/html/editor-rtl.css 340 B
build/block-library/blocks/html/editor.css 341 B
build/block-library/blocks/image/editor-rtl.css 834 B
build/block-library/blocks/image/editor.css 833 B
build/block-library/blocks/image/theme-rtl.css 137 B
build/block-library/blocks/image/theme.css 137 B
build/block-library/blocks/latest-comments/style-rtl.css 357 B
build/block-library/blocks/latest-comments/style.css 357 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 478 B
build/block-library/blocks/latest-posts/style.css 478 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 505 B
build/block-library/blocks/media-text/style.css 503 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 671 B
build/block-library/blocks/navigation-link/editor.css 672 B
build/block-library/blocks/navigation-link/style-rtl.css 115 B
build/block-library/blocks/navigation-link/style.css 115 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation/editor-rtl.css 2.26 kB
build/block-library/blocks/navigation/editor.css 2.26 kB
build/block-library/blocks/navigation/style-rtl.css 2.26 kB
build/block-library/blocks/navigation/style.css 2.25 kB
build/block-library/blocks/navigation/view.min.js 1.01 kB
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 401 B
build/block-library/blocks/page-list/editor.css 401 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 235 B
build/block-library/blocks/paragraph/editor.css 235 B
build/block-library/blocks/paragraph/style-rtl.css 335 B
build/block-library/blocks/paragraph/style.css 335 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/editor-rtl.css 96 B
build/block-library/blocks/post-comments-form/editor.css 96 B
build/block-library/blocks/post-comments-form/style-rtl.css 508 B
build/block-library/blocks/post-comments-form/style.css 508 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 71 B
build/block-library/blocks/post-excerpt/editor.css 71 B
build/block-library/blocks/post-excerpt/style-rtl.css 141 B
build/block-library/blocks/post-excerpt/style.css 141 B
build/block-library/blocks/post-featured-image/editor-rtl.css 588 B
build/block-library/blocks/post-featured-image/editor.css 586 B
build/block-library/blocks/post-featured-image/style-rtl.css 322 B
build/block-library/blocks/post-featured-image/style.css 322 B
build/block-library/blocks/post-navigation-link/style-rtl.css 215 B
build/block-library/blocks/post-navigation-link/style.css 214 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 314 B
build/block-library/blocks/post-template/style.css 314 B
build/block-library/blocks/post-terms/style-rtl.css 96 B
build/block-library/blocks/post-terms/style.css 96 B
build/block-library/blocks/post-time-to-read/style-rtl.css 69 B
build/block-library/blocks/post-time-to-read/style.css 69 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 125 B
build/block-library/blocks/preformatted/style.css 125 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 335 B
build/block-library/blocks/pullquote/style.css 335 B
build/block-library/blocks/pullquote/theme-rtl.css 168 B
build/block-library/blocks/pullquote/theme.css 168 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 288 B
build/block-library/blocks/query-pagination/style.css 284 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 486 B
build/block-library/blocks/query/editor.css 486 B
build/block-library/blocks/query/style-rtl.css 375 B
build/block-library/blocks/query/style.css 372 B
build/block-library/blocks/query/view.min.js 609 B
build/block-library/blocks/quote/style-rtl.css 222 B
build/block-library/blocks/quote/style.css 222 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 B
build/block-library/blocks/rss/editor-rtl.css 149 B
build/block-library/blocks/rss/editor.css 149 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 184 B
build/block-library/blocks/search/editor.css 184 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/search/view.min.js 471 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 234 B
build/block-library/blocks/separator/style.css 234 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 329 B
build/block-library/blocks/shortcode/editor.css 329 B
build/block-library/blocks/site-logo/editor-rtl.css 760 B
build/block-library/blocks/site-logo/editor.css 760 B
build/block-library/blocks/site-logo/style-rtl.css 204 B
build/block-library/blocks/site-logo/style.css 204 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 116 B
build/block-library/blocks/site-title/editor.css 116 B
build/block-library/blocks/site-title/style-rtl.css 57 B
build/block-library/blocks/site-title/style.css 57 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 682 B
build/block-library/blocks/social-links/editor.css 681 B
build/block-library/blocks/social-links/style-rtl.css 1.45 kB
build/block-library/blocks/social-links/style.css 1.45 kB
build/block-library/blocks/spacer/editor-rtl.css 359 B
build/block-library/blocks/spacer/editor.css 359 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 432 B
build/block-library/blocks/table/editor.css 432 B
build/block-library/blocks/table/style-rtl.css 646 B
build/block-library/blocks/table/style.css 645 B
build/block-library/blocks/table/theme-rtl.css 157 B
build/block-library/blocks/table/theme.css 157 B
build/block-library/blocks/tag-cloud/style-rtl.css 251 B
build/block-library/blocks/tag-cloud/style.css 253 B
build/block-library/blocks/template-part/editor-rtl.css 403 B
build/block-library/blocks/template-part/editor.css 403 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/term-description/style-rtl.css 111 B
build/block-library/blocks/term-description/style.css 111 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 99 B
build/block-library/blocks/verse/style.css 99 B
build/block-library/blocks/video/editor-rtl.css 552 B
build/block-library/blocks/video/editor.css 555 B
build/block-library/blocks/video/style-rtl.css 191 B
build/block-library/blocks/video/style.css 191 B
build/block-library/blocks/video/theme-rtl.css 139 B
build/block-library/blocks/video/theme.css 139 B
build/block-library/classic-rtl.css 179 B
build/block-library/classic.css 179 B
build/block-library/common-rtl.css 1.11 kB
build/block-library/common.css 1.11 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/editor-rtl.css 12.2 kB
build/block-library/editor.css 12.2 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/reset-rtl.css 472 B
build/block-library/reset.css 472 B
build/block-library/theme-rtl.css 700 B
build/block-library/theme.css 705 B
build/block-serialization-default-parser/index.min.js 1.13 kB
build/block-serialization-spec-parser/index.min.js 2.87 kB
build/commands/style-rtl.css 947 B
build/commands/style.css 942 B
build/customize-widgets/style-rtl.css 1.51 kB
build/customize-widgets/style.css 1.5 kB
build/data-controls/index.min.js 651 B
build/date/index.min.js 17.9 kB
build/deprecated/index.min.js 462 B
build/dom-ready/index.min.js 336 B
build/dom/index.min.js 4.68 kB
build/edit-post/classic-rtl.css 571 B
build/edit-post/classic.css 571 B
build/edit-post/style-rtl.css 7.92 kB
build/edit-post/style.css 7.91 kB
build/edit-site/style-rtl.css 14 kB
build/edit-site/style.css 14 kB
build/edit-widgets/style-rtl.css 4.84 kB
build/edit-widgets/style.css 4.84 kB
build/editor/style-rtl.css 3.58 kB
build/editor/style.css 3.58 kB
build/element/index.min.js 4.87 kB
build/escape-html/index.min.js 548 B
build/format-library/style-rtl.css 577 B
build/format-library/style.css 577 B
build/hooks/index.min.js 1.57 kB
build/html-entities/index.min.js 454 B
build/i18n/index.min.js 3.61 kB
build/interactivity/index.min.js 11.4 kB
build/is-shallow-equal/index.min.js 535 B
build/keycodes/index.min.js 1.9 kB
build/list-reusable-blocks/style-rtl.css 865 B
build/list-reusable-blocks/style.css 865 B
build/media-utils/index.min.js 2.92 kB
build/notices/index.min.js 964 B
build/nux/style-rtl.css 775 B
build/nux/style.css 771 B
build/patterns/style-rtl.css 325 B
build/patterns/style.css 325 B
build/preferences-persistence/index.min.js 1.85 kB
build/primitives/index.min.js 994 B
build/priority-queue/index.min.js 1.52 kB
build/private-apis/index.min.js 972 B
build/react-refresh-entry/index.min.js 9.46 kB
build/react-refresh-runtime/index.min.js 6.78 kB
build/redux-routine/index.min.js 2.71 kB
build/reusable-blocks/style-rtl.css 265 B
build/reusable-blocks/style.css 265 B
build/shortcode/index.min.js 1.4 kB
build/style-engine/index.min.js 1.98 kB
build/token-list/index.min.js 587 B
build/url/index.min.js 3.84 kB
build/vendors/inert-polyfill.min.js 2.48 kB
build/vendors/react-dom.min.js 41.8 kB
build/vendors/react.min.js 4.02 kB
build/warning/index.min.js 259 B
build/widgets/style-rtl.css 1.18 kB
build/widgets/style.css 1.18 kB
build/wordcount/index.min.js 1.03 kB

compressed-size-action

Copy link
Contributor

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

Yeah, this seems reasonable to me 👍

@cbravobernal cbravobernal added the Needs Accessibility Feedback Need input from accessibility label Jul 28, 2023
@cbravobernal
Copy link
Contributor Author

I added the accesibility feedback label. Cause @artemiomorales mentioned there could be accesibility issues with this approach.

@alexstine can you help me here? Thanks!

@alexstine
Copy link
Contributor

CC: @joedolson or @afercia .

@artemiomorales
Copy link
Contributor

CC @andrewhayward

@joedolson
Copy link
Contributor

Allowing the background to scroll can be disorienting if a user dismisses the lightbox and is no longer in the same location on the page as they were previously. Focus is set on close, but that won't necessarily land the user in exactly the same place they were when they started.

What about using overflow-y: hidden, to avoid changing the scrollbar behavior?

There's no evidence that has-lightbox-open has any conflicts, surprisingly: https://wpdirectory.net/search/01H6ENN1605ZQ2B4Z7GZFZ61TW

@cbravobernal
Copy link
Contributor Author

There's no evidence that has-lightbox-open has any conflicts, surprisingly: https://wpdirectory.net/search/01H6ENN1605ZQ2B4Z7GZFZ61TW

Renaming to wp-has-lightbox-open is not a problem, and we avoid future problems.

@SantosGuillamot
Copy link
Contributor

SantosGuillamot commented Jul 31, 2023

@joedolson Just to clarify the use case and ensure I understand it correctly:

  • When the user opens the lightbox it covers the whole screen, and there is no overflow. Neither in the X axis nor the Y axis.
  • Scroll is not 100% disabled because, if the user scrolls vertically, it closes the lightbox and focuses on the previous element.

Should overflow be set to hidden in the HTML root, even if there is actually no overflow and the user can scroll to close the lightbox?

Focus is set on close, but that won't necessarily land the user in exactly the same place they were when they started.

Regarding this, as part of this pull request, I added the preventScroll property to ensure the user lands in the same place where they started. Do you think that should be enough?

@artemiomorales
Copy link
Contributor

While testing, I noticed that the overflow: hidden property causes the lightbox zoom out animation to behave incorrectly in some cases.

I also found out that, if one scrolls quickly after opening the lightbox on iOS, that one is able to scroll even through the overflow: hidden property is enabled.

Since it seems like CSS may not be the best tool to prevent scrolling in this scenario considering that hiding the scrollbar causes a layout shift and potential errors with the lightbox calculations, I can explore how to prevent scrolling using JavaScript while the lightbox is open.

Copy link
Contributor

@artemiomorales artemiomorales left a comment

Choose a reason for hiding this comment

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

Without overflow: hidden, one can scroll freely while the lightbox is open on iOS, breaking the experience.

We can either try to find another CSS solution or solve it with JavaScript. I'm leaning towards JavaScript because even with overflow: hidden, the experience breaks sometimes on mobile.

I'll look into addressing this as soon as I have bandwidth, but would be happy for someone else to hop onto this as well 😄

@SantosGuillamot
Copy link
Contributor

SantosGuillamot commented Aug 8, 2023

You're right that removing overflow: hidden allows the user to scroll, breaking the experience. Although I believe it is not an issue only in iOS:

Lightbox.lightbox.-.8.August.2023.mp4

We don't see it on other devices because the lightbox is closed on mousewheel. Maybe we can solve it adding a listener to the touchmove to close it on mobile devices as well. Something like:

data-wp-on--wheel="actions.core.image.hideLightbox"
data-wp-on--touchmove="actions.core.image.hideLightbox"

By the way, it seems that mousewheel is deprecated, so I guess we should use wheel instead.

Maybe another option is using overscroll-behavior, although I am not familiar with that.

Or just keep the overflow: hidden, but I'm aware it was causing some issues as well.

@github-actions
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/block-supports/behaviors.php

@artemiomorales
Copy link
Contributor

@SantosGuillamot How does this approach look?

It feels a bit awkward to put the callback outside of the store, but I can't think of a good reason to have it otherwise.

Defining a general callback for the scroll event seems to handle the following scenarios:

  • Scrolling with a mouse
  • Scrolling via touch on mobile
  • Scrolling via up and down arrows on keyboard
  • Scrolling by dragging the scrollbar

I haven't tested on a physical mobile device yet so I'm not sure what the UX is like in that case, and dragging the scrollbar can feel a bit awkward, but otherwise it seems to work fairly well considering we're overriding the browser's native scrolling behavior.

@SantosGuillamot
Copy link
Contributor

@SantosGuillamot How does this approach look?

I didn't have time to test it, and I'll be AFK for ten days. I am not a big fan of adding global eventListeners, but if we ensure that it only runs when the lightbox is open and only once, I guess it could work.

If I understood it correctly, the other option would be to use:

data-wp-on--wheel="actions.core.image.hideLightbox"
data-wp-on--touchmove="actions.core.image.hideLightbox"

And add the arrow keys in the Escape conditional: link.

And scrolling with the scrollbar wouldn't close the lightbox.

Is this right? I don't know which one is better, to be honest.

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.
@artemiomorales artemiomorales force-pushed the update/remove-scroll-hiding-lightbox branch from a81710c to 2c94042 Compare September 27, 2023 02:06
@artemiomorales
Copy link
Contributor

Thanks everyone for the discussion! I've gone ahead and disabled closing the lightbox on scroll 👍

I've also added a few improvements to make sure scrolling is indeed disabled when the lightbox is open on mobile. I've also added logic to ensure that pinch to zoom works as expected.

Worth noting is that, after testing, I've come to the conclusion that the scrim should be 100% opaque, as having any visible content in the background is distracting when one is doing the pinch to zoom action, and I think it's best to have a consistent user experience for the lightbox across devices.

I believe this PR is ready for a hopefully final review 😄

@artemiomorales artemiomorales changed the title Image: Fix layout shift and styles when lightbox is opened and closed Image: Fix layout shift when lightbox is opened and closed Sep 27, 2023
@github-actions
Copy link

github-actions bot commented Sep 27, 2023

Flaky tests detected in 82eb2ba.
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/6341647450
📝 Reported issues:

@joedolson
Copy link
Contributor

Making sure pinch to zoom works and a fully opaque scrim are both wins from my perspective. Thanks @artemiomorales!

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

there's talk in the patch about differentiating mobile and desktop behavior, but I didn't see anything that detects mobile or behaves differently on desktop other than the isTouching event.

is there a point where we could know that we're not on mobile and pre-emptively unload the scroll handler?

it's too bad that the extra runtime overhead is only meant for mobile where it would matter more.

code wise nothing jumped out other than some places where I think we might be able to communicate more clearly what we're doing.

// 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.
Copy link
Member

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.

/**
 * 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
 * it's logic…
 *
 * @var {function}
 */
let scrollCallback;

/** 
 * Tracks whether user is touching screen; used to differentiate mobile and desktop behaviors.
 *
 * @var {boolean}
 */
let isTouching = false;

/** 
 * Used to differentiate mobile and desktop behaviors.
 *
 * @var {boolean}
 */
let isTouching = false;

/**
 * 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
 * because … a JavaScript version is required to avoid … in
 * order to provide the best visual experience.
 *
 * @param {object} context Interactivity page context?
 */
function handleScroll( context ) {
	
}

Copy link
Contributor

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.

// because the scroll event can't be canceled, so we reset the position instead.
window.scrollTo(
context.core.image.lastScrollLeft,
context.core.image.lastScrollTop
Copy link
Member

Choose a reason for hiding this comment

The 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 context.core.image.initialScrollLeft and initialScrollTop to me since they aren't updated; since they are resetting the scroll to the position it was in when initially opening the lightbox.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! While initialScrollTop and initialScrollLeft may be better, they still don't ring true to me, so I've renamed them to scrollTopReset and scrollLeftReset, which I think more clearly expresses the intent.

// 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.
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

@artemiomorales artemiomorales added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 27, 2023
Copy link
Contributor

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

Tested on a physical mobile device thanks to the Gutenberg PR Previewer

Looks great to me. Thanks Artemio! 👍

@artemiomorales
Copy link
Contributor

artemiomorales commented Sep 28, 2023

@dmsnell Thanks for taking a look!

there's talk in the patch about differentiating mobile and desktop behavior, but I didn't see anything that detects mobile or behaves differently on desktop other than the isTouching event.

is there a point where we could know that we're not on mobile and pre-emptively unload the scroll handler?

I looked further into this and we actually need to use both listeners, on mobile and desktop, the reason being that many devices support both touch and mouse input. The Surface Pro laptop is one example; one can even use a mouse with an iPad or an iPhone.

In these cases, we'll want to have the scroll callback, but still recognize the moment when touch input starts so that we can override the scroll reset and allow pinch to zoom to work as expected.

@artemiomorales artemiomorales enabled auto-merge (squash) September 28, 2023 17:09
@artemiomorales artemiomorales merged commit 9b2f4bf into trunk Sep 28, 2023
50 checks passed
@artemiomorales artemiomorales deleted the update/remove-scroll-hiding-lightbox branch September 28, 2023 17:12
@github-actions github-actions bot added this to the Gutenberg 16.8 milestone Sep 28, 2023
mikachan pushed a commit that referenced this pull request Sep 29, 2023
* 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 <[email protected]>
@mikachan
Copy link
Member

I just cherry-picked this PR to the commits-for-6.4-beta2 branch to get it included in the next release: 0d63803

@mikachan mikachan removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 29, 2023
fluiddot pushed a commit that referenced this pull request Oct 2, 2023
* Fix the install of system fonts from a font collection (#54713)

* Fix set upload dir test (#54762)

* Site Editor: Prevent unintended actions on the classic theme (#54422)

* Add action and selector to track access to Patterns page

* Add a URL parameter to check if the Patterns page was accessed directly

* Revert lib changes

* Fix critical error in the Post Editor

* Explicitly add `! isBlockBasedTheme` with `isTemplatePartsMode`

* Fix critical error in the Post Editor

* Patterns: Fix back navigation after pattern creation (#54852)

* Patterns: Fix category control width in site editor (#54853)

* Patterns: Allow non-user patterns under Standard filter (#54756)

* Performance Tests: Separate page setup from test (#53808)

# Conflicts:
#	test/performance/specs/post-editor.spec.js

* Performance Tests: Support legacy selector for expanded elements (#54690)

* Paragraph: Make 'aria-label' consistent with other blocks (#54687)

* Paragraph: Make 'aria-label' consistent with other blocks

* Update tests

* Try using BC labels in performance tests

* Move lightbox render function to filter (#54670)

* syntax refactor repace strpos with str_contains (#54832)

* Font Library: avoid deprected error in test (#54802)

* fix deprecated call

* removing unwanted line

* Fix the ShortcutProvider usage (#54851)

Co-authored-by: Marin Atanasov <[email protected]>

* Image: Ensure `false` values are preserved in memory when defined in `theme.json` (#54639)

* Modify conditional when parsing config

* Only drop the value if it's undefined.

---------

Co-authored-by: Michal Czaplinski <[email protected]>

* Use "Not synced" in place of "Standard" nomenclature for patterns (#54839)

* Standard -> Not synced

* Fix broken test

---------

Co-authored-by: Glen Davies <[email protected]>

* Format Library: Try to fix highlight popover jumping (#54736)

* Move mime-type collection generation to a function that can be tested… (#54844)

* Move mime-type collection generation to a function that can be tested.  Refactored to use that function.

* linting changes

* Add unit tests to mime type getter

* Fixed linting errors

* test the entire output array and replace assertTrue by assertEquals

* fixing docs

---------

Co-authored-by: Matias Benedetto <[email protected]>

* Ensure lightbox toggle is shown if block-level setting exists (#54878)

* Block Editor: Update strings in blocks 'RenameModal' component (#54887)

* Footnotes: Add aria-label to return links (#54843)

* Add aria-label to footnotes front-end links.

* Add visual output. Fix PHPCS errors.

* Remove visual changes, fix in follow-up.

* Font Library: Changed the OTF mime type expected value to be what PHP returns (#54886)

* Changed the OTF mime type expected value to be what PHP returns

* add unit test for otf file installation

---------

Co-authored-by: madhusudhand <[email protected]>

* Image: Fix layout shift when lightbox is opened and closed (#53026)

* 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 <[email protected]>

* Font Library: move font uploads to a new tab (#54655)

* move font uploads to a new tab in the modal

* fix invalid success message and revert the dropzone to modal

* add success notice for font uploads

* make supported file formats message dynamic based on allowed extensions

* update hint text and clear successful message with a fresh upload

* Block custom CSS: Fix incorrect CSS when multiple root selectors (#53602)

* Block custom CSS: Fix incorrect CSS when multiple root selectors

* Fix PHP lint error

* Use `scope_selector` and `append_to_selector` method and update unit test

* Use `scopeSelector` and `appendToSelector` function and update JS unit test

* Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js

Co-authored-by: Aaron Robertshaw <[email protected]>

* Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js

Co-authored-by: Aaron Robertshaw <[email protected]>

* Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js

Co-authored-by: Aaron Robertshaw <[email protected]>

* Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js

Co-authored-by: Aaron Robertshaw <[email protected]>

* Update packages/block-editor/src/components/global-styles/utils.js

Co-authored-by: Aaron Robertshaw <[email protected]>

* re-trigger CI

---------

Co-authored-by: Aaron Robertshaw <[email protected]>

* Add new e2e test for creating a pattern (#54855)

* Use list role instead of listbox for patterns (#54884)

* Popover: Fix the styles for components that use emotion within popovers (#54912)

# Conflicts:
#	packages/components/CHANGELOG.md

* Footnotes: use core’s meta revisioning if available (#52988)

# Conflicts:
#	packages/block-library/src/footnotes/index.php

* Remove base url from link control search results  (#54553)

* Expose baseURL setting on Post and Site Editors via block settings

* Strip baseURL from rendered search results

* Only fetch baseURL once in top level component

* Simplify implementation to utilise URL parse functions

* Improve comment wording to avoid referencing undefined var

* Remove superfluous conditional

* Decode URL prior to operations

* Refactor for readability

* Fix where url is not defined

* Revert change to filter util

* Ensure that filterURLForDisplay always receives a string as an arg

* Make e2e test locator less strict

* Prefer pipe

* Force remove trailing slash

* [Site Editor]: Update copy of using  the default template in a page (#54728)

* Remove overflow: hidden from the entity title h1 in the site editor sidebar (#54769)

* Site Editor: Avoid same key warnings in template parts area listings (#54863)

* TemplateAreas use template part clientId as key
* HomeTemplateDetails use template part clientId as key
* Cleanup useSelect hook

* Fix ToolSelector popover variant (#54840)

* Font Library: refactor endpoint permissions (#54829)

* break the checking of user permission and file write permissions

* return error 500 if the request to the install fonts endpoint needs write permissions and wordpress doens't have write permission on the server

* do not ask file write permission on uninstall endpoint

* Standardize the output of install and uninstall fonts endpoints

Co-authored-by: Jason Crist <[email protected]>
Co-authored-by: Jeff Ong <[email protected]>

* Handle standardized output from install and uninstall endpoints in the frontend

Co-authored-by: Jason Crist <[email protected]>
Co-authored-by: Jeff Ong <[email protected]>

* Update the install and unintall fonts endpoints unit tests for the new standardized output format

Co-authored-by: Jason Crist <[email protected]>
Co-authored-by: Jeff Ong <[email protected]>

* fix the refersh call for the library

Co-authored-by: Jason Crist <[email protected]>
Co-authored-by: Jeff Ong <[email protected]>

---------

Co-authored-by: Jason Crist <[email protected]>
Co-authored-by: Jeff Ong <[email protected]>

* Don’t use TypeScript files in scripts package (#54856)

* Fix Search Block not updating in Nav block (#54823)

* Avoid setState in render

* Attempt at test coverage

* Improve tests and make them work

* Remove word-wrap property (#54866)

---------

Co-authored-by: Matias Benedetto <[email protected]>
Co-authored-by: Aki Hamano <[email protected]>
Co-authored-by: Aaron Robertshaw <[email protected]>
Co-authored-by: Bart Kalisz <[email protected]>
Co-authored-by: George Mamadashvili <[email protected]>
Co-authored-by: Artemio Morales <[email protected]>
Co-authored-by: Riad Benguella <[email protected]>
Co-authored-by: Marin Atanasov <[email protected]>
Co-authored-by: Michal Czaplinski <[email protected]>
Co-authored-by: Rich Tabor <[email protected]>
Co-authored-by: Glen Davies <[email protected]>
Co-authored-by: Jason Crist <[email protected]>
Co-authored-by: Alex Stine <[email protected]>
Co-authored-by: madhusudhand <[email protected]>
Co-authored-by: Carlos Bravo <[email protected]>
Co-authored-by: Ricardo Artemio Morales <[email protected]>
Co-authored-by: Kai Hao <[email protected]>
Co-authored-by: Adam Silverstein <[email protected]>
Co-authored-by: Dave Smith <[email protected]>
Co-authored-by: Nik Tsekouras <[email protected]>
Co-authored-by: Ramon <[email protected]>
Co-authored-by: Jason Crist <[email protected]>
Co-authored-by: Jeff Ong <[email protected]>
Co-authored-by: Pascal Birchler <[email protected]>
@getdave
Copy link
Contributor

getdave commented Jan 23, 2024

✅ I updated the PHP Sync Tracking Issue for WP 6.5 to note this PR does not require a backport for WP 6.5 as it seems to have been cherry picked into WP 6.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block Needs Accessibility Feedback Need input from accessibility [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.