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: Reimplement lightbox trigger as a minimal button in corner of image #55212

Merged
merged 20 commits into from
Oct 17, 2023

Conversation

artemiomorales
Copy link
Contributor

@artemiomorales artemiomorales commented Oct 10, 2023

What?

This PR changes the lightbox trigger so that it exists in the top right corner of the image rather than overlaying it.

Why?

Addresses #55097
Having the lightbox overlay the entire image interrupts key browser functionality and is prone to failing with certain themes and layouts.

How?

The PR changes the styles of the button and removes the JavaScript handling that was calculating the button position dynamically.

Testing Instructions

  1. Add an image to a post
  2. Enable 'Expand on Click' in the image block settings
  3. Publish and view the post
  4. Hover over the image with a mouse and see that the lightbox trigger becomes visible in the top right corner. Notice that hovering over the button will cause the cursor to become a pointer. Try clicking on the button and ensure the lightbox opens.
  5. Notice also that hovering over the image causes the cursor to become a magnifying glass. Click on the image and ensure the lightbox opens.

Additionally:

  • Try adding a title to the image. Hover over the image and ensure the title appears in a tooltip.
  • Try right clicking on an image and ensuring the image contextual menu appears as expected in Chrome, Safari, Firefox, Opera.

Testing Instructions for Keyboard

  1. While viewing the post, tab to the image and make sure the lightbox trigger appears on focus.
  2. Press Enter or Spacebar to ensure the lightbox opens as expected.

Screenshot

Screenshot 2023-10-10 at 10 36 50 AM

@github-actions
Copy link

github-actions bot commented Oct 10, 2023

Size Change: +5.67 kB (0%)

Total Size: 1.66 MB

Filename Size Change
build/block-directory/index.min.js 7.25 kB +188 B (+3%)
build/block-editor/index.min.js 219 kB +243 B (0%)
build/block-editor/style-rtl.css 15.6 kB +15 B (0%)
build/block-editor/style.css 15.6 kB +15 B (0%)
build/block-library/blocks/image/style-rtl.css 1.54 kB +65 B (+4%)
build/block-library/blocks/image/style.css 1.54 kB +65 B (+4%)
build/block-library/blocks/image/view.min.js 2 kB +73 B (+4%)
build/block-library/index.min.js 211 kB +108 B (0%)
build/block-library/style-rtl.css 14.4 kB +63 B (0%)
build/block-library/style.css 14.4 kB +61 B (0%)
build/commands/index.min.js 15.5 kB +1 B (0%)
build/components/index.min.js 250 kB +1.39 kB (+1%)
build/components/style-rtl.css 11.9 kB +30 B (0%)
build/components/style.css 11.9 kB +30 B (0%)
build/core-commands/index.min.js 2.72 kB +3 B (0%)
build/core-data/index.min.js 70.9 kB +95 B (0%)
build/customize-widgets/index.min.js 12 kB -1 B (0%)
build/data/index.min.js 8.87 kB +87 B (+1%)
build/edit-post/index.min.js 35.7 kB +70 B (0%)
build/edit-post/style-rtl.css 7.88 kB -5 B (0%)
build/edit-post/style.css 7.88 kB -5 B (0%)
build/edit-site/index.min.js 205 kB +2.11 kB (+1%)
build/edit-site/style-rtl.css 14.2 kB +62 B (0%)
build/edit-site/style.css 14.2 kB +62 B (0%)
build/editor/index.min.js 45.9 kB -2 B (0%)
build/format-library/index.min.js 7.82 kB +19 B (0%)
build/patterns/index.min.js 4.51 kB +809 B (+22%) 🚨
build/private-apis/index.min.js 988 B +16 B (+2%)
build/reusable-blocks/index.min.js 2.73 kB +1 B (0%)
build/rich-text/index.min.js 10.2 kB +10 B (0%)
build/url/index.min.js 3.83 kB -12 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-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/form-input/editor-rtl.css 229 B
build/block-library/blocks/form-input/editor.css 228 B
build/block-library/blocks/form-input/style-rtl.css 343 B
build/block-library/blocks/form-input/style.css 343 B
build/block-library/blocks/form-submission-notification/editor-rtl.css 343 B
build/block-library/blocks/form-submission-notification/editor.css 342 B
build/block-library/blocks/form-submit-button/style-rtl.css 69 B
build/block-library/blocks/form-submit-button/style.css 69 B
build/block-library/blocks/form/view.min.js 452 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 103 B
build/block-library/blocks/navigation-link/style.css 103 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.02 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 345 B
build/block-library/blocks/post-featured-image/style.css 345 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 409 B
build/block-library/blocks/post-template/style.css 408 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/style-rtl.css 613 B
build/block-library/blocks/search/style.css 613 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.4 kB
build/block-library/editor.css 12.4 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/blocks/index.min.js 51.5 kB
build/commands/style-rtl.css 947 B
build/commands/style.css 942 B
build/compose/index.min.js 12.7 kB
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-widgets/index.min.js 17 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/keyboard-shortcuts/index.min.js 1.76 kB
build/keycodes/index.min.js 1.9 kB
build/list-reusable-blocks/index.min.js 2.21 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/index.min.js 2.01 kB
build/nux/style-rtl.css 775 B
build/nux/style.css 771 B
build/patterns/style-rtl.css 518 B
build/patterns/style.css 517 B
build/plugins/index.min.js 1.81 kB
build/preferences-persistence/index.min.js 1.85 kB
build/preferences/index.min.js 1.26 kB
build/primitives/index.min.js 994 B
build/priority-queue/index.min.js 1.52 kB
build/react-i18n/index.min.js 631 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/router/index.min.js 1.79 kB
build/server-side-render/index.min.js 1.96 kB
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/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/viewport/index.min.js 967 B
build/warning/index.min.js 259 B
build/widgets/index.min.js 7.18 kB
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

@artemiomorales artemiomorales added [Type] Bug An existing feature does not function as intended [Block] Image Affects the Image Block Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta [Feature] Interactivity API API to add frontend interactivity to blocks. labels Oct 11, 2023
@artemiomorales artemiomorales marked this pull request as ready for review October 11, 2023 02:09
@jasmussen jasmussen requested review from mtias, richtabor and a team October 11, 2023 09:09
@artemiomorales
Copy link
Contributor Author

I'm noting here that if we proceed with this, as part of it, we'll also be able to add an additional style declaration that would resolve the bug wherein theme styles conflict with the lightbox button in #54682

@michalczaplinski
Copy link
Contributor

I think that showing a button on hover only is a decent compromise, nice work @artemiomorales 👍

I see 2 issues here though:

  1. When using some custom aspect ratios, the button can appear outside the image:

    image.lightbox.issue.with.custom.aspect.ratio.mp4
  2. Images with rounded borders look a bit weird having the button in the corner which is actually outside of the image.

    Screenshot 2023-10-11 at 16 32 53

    Edge cases like this make me question a bit whether having this button is worth the trouble, but I don't want to reopen the discussion that was already had in Image block lightbox: do not overlay the image with a transparent button #55097. If we can fix 1., I'm fine with shipping the button in the corner of the image.

@artemiomorales
Copy link
Contributor Author

@michalczaplinski Great, thanks! I've just pushed a commit covering edge cases when using contain. Next step is to address scenarios wherein the image has custom width or height properties.

@artemiomorales
Copy link
Contributor Author

Do we need 25px offset? Cutting it to 10 reduces the display issue on smaller images:

@richtabor Thanks for taking a look! I just cut it to 10 pixels.

@SiobhyB
Copy link
Contributor

SiobhyB commented Oct 17, 2023

@artemiomorales, I wanted to check whether you're still aiming to get this into today's RC1? As a reminder, the commit freeze is at 15.00 UTC today, so this would ideally need to be merged a couple of hours before that to give us time to organise the sync and commit.

@aristath aristath self-requested a review October 17, 2023 09:26
@artemiomorales
Copy link
Contributor Author

I wanted to check whether you're still aiming to get this into today's RC1? As a reminder, the commit freeze is at 15.00 UTC today, so this would ideally need to be merged a couple of hours before that to give us time to organise the sync and commit.

@SiobhyB Yes, that's the hope! Although it needs a review and approval from someone else at this point. I've pinged a couple of folks so we'll see if they can take a look.

Comment on lines +285 to +292
// We want to avoid drawing unnecessary attention to the close
// button for mouse and touch users. Note that even if opening
// the lightbox via keyboard, the event fired is of type
// `pointerEvent`, so we need to rely on the `event.pointerType`
// property, which returns an empty string for keyboard events.
if ( context.core.image.pointerType === '' ) {
ref.querySelector( '.close-button' ).focus();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we want to focus the close button when opening the button by clicking the image? Right now, it keeps the active element outside of the modal, which I believe is not correct, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a point brought up some folks from design a while back — they did not want a close button at all because they felt it would distract and be unnecessary.

I figured adding this so the focus ring doesn't become visible and draw attention to the close button would be a good move. Personally I would prefer to not have my attention immediately drawn to a close button when trying to get a better view of an image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are potential accessibility issues here, though. The context would indeed be incorrect for folks who activate the lightbox using a mouse while also using a screen reader.

Is this a use case we should handle? I'm currently searching Google for guidance around this, and will also ping @afercia @joedolson @alexstine for feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that with that approach we are losing the focus and it is causing some issues. When opening an image by clicking on it:

  1. I can't close it with the ESC key.
  2. I can't close it navigating with the keyboard because I can't focus on the close button.

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

Let me prefix this by saying I understand this is not yet perfect... I can see a couple of edge-cases above that would warrant some more polishing.
However, as a whole, this implementation is a lot better than the one we currently have.
With that in mind, and understanding that we're on the clock for RC1, I'll go ahead and pre-approve this PR. This will give us the confidence that we have something better than what was previously there, while giving us another week to polish the styles based on feedback we receive from RC1 testers 👍

The code looks good, and this is definitely a welcomed improvement

Comment on lines +156 to +164
// We want to avoid drawing attention to the button
// after the lightbox closes for mouse and touch users.
// Note that the `event.pointerType` property returns
// as an empty string if a keyboard fired the event.
if ( event.pointerType === '' ) {
context.core.image.lastFocusedElement.focus( {
preventScroll: true,
} );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as with the close button: if we navigate to a lightbox with the keyboard, but I close it using a click, I lose the focus, which I assume is not correct, right?

Copy link
Contributor

@SantosGuillamot SantosGuillamot left a comment

Choose a reason for hiding this comment

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

I've looked at it, and, apart from a couple of comments I left, I believe the PR does what is indicated in the description. The only issue I can see is the one described here, but I believe we can keep it as a limitation for this RC and analyze it later.

Apart from that, I am a bit afraid of making big changes like these ones in a RC, but I'll let you decide on that.

@artemiomorales artemiomorales merged commit 017d1ad into trunk Oct 17, 2023
51 checks passed
@artemiomorales artemiomorales deleted the fix/lightbox-trigger branch October 17, 2023 11:33
@github-actions github-actions bot added this to the Gutenberg 16.9 milestone Oct 17, 2023
@artemiomorales
Copy link
Contributor Author

Let's also use the updated fullscreen icon from #55021.

@richtabor As we were pushing the RC1 deadline, I'll change this in a follow up PR.

SiobhyB pushed a commit that referenced this pull request Oct 17, 2023
…image (#55212)

* Reimplement lightbox trigger as a minimal button in corner of image

* Remove obsolete directives

* Update directives to fire logic properly via image or button click

* Ensure lightbox button only appears when hovering over image, not whole figure

* Ensure close button does not receive focus when opening lightbox via mouse

* Ensure button only receives focus when lightbox is closed via keyboard

* Add comments

* Prevent unnecessary focus being shown on mobile

* Add dynamic positioning for button when image uses 'contain' setting

* WORK IN PROGRESS - Begin accounting for various edge cases

We need to account for the fact that an image can have
custom dimensions, aspect ratio, cover or contain, captions,
thumbnail dimensions, and potentially other scenarios.

This commit begins to address those issues but notably fails
in cases where one uses a horizontal image, at full scale,
with custom aspect ratio, using 'contain'.

It seems to work in all other cases that I've checked but
needs more thorough testing and the code can probably be cleaner,
and may contain some unnecessary items.

* Simplify and improve button placement logic

* Simplify logic to show button on hover

* Fix styles

* Simplify calls to showLightbox

* Fix style inconsistency between browsers

* Change button position slightly

* Reduce button offset

* Add style override for better consistency across themes

* Fix logic so lightbox animates as intended; remove extraneous code

* Update comment
@SiobhyB
Copy link
Contributor

SiobhyB commented Oct 17, 2023

I've gone ahead to cherry-pick this to the release branch in 7bce633, so it'll be included in the RC. :)

@afercia
Copy link
Contributor

afercia commented Oct 17, 2023

I see this has been merged while I was reviewing it. No wirries, it can happen. My lunch break took more than expected I guess :)

However, there are issues that need to be fixed, not only accessibility but also the fact that:

  • event.pointerType is buggy across browsers
  • differentiating behavior depending on pointing device vs. 'other' is not ok for accessibility

@SiobhyB SiobhyB 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 Oct 17, 2023
@afercia
Copy link
Contributor

afercia commented Oct 17, 2023

So this will need a follow up for the next RC.

  • The implementation based on event.pointerType is buggy, as Chrome and Opera regurn an empty string as expected when using the keyboard while Firefox and Safari return undefined.
  • Regardless, differentiating the behavior depending on pointing device vs. 'other' is not OK for accessibility. The modal dialog focus management should always have the same behaevior regardless of mouse, touch, keyboard, other.
  • The assumption that users are always either using a pointing device or other is incorrect. For example, I may want to open the modal dialog with a click and close it by pewssing the Escape key.

Depending on the flow used to open the dialog and on the browser in use, this is what I observed when testing:

  • When opening the dialog, focus is not transferred to the dialog. As such:
    • Focus stays on the page behind the dialog. Tabbing will navigate the page content behing the modal dialog.
    • The dialgo is not announced by screen readers because focus is elsewhere.
    • The dialog doesn't close on Escape.
    • When closing the dialog, focus should always return to the trigger button, instead this is not the case.
    • As such, there is a full focus loss: tabbing again will restart the tab order from the document root.
  • The trigger button doesn't visually expose its accessible name. This is a broadeer issue with all icon buttons ued on the front end and should be addressed holistically.
  • The trigger button is only 24 by 24 pixels. While this is compliant with the current WCAG 2.2, I would like to see the button target size a little larger.
  • When opening the modal dialog, Initial focus should go to the element with role=dialog instead of the Close button.
  • The clickable image is problematic. I'm not sure why we want to make a HTML element have a non-native behavior. I'm not sure that would make it more usable in the first place, as it is a non expected beheavi, especially considering the following point. It is even more problematic on mobile.
  • Still, there is no visual hint that the image can be enlarged. It looks like a normal image and only when hovered or focusd shows a visual hint. As a user, looking at a page with a Lightbox image, I wouldn't have any clue that the image can be enlarged. See screnshot below: how users are supposed to understand the image can be enlarged?

Screenshot 2023-10-17 at 13 12 47

SiobhyB pushed a commit that referenced this pull request Oct 17, 2023
* Unconditionally load the Gutenberg copy of the HTML Tag Processor (#55404)

Temporary fix to resolve breaking tests in `trunk` after the WordPress 6.4 backport.

* Revert "Unconditionally load the Gutenberg copy of the HTML Tag Processor (#55404)"

This reverts commit 271587a.

* Image: Reimplement lightbox trigger as a minimal button in corner of image (#55212)

* Reimplement lightbox trigger as a minimal button in corner of image

* Remove obsolete directives

* Update directives to fire logic properly via image or button click

* Ensure lightbox button only appears when hovering over image, not whole figure

* Ensure close button does not receive focus when opening lightbox via mouse

* Ensure button only receives focus when lightbox is closed via keyboard

* Add comments

* Prevent unnecessary focus being shown on mobile

* Add dynamic positioning for button when image uses 'contain' setting

* WORK IN PROGRESS - Begin accounting for various edge cases

We need to account for the fact that an image can have
custom dimensions, aspect ratio, cover or contain, captions,
thumbnail dimensions, and potentially other scenarios.

This commit begins to address those issues but notably fails
in cases where one uses a horizontal image, at full scale,
with custom aspect ratio, using 'contain'.

It seems to work in all other cases that I've checked but
needs more thorough testing and the code can probably be cleaner,
and may contain some unnecessary items.

* Simplify and improve button placement logic

* Simplify logic to show button on hover

* Fix styles

* Simplify calls to showLightbox

* Fix style inconsistency between browsers

* Change button position slightly

* Reduce button offset

* Add style override for better consistency across themes

* Fix logic so lightbox animates as intended; remove extraneous code

* Update comment

* [Edit Widgets] Only suppress admin notices when JS enabled. (#55403)

Only suppresses the display of notices on the widget block editor screen if JavaScript is available.

---------

Co-authored-by: Dennis Snell <[email protected]>
Co-authored-by: Artemio Morales <[email protected]>
Co-authored-by: Peter Wilson <[email protected]>
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Oct 17, 2023
Updates for needed bugfixes in RC1:

* [WordPress/gutenberg#55212 Image: Reimplement lightbox trigger as a minimal button in corner of image]

* [WordPress/gutenberg#55403 [Edit Widgets] Only suppress admin notices when JS enabled.]

Follow-up to [56849], [56818], [56816].

Props artemiosans, jameskoster, SantosGuillamot, aristath, czapla, joen, afercia, richtabor, peterwilsoncc, andraganescu, hellofromTonya, siobhyb.
See #59411.

git-svn-id: https://develop.svn.wordpress.org/trunk@56961 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Oct 17, 2023
Updates for needed bugfixes in RC1:

* [WordPress/gutenberg#55212 Image: Reimplement lightbox trigger as a minimal button in corner of image]

* [WordPress/gutenberg#55403 [Edit Widgets] Only suppress admin notices when JS enabled.]

Follow-up to [56849], [56818], [56816].

Props artemiosans, jameskoster, SantosGuillamot, aristath, czapla, joen, afercia, richtabor, peterwilsoncc, andraganescu, hellofromTonya, siobhyb.
See #59411.
Built from https://develop.svn.wordpress.org/trunk@56961


git-svn-id: http://core.svn.wordpress.org/trunk@56472 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Oct 17, 2023
Updates for needed bugfixes in RC1:

* [WordPress/gutenberg#55212 Image: Reimplement lightbox trigger as a minimal button in corner of image]

* [WordPress/gutenberg#55403 [Edit Widgets] Only suppress admin notices when JS enabled.]

Follow-up to [56849], [56818], [56816].

Props artemiosans, jameskoster, SantosGuillamot, aristath, czapla, joen, afercia, richtabor, peterwilsoncc, andraganescu, hellofromTonya, siobhyb.
See #59411.
Built from https://develop.svn.wordpress.org/trunk@56961


git-svn-id: https://core.svn.wordpress.org/trunk@56472 1a063a9b-81f0-0310-95a4-ce76da25c4cd
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 [Feature] Interactivity API API to add frontend interactivity to blocks. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image block lightbox: do not overlay the image with a transparent button
8 participants